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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Thu, 30 May 2013, Yao Qi wrote:

> This patch is to include asm/ptrace.h in mips-linux-nat.c and remove
> some duplicated structs already defined in asm/ptrace.h.

 You missed the local definitions of the PTRACE_GET_WATCH_REGS and 
PTRACE_SET_WATCH_REGS macros that could also be removed, however...

> The ptrace support for hardware support was added into kernel in Sep.
> 2008 and the mips hardware watchpoint for native GDB was committed
> on April 2009.  Considering the time order, GDB doesn't have to carry
> these local definitions, and should include asm/ptrace.h instead.

 ... this change actually resulted in a graceless build failure for me:

../../src/gdb/mips-linux-nat.c:599: error: storage size of 'dummy_regs' isn't known

etc. for the very reason the system I used for testing has old kernel 
headers installed (2.6.19 it would seem, according to <linux/version.h>).  

 Such a failure is not acceptable from the user's point of view; I think 
there are three ways to deal with this:

1. Add an autoconf test that checks for the presence of a key 
   <asm/ptrace.h> definition; I think 'struct pt_watch_regs' is a good 
   candidate.  If that test does not succeed, then the configure process 
   fails gracefully stating the minimum released version of kernel headers 
   required.

2. Add the same test, except in the failure case fall back to the internal 
   definitions we already have, wrapped into #ifndef 
   HAVE_STRUCT_PT_WATCH_REGS.

3. Add the same test and disable hardware watchpoint support in the 
   failure case.

 The user ABI of the Linux kernel has a stability guarantee and your 
change makes GDB stop building on this otherwise fine system, so I'd lean 
towards the second choice, moving all the stuff into 
common/mips-linux-watch.h as per your second patch in this series.  I am 
not entirely sure what the general practice about such changes in GDB has 
been though, so I'd ask other maintainers for opinion here.

 Note that likewise the local PTRACE_GET_THREAD_AREA definition could be 
removed if relying on <asm/ptrace.h> for watchpoint support as the macro 
has already been defined in the 2.6.19 version I have on this system that 
does not have watchpoint definitions yet.  For this reason I think it will 
be better actually if common/mips-linux-watch.h relies on <asm/ptrace.h> 
having been included earlier on by the source file including the former 
header.

 For the record the system I intended to run native testing on does indeed 
support hardware watchpoints, although a minimal number of them:

hardware watchpoint	: yes, count: 2, address/irw mask: [0x0ffc, 0x0ffb]

-- i.e. a single data watchpoint and a single execution watchpoint only 
(as noted by the "irw mask" field, taking the 3 LSBs of the values 
reported).  I expect to have results by the end of tomorrow and will let 
you know how that has gone.

 Would general maintainers please comment on the three-way choice and the 
preferred way to move forward I outlined above?

  Maciej