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

Maciej W. Rozycki
2013-06-13T17:37:38Z

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