[PATCH] gdb: nat/x86-dregs.c: add and use x86_dr_low_can_get_status

[PATCH] gdb: nat/x86-dregs.c: add and use x86_dr_low_can_get_status

[Simon Marchi]

I built and tried gdb on OpenBSD, and it immediately segfaults when
running a program.  I tracked down the problem to x86_dr_low.get_status
being nullptr at this point:

    (lldb) print x86_dr_low.get_status
    (unsigned long (*)()) $0 = 0x0000000000000000
    (lldb) bt
    * thread #1, stop reason = step over
      * frame #0: 0x0000033b64b764aa gdb`x86_dr_stopped_data_address(state=0x0000033d7162a310, addr_p=0x00007f7ffffc5688) at x86-dregs.c:645:12
        frame #1: 0x0000033b64b766de gdb`x86_dr_stopped_by_watchpoint(state=0x0000033d7162a310) at x86-dregs.c:687:10
        frame #2: 0x0000033b64ea5f72 gdb`x86_stopped_by_watchpoint() at x86-nat.c:206:10
        frame #3: 0x0000033b64637fbb gdb`x86_nat_target<obsd_nat_target>::stopped_by_watchpoint(this=0x0000033b65252820) at x86-nat.h:100:12
        frame #4: 0x0000033b64d3ff11 gdb`target_stopped_by_watchpoint() at target.c:468:46
        frame #5: 0x0000033b6469b001 gdb`watchpoints_triggered(ws=0x00007f7ffffc61c8) at breakpoint.c:4790:32
        frame #6: 0x0000033b64a8bb8b gdb`handle_signal_stop(ecs=0x00007f7ffffc61a0) at infrun.c:6072:29
        frame #7: 0x0000033b64a7e3a7 gdb`handle_inferior_event(ecs=0x00007f7ffffc61a0) at infrun.c:5694:7
        frame #8: 0x0000033b64a7c1a0 gdb`fetch_inferior_event() at infrun.c:4090:5
        frame #9: 0x0000033b64a51921 gdb`inferior_event_handler(event_type=INF_REG_EVENT) at inf-loop.c:41:7
        frame #10: 0x0000033b64a827c9 gdb`infrun_async_inferior_event_handler(data=0x0000000000000000) at infrun.c:9384:3
        frame #11: 0x0000033b6465bd4f gdb`check_async_event_handlers() at async-event.c:335:4
        frame #12: 0x0000033b65070917 gdb`gdb_do_one_event() at event-loop.cc:216:10
        frame #13: 0x0000033b64af0db1 gdb`start_event_loop() at main.c:421:13
        frame #14: 0x0000033b64aefe9a gdb`captured_command_loop() at main.c:481:3
        frame #15: 0x0000033b64aed5c2 gdb`captured_main(data=0x00007f7ffffc6470) at main.c:1353:4
        frame #16: 0x0000033b64aed4f2 gdb`gdb_main(args=0x00007f7ffffc6470) at main.c:1368:7
        frame #17: 0x0000033b6459d787 gdb`main(argc=5, argv=0x00007f7ffffc6518) at gdb.c:32:10
        frame #18: 0x0000033b6459d521 gdb`___start + 321

On BSDs, get_status is set in _initialize_x86_bsd_nat, but only if
HAVE_PT_GETDBREGS is defined.  PT_GETDBREGS doesn't exist on OpenBSD, so
get_status (and the other fields of x86_dr_low) is left nullptr.

I don't know if there is a way to get the DR6 / status register on
OpenBSD, but even if there is, GDB doesn't leverage it as of today.
Since there's no way of getting the status register, it's ok that
x86_dr_low.get_status is nullptr, but code using it should be aware that
it can be nullptr.

Add a x86_dr_low_can_get_status function and use it it functions that
call x86_dr_low_get_status.

