|
From: <sv...@va...> - 2013-07-25 20:40:25
|
philippe 2013-07-25 21:40:17 +0100 (Thu, 25 Jul 2013)
New Revision: 13471
Log:
do not include linux/ptrace.h in vgdb.c
Include of linux/ptrace.h was added in revision r11740,
to avoid compilation error on s390x (fedora and suse).
The compilation error was retrieved thanks to archeological research
done by Christian Borntraeger: without this include, the following was given:
error: 'PT_ENDREGS' undeclared
There was also some errors on ppc64 around the same time:
error: 'PTRACE_GETREGS' undeclared
Currently, the inclusion of linux/ptrace.h gives a problem on amd64/fedora20:
/usr/include/linux/ptrace.h:58:8: error: redefinition of ‘struct ptrace_peeksiginfo_args’
/usr/include/sys/ptrace.h:191:8: note: originally defined here
According to man ptrace, it is good enough to include sys/ptrace.h
(which should avoid the problem on amd64/f20).
The linux/ptrace.h is deemed not necessary anymore as:
1. Christian has tested on sles11sp2 on s390x.
2. since linux/ptrace.h was added in vgdb.c, #ifdef PT_ENDREGS and
#ifdef PTRACE_GETREGS were added
=> remove the linux/ptrace.h
(tested on x86/f12, ppc64/f18, amd64/deb6, sles11sp2/s390x)
Thanks to Christian for the investigations
Modified files:
trunk/coregrind/vgdb.c
Modified: trunk/coregrind/vgdb.c (+0 -3)
===================================================================
--- trunk/coregrind/vgdb.c 2013-07-25 09:22:08 +01:00 (rev 13470)
+++ trunk/coregrind/vgdb.c 2013-07-25 21:40:17 +01:00 (rev 13471)
@@ -100,10 +100,7 @@
#if defined(PTRACEINVOKER)
#include <sys/user.h>
-#if defined(VGO_linux)
-# include <linux/ptrace.h>
#endif
-#endif
// Outputs information for the user about ptrace not working.
|
|
From: Dejan J. <dej...@rt...> - 2013-07-31 07:14:09
Attachments:
patch_gdb_mips1.diff
|
On 07/25/2013 10:40 PM, sv...@va... wrote: > philippe 2013-07-25 21:40:17 +0100 (Thu, 25 Jul 2013) > > New Revision: 13471 > > Log: > do not include linux/ptrace.h in vgdb.c > > Include of linux/ptrace.h was added in revision r11740, > to avoid compilation error on s390x (fedora and suse). > > The compilation error was retrieved thanks to archeological research > done by Christian Borntraeger: without this include, the following was given: > error: 'PT_ENDREGS' undeclared > There was also some errors on ppc64 around the same time: > error: 'PTRACE_GETREGS' undeclared > > Currently, the inclusion of linux/ptrace.h gives a problem on amd64/fedora20: > /usr/include/linux/ptrace.h:58:8: error: redefinition of ‘struct ptrace_peeksiginfo_args’ > /usr/include/sys/ptrace.h:191:8: note: originally defined here > > According to man ptrace, it is good enough to include sys/ptrace.h > (which should avoid the problem on amd64/f20). > > The linux/ptrace.h is deemed not necessary anymore as: > 1. Christian has tested on sles11sp2 on s390x. > 2. since linux/ptrace.h was added in vgdb.c, #ifdef PT_ENDREGS and > #ifdef PTRACE_GETREGS were added > > => remove the linux/ptrace.h > (tested on x86/f12, ppc64/f18, amd64/deb6, sles11sp2/s390x) > > Thanks to Christian for the investigations > > Modified files: > trunk/coregrind/vgdb.c > > > Modified: trunk/coregrind/vgdb.c (+0 -3) > =================================================================== > --- trunk/coregrind/vgdb.c 2013-07-25 09:22:08 +01:00 (rev 13470) > +++ trunk/coregrind/vgdb.c 2013-07-25 21:40:17 +01:00 (rev 13471) > @@ -100,10 +100,7 @@ > > #if defined(PTRACEINVOKER) > #include <sys/user.h> > -#if defined(VGO_linux) > -# include <linux/ptrace.h> > #endif > -#endif > > > // Outputs information for the user about ptrace not working. > > > ------------------------------------------------------------------------------ > See everything from the browser to the database with AppDynamics > Get end-to-end visibility with application monitoring from AppDynamics > Isolate bottlenecks and diagnose root cause in seconds. > Start your free trial of AppDynamics Pro today! > http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers Hello all, in revision 13471 Philippe deleted include <linux/ptrace.h>. In getregs() and setregs() we have # ifdef PTRACE_GETREGS and I think that PTRACE_GETREGS is defined in <linux/ptrace.h>. Because of that the first part of code will not be compiled. In the second part of getregs and setregs we call ptrace and for mips the offset is in range 0,1,2...31 (not 0, 4, 8...). I attached a small patch that fix the second problem for mips, and if it's ok with you I can apply it. Regards, Dejan |
|
From: Philippe W. <phi...@sk...> - 2013-07-31 22:13:27
|
On Wed, 2013-07-31 at 09:13 +0200, Dejan Jevtic wrote:
> in revision 13471 Philippe deleted include <linux/ptrace.h>.
> In getregs() and setregs() we have # ifdef PTRACE_GETREGS and I think
> that PTRACE_GETREGS is defined in <linux/ptrace.h>. Because of that
> the first part of code will not be compiled.
> In the second part of getregs and setregs we call ptrace and for
> mips the offset is in range 0,1,2...31 (not 0, 4, 8...).
> I attached a small patch that fix the second problem for mips, and
> if it's ok with you I can apply it.
I do not have access to a mips machine for the moment (gcc farm mips
systems seems down for the moment) so cannot look in depth at
the problem.
At least on x86, amd64, PTRACE_GETREGS is defined in sys/ptrace.h.
On ppc64, PTRACE_GETREGS is not defined in sys/ptrace.h but
also not in linux/ptrace.h
What do you find in mips sys/ptrace.h and linux/ptrace.h ?
(it looks better to use PTRACE_(GET/SET)REGS if possible,
as there are more chance that all needed registers are
fetched/restored than if the registers are fetched individually
with PEEKUSER.
Otherwise, if the PEEKUSER solution must be made working on mips,
I have a few comments:
The patch is removing the sign extension
of $a0 and the "0" assignment to the "2nd register"
of the register pair (e.g.
user_mod.regs[34*2] = shared32->invoke_gdbserver;
user_mod.regs[34*2+1] = 0;
is replaced by
user_mod.regs[EF_CP0_EPC] = shared32->invoke_gdbserver;
The sign extension and 0 assignment were added after a discussion
with Petar as otherwise a vgdb compiled in 32 bits was not working
on a 64 bits arch.
Isn't it better to keep these assignments ?
(if valgrind is compiled in dual arch, then vgdb is a 64 bits
application).
2nd question: looking (on the net) the user.h, it seems there
are some other registers e.g. EF_LO and EF_HI, and some others
following EF_CP0_EPC.
Shouldn't these registers also be saved and restored via PEEKUSER ?
It is not clear to me why the current code which should peek/poke
the full "regs" structure is not ok ?
i.e. in asm-mips/user.h:
struct user {
unsigned long regs[EF_SIZE/4+64]; /* integer and fp regs */
(I have an almost zero knowledge of mips and no access to a mips
system, so the above might just be rubbish comments).
Would be better in any case to validate the changes
using the combinations vgdb32 on 32bits os
vgdb32 on 64bits os
(so valgrind compiled single arch 32 bits)
vgdb64 debugging a 32 and 64 bits executable
Thanks
Philippe
|
|
From: Dejan J. <dej...@rt...> - 2013-08-01 11:44:19
|
On 08/01/2013 12:13 AM, Philippe Waroquiers wrote: > On Wed, 2013-07-31 at 09:13 +0200, Dejan Jevtic wrote: > >> in revision 13471 Philippe deleted include <linux/ptrace.h>. >> In getregs() and setregs() we have # ifdef PTRACE_GETREGS and I think >> that PTRACE_GETREGS is defined in <linux/ptrace.h>. Because of that >> the first part of code will not be compiled. >> In the second part of getregs and setregs we call ptrace and for >> mips the offset is in range 0,1,2...31 (not 0, 4, 8...). >> I attached a small patch that fix the second problem for mips, and >> if it's ok with you I can apply it. > I do not have access to a mips machine for the moment (gcc farm mips > systems seems down for the moment) so cannot look in depth at > the problem. > > At least on x86, amd64, PTRACE_GETREGS is defined in sys/ptrace.h. > On ppc64, PTRACE_GETREGS is not defined in sys/ptrace.h but > also not in linux/ptrace.h I checked this on gcc110 (ppc64) and I think that PTRACE_GETREGS is defined in asm/ptrace.h that is included in linux/ptrace.h > What do you find in mips sys/ptrace.h and linux/ptrace.h ? For mips PTRACE_GETREGS is defined in asm/ptrace.h that is included in linux/ptrace.h. In sys/ptrace.h for mips PTRACE_GETREGS is part of the enum. We need to include asm/ptrace.h or linux/ptrace.h if we want to preprocessor see PTRACE_GETREGS in setregs/getregs: # ifdef PTRACE_GETREGS ... # endif > (it looks better to use PTRACE_(GET/SET)REGS if possible, > as there are more chance that all needed registers are > fetched/restored than if the registers are fetched individually > with PEEKUSER. > > Otherwise, if the PEEKUSER solution must be made working on mips, > I have a few comments: > The patch is removing the sign extension > of $a0 and the "0" assignment to the "2nd register" > of the register pair (e.g. > user_mod.regs[34*2] = shared32->invoke_gdbserver; > user_mod.regs[34*2+1] = 0; > is replaced by > user_mod.regs[EF_CP0_EPC] = shared32->invoke_gdbserver; > > The sign extension and 0 assignment were added after a discussion > with Petar as otherwise a vgdb compiled in 32 bits was not working > on a 64 bits arch. > Isn't it better to keep these assignments ? For now we are trying for fix all the tests only for mips32/32bit os and mips64/64bit os. Valgrind for mips can't be compiled in dual arch for now. > (if valgrind is compiled in dual arch, then vgdb is a 64 bits > application). > > > 2nd question: looking (on the net) the user.h, it seems there > are some other registers e.g. EF_LO and EF_HI, and some others > following EF_CP0_EPC. > Shouldn't these registers also be saved and restored via PEEKUSER ? From invoke_gdbserver() I see that we are getting all registers with getregs(), modify only some of them, and then with setregs() we are writing these new values back. With PTRACE_PEEKUSER we are getting only one single registers, and I thought that would be better only to get some of the registers that we need to modify, because with PTRACE_POKEUSER we are writing back only one register at the time. Am I missing something? > It is not clear to me why the current code which should peek/poke > the full "regs" structure is not ok ? When we are using ptrace() syscall with PTRACE_PEEKUSER/PTRACE_POKEUSER mips kernel is expecting that the offset will be the number of register that need to be return as a return value. You can find more details on (search for the PTRACE_PEEKUSR/PTRACE_POKEUSR): http://lxr.free-electrons.com/source/arch/mips/kernel/ptrace.c > i.e. in asm-mips/user.h: > struct user { > unsigned long regs[EF_SIZE/4+64]; /* integer and fp regs */ > > (I have an almost zero knowledge of mips and no access to a mips > system, so the above might just be rubbish comments). > > Would be better in any case to validate the changes > using the combinations vgdb32 on 32bits os > vgdb32 on 64bits os > (so valgrind compiled single arch 32 bits) > vgdb64 debugging a 32 and 64 bits executable > > Thanks > > Philippe > > > > Regards, Dejan |
|
From: Philippe W. <phi...@sk...> - 2013-08-02 22:44:09
|
On Thu, 2013-08-01 at 13:44 +0200, Dejan Jevtic wrote:
> On 08/01/2013 12:13 AM, Philippe Waroquiers wrote:
> > On Wed, 2013-07-31 at 09:13 +0200, Dejan Jevtic wrote:
> > At least on x86, amd64, PTRACE_GETREGS is defined in sys/ptrace.h.
> > On ppc64, PTRACE_GETREGS is not defined in sys/ptrace.h but
> > also not in linux/ptrace.h
> I checked this on gcc110 (ppc64) and I think that PTRACE_GETREGS is
> defined in asm/ptrace.h that is included in linux/ptrace.h
Yes you are correct, on ppc64/gcc110, PTRACE_GETREGS is defined in
asm/ptrace.h, which is indirectly included via the inclusion of
sys/user.h.
Now, checking more in depth what is happening on x86 and amd64, it
looks like PTRACE_GETREGS is not defined anymore since
linux/ptrace.h has been removed.
So, behaviour similar to mips.
> > The sign extension and 0 assignment were added after a discussion
> > with Petar as otherwise a vgdb compiled in 32 bits was not working
> > on a 64 bits arch.
> > Isn't it better to keep these assignments ?
> For now we are trying for fix all the tests only for mips32/32bit os
> and mips64/64bit os. Valgrind for mips can't be compiled in dual arch
> for now.
But it used to work when compiled in 32 bits, running on 64 bits,
but only with these sign extensions and 0 assignment.
So, it looks better to keep them, no ?
> > 2nd question: looking (on the net) the user.h, it seems there
> > are some other registers e.g. EF_LO and EF_HI, and some others
> > following EF_CP0_EPC.
> > Shouldn't these registers also be saved and restored via PEEKUSER ?
> From invoke_gdbserver() I see that we are getting all registers with
> getregs(), modify only some of them, and then with setregs() we are
> writing these new values back. With PTRACE_PEEKUSER we are getting only
> one single registers, and I thought that would be better only to get
> some of the registers that we need to modify, because with
> PTRACE_POKEUSER we are writing back only one register at the time.
> Am I missing something?
I believe it is better (or even needed?) to
1. save all registers values,
2. set some registers, needed to invoke gdbserver
(at this point, gdbserver invocation might modify
other registers)
3. set all registers to the values saved at 1.
So, GETREGS/SETREGS looks safer to use, and if using PEEK/POKE,
we better save/restore all registers.
Now, I see that by removing linux/ptrace.h, at least x86, amd64 and mips
have switched to PEEK/POKE, while it is better to use GET/SET REGS
(at least gdb also first tries to use GET/SET REGS, PEEK/POKE is
the failover plan if GET/SET REGS are not available).
It is however not clear how to re-enable GETREGS/SETREGS without
recreating the compile error on f20/amd64.
Maybe by #include <asm/ptrace.h> instead of linux/ptrace.h ?
Or maybe we just re-include linux/ptrace.h, hoping fedora20
will change/fix the include files to avoid such compilation error ?
Philippe
|
|
From: Philippe W. <phi...@sk...> - 2013-08-03 20:40:46
|
On Sat, 2013-08-03 at 00:44 +0200, Philippe Waroquiers wrote: > It is however not clear how to re-enable GETREGS/SETREGS without > recreating the compile error on f20/amd64. > > Maybe by #include <asm/ptrace.h> instead of linux/ptrace.h ? > > Or maybe we just re-include linux/ptrace.h, hoping fedora20 > will change/fix the include files to avoid such compilation error ? Hello Dejan, Finally, I took another approach: Revision r13482 changes the way PTRACE_GETREGS is detected. Rather than doing that at compilation time, it is done at configure time. This should make that all platforms providing PTRACE_GETREGS in either sys/ptrace.h (most) or sys/user.h (ppc64) should properly detect again that PTRACE_GETREGS can be used. (this should repair the mips build, but still no gcc farm mips access). As a follow up, vgdb.c MIPS specific sections might still be reworked to e.g. use the symbolic constants rather than hardcoded numbers. Philippe |