Re: [PATCH 1/3] Include asm/ptrace.h in mips-linux-nat.c

[Pedro Alves <palves at redhat dot com>]

On 06/20/2013 09:42 PM, Maciej W. Rozycki wrote:
> On Thu, 20 Jun 2013, Pedro Alves wrote:

>> For new features, things get a bit more blurry.
>>
>> Given kernel headers guarantee backwards compatibility, it's my
>> understanding that you should always be able to build against the
>> latest kernel headers, even if you're running against an old kernel.
> 
>  Well, the rule isn't exactly that -- the rule is the kernel headers 
> installed in the system for use by user programs must be exactly those the 
> C library (typically glibc) has been built against.  Otherwise all hell is 
> bound to break loose.  

Ah.  Yeah, that was it.

> Of course not everyone is prepared to build their 
> own C library, and we cannot expect one to be.

Certainly.

>  Actually the case is I believe even more complicated.  The rule expressed 
> by Linus as I remember it used to be not to make use of the kernel headers 
> in user programs at all.  Any definitions and declarations required for 
> user programs were expected to be either folded into the relevant C 
> library headers (e.g. glibc frequently chooses <bits/foo.h> for such 
> stuff), or bundled with the piece of user software that relies on the 
> relevant subsystem (that used to be the case with the more obscure stuff, 
> e.g. all the programs relying on the netlink stuff used to do that, and 
> perhaps still do).
> 
>  What we currently have in GDB with regard to MIPS hardware watchpoint 
> functionality carefully follows the second choice.  Now times are changing 
> and I reckon over the years a project has been under way to split the 
> kernel headers into a strictly internal set and an exported user-ABI set, 
> with the latter intended to make user program developers' lives easier.  

Yeah, I'm aware of that UAPI header split project as well.
I'd sooner believe we ended up with the structs in GDB because of the desire
to use the feature in GDB with an updated kernel, but not an updated libc
(Daney did the kernel side bits, afterall), rather than due to special
care to follow the second choice, though.

> I haven't tracked progress there and I don't know offhand how far the 
> project has gone, but unless you have a better knowledge of what the 
> situation is there and can give good counterarguments, we certainly have 
> to be careful especially with such an old Linux port the MIPS one is.

No special knowledge, but I wasn't arguing for not being
careful either.

>> ISTR that's how CS builds things.  However, naturally not everyone
>> will do that.  Most will rely on the system's kernel version's
>> headers.
> 
>  Well, with my upstream reviewer's hat on I try to avoid assuming one's 
> build environment is going to be as sophisticated as one used by those who 
> professionally work on toolchain components.  The minimal and probably 
> still typical recipe of a random developer or even system administrator 
> out there roughly is:
> 
> $ ./configure && make && make install

Certainly.  The details were escaping me, but indeed I was thinking
of glibc building.

> 
> and I think our objective is to make this as smooth as possible, avoiding 
> making people pull their hair.  Professional software packagers will cope 
> either way, we don't need to care that much about them.

*nod*

>> In this case, I was borderline when I initially reviewed the
>> Daney's watchpoints's code, but I accepted it then.  Given they're
>> already in place, it just sounds like causing unnecessary trouble
>> to me to remove them, more so you've shown an example where they're
>> necessary.  I'd vote for keeping them.  Supposedly, MAX_DEBUG_REGISTER
>> or some other convenient define was added to the kernel's asm/ptrace.h
>> at the same time as struct pt_watch_regs etc, so we could
>> still include asm/ptrace.h, and only define the fallbacks if
>> that such macro is not defined?
> 
>  We have the choice between PTRACE_GET_WATCH_REGS and 
> PTRACE_SET_WATCH_REGS only here, there have been no other macros added 
> with the Linux commit 0926bf953ee79b8f139741b442e5a18520f81705 Yao 
> referred to above.

Looks like those are good enough to me.  I notice the code added to GDB does:

#ifndef PTRACE_GET_WATCH_REGS
#  define PTRACE_GET_WATCH_REGS 0xd0
#endif

#ifndef PTRACE_SET_WATCH_REGS
#  define PTRACE_SET_WATCH_REGS 0xd1
#endif

and only does that #ifndef dance in those two particular macros...

But I'd hope that was more an historical accident than the case
of Daney finding a system with conflicting PTRACE_GET/SET_WATCH_REGS
definitions (and no corresponding structs)...

> The change regrettably has the maximum architectural
> number of watchpoint register sets possible to implement in hardware 
> hardcoded throughout as literal 8 rather than using a macro like our 
> MAX_DEBUG_REGISTER (a misnomer actually, as I noted elsewhere).

Yeah, to make that accept anything but 8, we'd have to change
the struct's layouts.  Could have worked for SET without breaking
the ABI if num_valid was the first field in the struct, alas,
it isn't...  For GET, we'd be stuck anyway, as the caller would
have no indication of the size of the struct the kernel would fill
in.  As is, MAX_DEBUG_REGISTER is just an ABI array length constant.

-- 
Pedro Alves