[lldb][AIX] Added Ptrace extensions for AIX #108000
Conversation
|
@llvm/pr-subscribers-lldb Author: Dhruv Srivastava (DhruvSrivastavaX) ChangesThis PR is in reference to porting LLDB on AIX. Link to discussions on llvm discourse and github:
Added Ptrace extensions for AIX.
Review Request: @DavidSpickett Full diff: https://github.com/llvm/llvm-project/pull/108000.diff 1 Files Affected:
diff --git a/lldb/include/lldb/Host/aix/Ptrace.h b/lldb/include/lldb/Host/aix/Ptrace.h
new file mode 100644
index 00000000000000..5d5ae82c9dab7d
--- /dev/null
+++ b/lldb/include/lldb/Host/aix/Ptrace.h
@@ -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
+
+// Support ptrace extensions even when compiled without required kernel support
+#ifndef PTRACE_GETREGS
+#define PTRACE_GETREGS (PT_COMMAND_MAX + 1)
+#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_
|
There was a problem hiding this comment.
This is exactly the style of PR I was looking for, thanks.
I don't want this to land without some code that uses it however. So I suggest we iterate on this until it's approved, then you put up the next PR and so on. We'll find some later point where it makes sense to start landing them.
lldb/include/lldb/Host/aix/Ptrace.h
Outdated
|
|
||
| #include <sys/ptrace.h> | ||
|
|
||
| #define DEBUG_PTRACE_MAXBYTES 20 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Xmay 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
There was a problem hiding this comment.
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));
....
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Okay, sure. I will try that implementation as well.
Glad to hear that! Sure that should be okay. I can proceed with the files which directly rely on this one for the next PRs, so that we can plan accordingly. |
|
Given #108000 (comment), my feeling is that this file just should not (need not) exist. For now, I'd suggest moving the header into NativeProcessAIX (that's the only place which uses it, right?), and then we can figure out what to do with it when we get to that. |
|
(NativeProcessAIX is one of the classes which I'm certain they will not be shared with their linux counterpart, so there's really no reason to emulate linux implementation if it does not come natural. And "inventing" OS constants to make the code layout similar definitely falls in that category.) |
So based on this, shall I move this file in the same path as NativeProcessAIX path for now? |
You can do that as a temporary fix so that we can move on to dealing with other Host functionality. In the longer run I'll still most likely want to get rid of those constants by doing something like I described in that comment. |
Yes, agreed. If we do not need be keep the code strictly similar to Linux, that approach can be better. |
This PR is in reference to porting LLDB on AIX.
Link to discussions on llvm discourse and github:
The complete changes for porting are present in this draft PR:
Extending LLDB to work on AIX #102601
Added Ptrace extensions for AIX.
Details about ptrace on AIX: ptrace
Review Request: @DavidSpickett