Skip to content

[lldb][AIX] Added Ptrace extensions for AIX #108000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

[lldb][AIX] Added Ptrace extensions for AIX #108000

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
426874a
Ptrace code base for AIX
DhruvSrivastavaX Sep 10, 2024
61bdaf7
Modified specific to AIX
DhruvSrivastavaX Sep 10, 2024
1134b6f
Addressed comments
DhruvSrivastavaX Sep 10, 2024
730c78d
Modified path for usage
DhruvSrivastavaX Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions lldb/include/lldb/Host/aix/Ptrace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
//===-- Ptrace.h ------------------------------------------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// This file defines ptrace functions & structures

#ifndef liblldb_Host_aix_Ptrace_h_
#define liblldb_Host_aix_Ptrace_h_

#include <sys/ptrace.h>

#define DEBUG_PTRACE_MAXBYTES 20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this and should it be declared like:

#ifndef DEBUG_PTRACE_MAXBYTES
#define ...
#endif

In case the build machine has a kernel that has changed this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lldb-specific macro that shouldn't be here even on linux (it's just used in one place, and might as well be inlined there). Let's delete it from here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be specific, it is being used in the NativeProcessLinux.cpp. For our context, it is used similarly for NativeProcessAIX.cpp, utilised by DisplayBytes(..)<--PtraceDisplayBytes(..).
We can move it to the same file if you suggest so and in the same way as David suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please do that. (Alternatively, we could move this constant to some common header, if you think that having a consistent number of logged bytes is useful (I don't think that)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I can define it with the given value in that file locally and use it, and going forward we can update/get it removed if that is a better alternative.


// Support ptrace extensions even when compiled without required kernel support
#ifndef PTRACE_GETREGS
#define PTRACE_GETREGS (PT_COMMAND_MAX + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PT_COMMAND_MAX is provided by some AIX include file, correct?

If that's correct then using it like this is fine because PTrace.h is only included in code built natively.

Do you have public documentation for these ptrace numbers? Perhaps there is a man page like https://man7.org/linux/man-pages/man2/ptrace.2.html? It would be good to include a link to that in the PR description.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just add that all of these defines are only required if your system headers don't always provide these definitions. This was necessary on linux (in the past, maybe not in present), because people built lldb on all kinds of kernel versions. If you're only going to support building lldb on AIX version >=X (where X may even be the most recent version of the OS) then you only need to provide the symbols that aren't available on every supported version. If all the supported versions of AIX define these symbols, then you don't need to define anything here (and maybe you don't even need this file).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to avoid confusion, this comment was actually question: Is it the case that the AIX system headers (on all supported versions) do not already define these constants?

Copy link
Member Author

@DhruvSrivastavaX DhruvSrivastavaX Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PT_COMMAND_MAX is provided by some AIX include file, correct?

If that's correct then using it like this is fine because PTrace.h is only included in code built natively.

Do you have public documentation for these ptrace numbers? Perhaps there is a man page like https://man7.org/linux/man-pages/man2/ptrace.2.html? It would be good to include a link to that in the PR description.

Although it is defined in AIX's system header sys/ptrace.h , its not been mentioned in AIX public documentation.
For other details, here is the link though:
https://www.ibm.com/docs/en/aix/7.3?topic=p-ptrace-ptracex-ptrace64-subroutine

I'll just add that all of these defines are only required if your system headers don't always provide these definitions. This was necessary on linux (in the past, maybe not in present), because people built lldb on all kinds of kernel versions. If you're only going to support building lldb on AIX version >=X (where X may even be the most recent version of the OS) then you only need to provide the symbols that aren't available on every supported version. If all the supported versions of AIX define these symbols, then you don't need to define anything here (and maybe you don't even need this file).

We are actually using these defines do some emulation in the NativeProcess files, hence the need for them. Please take a look here:
https://github.com/llvm/llvm-project/pull/102601/files#diff-932a7c13bbba2ce92d14f75c525489f726af9e95f955df24b3f840727e9f757a

Copy link
Member Author

@DhruvSrivastavaX DhruvSrivastavaX Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pasting a small snippet from that here:
line: 1755

  if (req == PTRACE_GETREGS) {
    GetRegister(pid, GPR0, &(((GPR *)data)->r0));
    GetRegister(pid, GPR1, &(((GPR *)data)->r1));
    GetRegister(pid, GPR2, &(((GPR *)data)->r2));
    GetRegister(pid, GPR3, &(((GPR *)data)->r3));
    GetRegister(pid, GPR4, &(((GPR *)data)->r4));
    GetRegister(pid, GPR5, &(((GPR *)data)->r5));
    GetRegister(pid, GPR6, &(((GPR *)data)->r6));
    GetRegister(pid, GPR7, &(((GPR *)data)->r7));
    GetRegister(pid, GPR8, &(((GPR *)data)->r8));
    GetRegister(pid, GPR9, &(((GPR *)data)->r9));
    GetRegister(pid, GPR10, &(((GPR *)data)->r10));
    ....

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so if I understand what you're saying, these definitions don't correspond to any actual values defined or supported by the system.

In that case, I think these values do not belong here, as this is basically an OS compatibility header. In fact, I think there's no reason for these constants should exist. You don't have to follow the patterns in NativeProcessLinux, if they don't make sense for you. There's nothing forcing you do implement ReadGPR like this


Status NativeRegisterContextAIX::ReadGPR() {
  return NativeProcessAIX::PtraceWrapper(
      PTRACE_GETREGS, m_thread.GetID(), nullptr, GetGPRBuffer(), GetGPRSize());
}

If your host ptrace call does not support reading GPRs in bulk. If you need to read registers one by one, the most obvious implementation is to just do that directly inside the ReadGPR function -- basically inline the relevant part of PtraceWrapper into this function (and then get rid of the constant).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sure. I will try that implementation as well.

#endif
#ifndef PTRACE_SETREGS
#define PTRACE_SETREGS (PT_COMMAND_MAX + 2)
#endif
#ifndef PTRACE_GETFPREGS
#define PTRACE_GETFPREGS (PT_COMMAND_MAX + 3)
#endif
#ifndef PTRACE_SETFPREGS
#define PTRACE_SETFPREGS (PT_COMMAND_MAX + 4)
#endif
#ifndef PTRACE_GETREGSET
#define PTRACE_GETREGSET 0x4204
#endif
#ifndef PTRACE_SETREGSET
#define PTRACE_SETREGSET 0x4205
#endif
#ifndef PTRACE_GETVRREGS
#define PTRACE_GETVRREGS (PT_COMMAND_MAX + 5)
#endif
#ifndef PTRACE_GETVSRREGS
#define PTRACE_GETVSRREGS (PT_COMMAND_MAX + 6)
#endif

#endif // liblldb_Host_aix_Ptrace_h_
Loading