With this, running a program gets a bit further, but we hit this:

    (gdb) r
    Starting program: /home/simark/build/binutils-gdb/gdb/a.out
    Child process unexpectedly missing: No child processes.
    /home/simark/src/binutils-gdb/gdb/inferior.c:303: internal-error: struct inferior *find_inferior_pid(process_stratum_target *, int): Assertion `pid != 0' failed

That will be the topic for another patch.

Change-Id: Ibbf6ec02469c80c1a7ab34e71d503779c9ae840c
---
 gdb/nat/x86-dregs.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index cf8e517eb0d6..699918674547 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -82,6 +82,14 @@ x86_dr_low_get_control ()
   return x86_dr_low.get_control ();
 }
 
+/* Can we read the status (DR6) register?  */
+
+static bool
+x86_dr_low_can_get_status ()
+{
+  return x86_dr_low.get_status != nullptr;
+}
+
 /* Return the value of the inferior's DR6 debug status register.  */
 
 static unsigned long
@@ -604,6 +612,9 @@ int
 x86_dr_stopped_data_address (struct x86_debug_reg_state *state,
 			     CORE_ADDR *addr_p)
 {
+  if (!x86_dr_low_can_get_status ())
+    return 0;
+
   CORE_ADDR addr = 0;
   int i;
   int rc = 0;
@@ -693,6 +704,9 @@ x86_dr_stopped_by_watchpoint (struct x86_debug_reg_state *state)
 int
 x86_dr_stopped_by_hw_breakpoint (struct x86_debug_reg_state *state)
 {
+  if (!x86_dr_low_can_get_status ())
+    return 0;
+
   CORE_ADDR addr = 0;
   int i;
   int rc = 0;
-- 
2.32.0

Re: [PATCH] gdb: nat/x86-dregs.c: add and use x86_dr_low_can_get_status

[John Baldwin]

On 7/15/21 12:53 PM, Simon Marchi via Gdb-patches wrote:
> I built and tried gdb on OpenBSD, and it immediately segfaults when
> running a program.  I tracked down the problem to x86_dr_low.get_status
> being nullptr at this point:

I've looked, and another way to possibly fix this is to make the x86_bsd_nat_target
class not inherit from x86_nat_target<> when x86 debug registers aren't supported.
Something like this in x86-bsd-nat.h:

/* A prototype *BSD/x86 target.  */

#ifdef HAVE_PT_GETDBREGS
template<typename BaseTarget>
class x86bsd_nat_target : public x86_nat_target<BaseTarget>
{
   using base_class = x86_nat_target<BaseTarget>
public:
   void mourn_inferior () override
   {
     x86_cleanup_dregs ();
     base_class::mourn_inferior ();
   }
};
#else /* !HAVE_PT_GETDBREGS */
template<typename BaseTarget>
class x86bsd_nat_target : public BaseTarget
{
};
#endif /* HAVE_PT_GETDBREGS */

I would be further tempted to then remove x86-bsd-nat.o from OpenBSD's configure.nat
lines entirely sine it shouldn't be needed at that point, and maybe adjust x86-bsd-nat.c
to start with something like:

#ifndef HAVE_PT_GETDBREGS
#error "x86-bsd-nat.c requires PT_GETDBREGS"
#endif

My other series that divorces FreeBSD from using i386-bsd-nat.c and
amd64-bsd-nat.c almost makes it so that we could further clean those
files up perhaps so that NetBSD explicitly used x86bsd_nat_target as the
Base class passed to the templates defined in i386-bsd-nat.c and
amd64-bsd-nat.c so that we wouldn't even need an empty version of
x86bsd_nat_target for the OpenBSD case, but I think we should try to
fix the immediate issue on OpenBSD first and maybe do that cleanup
later.

I wouldn't be opposed to this fix also going in, but I kind of think it's
cleaner overall for the OpenBSD target to not use x86-nat.c at all if
it doesn't support the x86 debug registers.  I can take a stab at a
patch to do what I described above if that is easier (I don't currently
have an OpenBSD VM around, but it shouldn't be too hard for me to spin
one up to test this)

-- 
John Baldwin

Re: [PATCH] gdb: nat/x86-dregs.c: add and use x86_dr_low_can_get_status

[John Baldwin]

On 7/16/21 1:38 PM, John Baldwin wrote:
> On 7/15/21 12:53 PM, Simon Marchi via Gdb-patches wrote:
>> I built and tried gdb on OpenBSD, and it immediately segfaults when
>> running a program.  I tracked down the problem to x86_dr_low.get_status
>> being nullptr at this point:
> 
> I've looked, and another way to possibly fix this is to make the x86_bsd_nat_target
> class not inherit from x86_nat_target<> when x86 debug registers aren't supported.
> Something like this in x86-bsd-nat.h:
> 
> /* A prototype *BSD/x86 target.  */
> 
> #ifdef HAVE_PT_GETDBREGS
> template<typename BaseTarget>
> class x86bsd_nat_target : public x86_nat_target<BaseTarget>
> {
>     using base_class = x86_nat_target<BaseTarget>
> public:
>     void mourn_inferior () override
>     {
>       x86_cleanup_dregs ();
>       base_class::mourn_inferior ();
>     }
> };
> #else /* !HAVE_PT_GETDBREGS */
> template<typename BaseTarget>
> class x86bsd_nat_target : public BaseTarget
> {
> };
> #endif /* HAVE_PT_GETDBREGS */

I think this change is ok (but have only tested it on FreeBSD so far).

> I would be further tempted to then remove x86-bsd-nat.o from OpenBSD's configure.nat
> lines entirely sine it shouldn't be needed at that point, and maybe adjust x86-bsd-nat.c
> to start with something like:
> 
> #ifndef HAVE_PT_GETDBREGS
> #error "x86-bsd-nat.c requires PT_GETDBREGS"
> #endif

These changes are probably premature (though might be realistic after my
other series lands that pulls x86bsd_xsave_len into the FreeBSD-specific
targets such that x86-bsd-nat.c is purely about debug registers).  It
should still be possible to remove x86-nat.c from OpenBSD in configure.nat
with the x86-bsd-nat.h change.  I can try to test it once I get an
OpenBSD VM spun up.

-- 
John Baldwin

Re: [PATCH] gdb: nat/x86-dregs.c: add and use x86_dr_low_can_get_status

[Simon Marchi]

On 2021-07-18 6:39 p.m., John Baldwin wrote:
> On 7/16/21 1:38 PM, John Baldwin wrote:
>> On 7/15/21 12:53 PM, Simon Marchi via Gdb-patches wrote:
>>> I built and tried gdb on OpenBSD, and it immediately segfaults when
>>> running a program.  I tracked down the problem to x86_dr_low.get_status
>>> being nullptr at this point:
>>
>> I've looked, and another way to possibly fix this is to make the x86_bsd_nat_target
>> class not inherit from x86_nat_target<> when x86 debug registers aren't supported.
>> Something like this in x86-bsd-nat.h:
>>
>> /* A prototype *BSD/x86 target.  */
>>
>> #ifdef HAVE_PT_GETDBREGS
>> template<typename BaseTarget>
>> class x86bsd_nat_target : public x86_nat_target<BaseTarget>
>> {
>>     using base_class = x86_nat_target<BaseTarget>
>> public:
>>     void mourn_inferior () override
>>     {
>>       x86_cleanup_dregs ();
>>       base_class::mourn_inferior ();
>>     }
>> };
>> #else /* !HAVE_PT_GETDBREGS */
>> template<typename BaseTarget>
>> class x86bsd_nat_target : public BaseTarget
>> {
>> };
>> #endif /* HAVE_PT_GETDBREGS */
> 
> I think this change is ok (but have only tested it on FreeBSD so far).
> 
>> I would be further tempted to then remove x86-bsd-nat.o from OpenBSD's configure.nat
>> lines entirely sine it shouldn't be needed at that point, and maybe adjust x86-bsd-nat.c
>> to start with something like:
>>
>> #ifndef HAVE_PT_GETDBREGS
>> #error "x86-bsd-nat.c requires PT_GETDBREGS"
>> #endif
> 
> These changes are probably premature (though might be realistic after my
> other series lands that pulls x86bsd_xsave_len into the FreeBSD-specific
> targets such that x86-bsd-nat.c is purely about debug registers).  It
> should still be possible to remove x86-nat.c from OpenBSD in configure.nat
> with the x86-bsd-nat.h change.  I can try to test it once I get an
> OpenBSD VM spun up.

All you said sounds good to me.  If you want to do it while you do the
changes to FreeBSD, please go ahead, because this is pretty far down on
my todo list (I just happened to give a try on OpenBSD and wanted to
minimally unbreak it).

Simon

Re: [PATCH] gdb: nat/x86-dregs.c: add and use x86_dr_low_can_get_status

[John Baldwin]

On 7/19/21 7:44 AM, Simon Marchi wrote:
> On 2021-07-18 6:39 p.m., John Baldwin wrote:
>> On 7/16/21 1:38 PM, John Baldwin wrote:
>>> On 7/15/21 12:53 PM, Simon Marchi via Gdb-patches wrote:
>>>> I built and tried gdb on OpenBSD, and it immediately segfaults when
>>>> running a program.  I tracked down the problem to x86_dr_low.get_status
>>>> being nullptr at this point:
>>>
>>> I've looked, and another way to possibly fix this is to make the x86_bsd_nat_target
>>> class not inherit from x86_nat_target<> when x86 debug registers aren't supported.
>>> Something like this in x86-bsd-nat.h:
>>>
>>> /* A prototype *BSD/x86 target.  */
>>>
>>> #ifdef HAVE_PT_GETDBREGS
>>> template<typename BaseTarget>
>>> class x86bsd_nat_target : public x86_nat_target<BaseTarget>
>>> {
>>>      using base_class = x86_nat_target<BaseTarget>
>>> public:
>>>      void mourn_inferior () override
>>>      {
>>>        x86_cleanup_dregs ();
>>>        base_class::mourn_inferior ();
>>>      }
>>> };
>>> #else /* !HAVE_PT_GETDBREGS */
>>> template<typename BaseTarget>
>>> class x86bsd_nat_target : public BaseTarget
>>> {
>>> };
>>> #endif /* HAVE_PT_GETDBREGS */
>>
>> I think this change is ok (but have only tested it on FreeBSD so far).
>>
>>> I would be further tempted to then remove x86-bsd-nat.o from OpenBSD's configure.nat
>>> lines entirely sine it shouldn't be needed at that point, and maybe adjust x86-bsd-nat.c
>>> to start with something like:
>>>
>>> #ifndef HAVE_PT_GETDBREGS
>>> #error "x86-bsd-nat.c requires PT_GETDBREGS"
>>> #endif
>>
>> These changes are probably premature (though might be realistic after my
>> other series lands that pulls x86bsd_xsave_len into the FreeBSD-specific
>> targets such that x86-bsd-nat.c is purely about debug registers).  It
>> should still be possible to remove x86-nat.c from OpenBSD in configure.nat
>> with the x86-bsd-nat.h change.  I can try to test it once I get an
>> OpenBSD VM spun up.
> 
> All you said sounds good to me.  If you want to do it while you do the
> changes to FreeBSD, please go ahead, because this is pretty far down on
> my todo list (I just happened to give a try on OpenBSD and wanted to
> minimally unbreak it).

I've sent a little series of 3 patches that fixes this as well as the
assertion failure with find_inferior_pid.  I suspect the fork handling
does need detach_on_fork (which in theory is just a simple matter of
using PT_DETACH like the FreeBSD code, but I didn't try to test it).

Given the fact that simple programs don't work currently (and the lack
of an active maintainer), I think you probably don't need to try too
hard to test your follow fork changes on OpenBSD.  I'm happy to try to
help if you run into more hiccups.

-- 
John Baldwin