Re: [Valgrind-developers] valgrind: r13471: do not include linux/ptrace.h in vgdb.c

[Dejan J. <dej...@rt...>]

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