On Mon, 23 Jun 2014, Todd Fiala wrote:
Hi Paul,
>I guess we're not going to require access to kernel headers for lldb
cross-compilation, so your fix should be a better solution.
Yeah I think we may want to go back and change that last commit to use the
other define-based approach. That'll allow it to work on systems where the
cross-compiler doesn't have the headers but the target
really supports the values. If they're not supported, we'll still get the
error on trying to ptrace with the (unsupported) values in that case. Sound
reasonable?
That's another thing I did not notice at first. We should not do #ifdef on
enums - they are not a part of preprocessor world. Try to compile
following code:
#include <stdio.h>
enum Things
{
THING_ONE = 0,
THING_TWO = 1
};
#ifdef THING_ONE
#warning there is THING_ONE
#else
#warning there is NOT THING_ONE
#define THING_ONE 0
#endif
int main()
{
printf("THING_ONE: %d\n", THING_ONE);
return 0;
}
Compile time warning is:
enum.c:12:2: warning: #warning there is NOT THING_ONE [-Wcpp]
In sys/ptrace.h we have:
enum __ptrace_request
{
/* Indicate that the process making this request should be traced.
All signals received by this process can be intercepted by its
parent, and its parent can use the other `ptrace' requests. */
...
/* Get all general purpose registers used by a processes.
This is not supported on all machines. */
PTRACE_GETREGS = 12,
#define PT_GETREGS PTRACE_GETREGS
...
/* Get register content. */
PTRACE_GETREGSET = 0x4204,
#define PTRACE_GETREGSET PTRACE_GETREGSET
...
}
So following code:
// Support ptrace extensions even when compiled without required kernel support
#ifndef PTRACE_GETREGSET
#define PTRACE_GETREGSET 0x4204
#endif
...is OK quite accidently, due to this line: #define PTRACE_GETREGSET
PTRACE_GETREGSET
however,
#ifndef PTRACE_GETREGS
#define PTRACE_GETREGS 12
#endif
...will make AArch64 happy, but on desktop PC Linux architectures it will
always redefine enum value with preprocessor constant. In this case it is
the same literal value (12) but still it looks risky.
Something like this would be safer (if you don't want to limit program
capabilities the way I did in my original patch):
#ifdef __aarch64__
enum __ptrace_request_compat
{
PTRACE_GETREGS = 12,
PTRACE_SETREGS = 13,
PTRACE_GETFPREGS = 14,
PTRACE_SETFPREGS = 15
};
#endif
> Is NativeProcessLinux.cpp intented to replace Linux/ProcessMonitor.cpp? I
gave your branch a try and found that now, both are compiled.
Right - Greg Clayton and I decided to put the NativeProcessLinux in llgs
side-by-side so I didn't have to change the world at the same time I'm trying
to get llgs up and running. The intent was to get
NativeProcessProtocol, NativeThreadProtocol, NativeRegisterContext and
GDBRemoteCommunicationServer (lldb-gdbserver/lldb-platform) up and running
first, working the kinks out of it. Then, we can rework
local debugging and process monitoring to use the Native* classes as a
follow-up pass. That will enable supporting a remote and local debugging
scenario for a new os/arch with a single set of code for both
remote and local debugging rather than writing a separate remote and local path.
On Mon, Jun 23, 2014 at 1:14 PM, Paul Osmialowski <[email protected]> wrote:
Hi Todd,
These values looks ok. Although they cannot be found in sys/ headers
shipped within Linaro's toolchain sysroot for AArch64, I could find them in
AArch64-related headers that are part of Linux
kernel source code:
arch/arm64/include/asm/ptrace.h:
/* AArch32-specific ptrace requests */
#define COMPAT_PTRACE_GETREGS 12
#define COMPAT_PTRACE_SETREGS 13
#define COMPAT_PTRACE_GET_THREAD_AREA 22
#define COMPAT_PTRACE_SET_SYSCALL 23
#define COMPAT_PTRACE_GETVFPREGS 27
#define COMPAT_PTRACE_SETVFPREGS 28
#define COMPAT_PTRACE_GETHBPREGS 29
#define COMPAT_PTRACE_SETHBPREGS 30
#define COMPAT_PSR_MODE_USR 0x00000010
#define COMPAT_PSR_T_BIT 0x00000020
#define COMPAT_PSR_IT_MASK 0x0600fc00 /* If-Then execution
state mask */
I guess we're not going to require access to kernel headers for lldb
cross-compilation, so your fix should be a better solution.
Is NativeProcessLinux.cpp intented to replace Linux/ProcessMonitor.cpp? I
gave your branch a try and found that now, both are compiled.
On Mon, 23 Jun 2014, Todd Fiala wrote:
FWIW on the NativeProcessLinux side (llgs branch) I'm solving this
more like how the existing code did it:
--- a/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -52,6 +52,12 @@
#define DEBUG_PTRACE_MAXBYTES 20
// Support ptrace extensions even when compiled without required
kernel support
+#ifndef PTRACE_GETREGS
+#define PTRACE_GETREGS 12
+#endif
+#ifndef PTRACE_SETREGS
+ #define PTRACE_SETREGS 13
+#endif
#ifndef PTRACE_GETREGSET
#define PTRACE_GETREGSET 0x4204
#endif
That approach should also clear up the cross compile issues. More
importantly, if it's really running this code on the target, where the ptrace
support really does exist, then the
code would still run
properly.
On Mon, Jun 23, 2014 at 9:07 AM, Todd Fiala <[email protected]>
wrote:
svn commit
Sending source/Plugins/Process/Linux/ProcessMonitor.cpp
Transmitting file data .
Committed revision 211503.
http://reviews.llvm.org/D4091
--
Todd Fiala |
Software Engineer |
[email protected] |
650-943-3180
--
Todd Fiala |
Software Engineer |
[email protected] |
650-943-3180
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
