Toolchain/binutils-gdb
1
0
Fork 0
forked from Imagelibrary/binutils-gdb
Code Pull Requests Activity
Files
21c90ca166ee384d271e92a409c690a7faf945a2
binutils-gdb/gdb/aarch64-fbsd-nat.c
Your Name 21c90ca166 gdb/aarch64: restore in-order watchpoint matching 
At Red Hat we have an out of tree AArch64 watchpoint test which broke
after this commit:

  commit cf16ab724a
  Date:   Tue Mar 12 17:08:18 2024 +0100

      [gdb/tdep] Fix gdb.base/watch-bitfields.exp on aarch64

The problem with AArch64 hardware watchpoints is that they (as I
understand it) are restricted to a minimum of 8 bytes.  This means
that, if the thing you are watching is less than 8-bytes, then there
is always scope for invalid watchpoint triggers caused by activity in
the part of the 8-bytes that are not being watched.

Or, as is the case in this RH test, multiple watchpoint are created
within an 8-byte region, and GDB can miss-identify which watchpoint
actually triggered.

Prior to the above commit the RH test was passing.  However, the test
was relying on, in the case of ambiguity, GDB selecting the first
created watchpoint.  That behaviour changed with the above commit.
Now GDB favours reporting non write breakpoints, and will only report
a write breakpoint if no non-write breakpoint exists in the same
region.

I originally posted a patch to try and tweak the existing logic to
restore enough of the original behaviour that the RH test would pass,
this can be found here (2 iterations):

  https://inbox.sourceware.org/gdb-patches/65e746b6394f04faa027e778f733eda95d20f368.1753115072.git.aburgess@redhat.com
  https://inbox.sourceware.org/gdb-patches/638cbe9b738c0c529f6370f90ba4a395711f63ae.1753971315.git.aburgess@redhat.com

Neither of these really resolved the problem, they fixed some cases,
but broke others.

Ultimately, the problem on AArch64 is that for a single watchpoint
trap, there could be multiple watchpoints that are potentially
responsible.  The existing API defined by the target_ops methods
stopped_by_watchpoint() and stopped_data_address() only allow for two
possible options:

  1. If stopped_by_watchpoint() is true then stopped_data_address()
     can return true and a single address which identifies all
     watchpoints at that single address, or

  2. If stopped_by_watchpoint() is true then stopped_data_address()
     can return false, in which case GDB will check all write
     watchpoints to see if any have changed, if they have, then GDB
     tells the user that that was the triggering watchpoint.

If we are in a situation where we have to choose between multiple
write and read watchpoints then the current API doesn't allow the
architecture specific code to tell GDB core about this case.

In this commit I propose that we change the target_ops API,
specifically, the method:

  bool target_ops::stopped_data_address (CORE_ADDR *);

will change to:

  std::vector<CORE_ADDR> target_ops::stopped_data_addresses ();

The architecture specific code can now return a set of watchpoint
addresses, allowing GDB to identify a set of watchpoints that might
have triggered.  GDB core can then select the most likely watchpoint,
and present that to the user.

As with the old API, target_ops::stopped_data_addresses should only be
called when target_ops::stopped_by_watchpoint is true, in which case
it's return values can be interpreted like this:

  a. An empty vector; this replaces the old case where false was
     returned.  GDB should check all the write watchpoints and select
     the one that changed as the responsible watchpoint.

  b. A single entry vector; all targets except AArch64 currently
     return at most a single entry vector.  The single address
     indicates the watchpoint(s) that triggered.

  c. A multi-entry vector; currently AArch64 only.  These addresses
     indicate the set of watchpoints that might have triggered.  GDB
     will check the write watchpoints to see which (if any) changed,
     and if no write watchpoints changed, GDB will present the first
     access watchpoint.

In the future, we might want to improve the handling of (c) so that
GDB tells the user that multiple access watchpoints might have
triggered, and then list all of them.  This might clear up some
confusion.  But I think that can be done in the future (I don't have
an immediate plan to work on this).  I think this change is already a
good improvement.

The changes for this are pretty extensive, but here's a basic summary:

  * Within gdb/ changing the API name from stopped_data_address to
    stopped_data_addresses throughout.  Comments are updated too where
    needed.

  * For targets other than AArch64, the existing code is retained with
    as few changes as possible, we only allow for a single address to
    be returned, the address is now wrapped in a vector.  Where we
    used to return false, we now return the empty vector.

  * For AArch64, the return a vector logic is pushed through to
    gdb/nat/aarch64-hw-point.{c,h}, and aarch64_stopped_data_address
    changes to aarch64_stopped_data_addresses, and is updated to
    return a vector of addresses.

  * In infrun.c there's some updates to some debug output.

  * In breakpoint.c the interesting changes are in
    watchpoints_triggered.  The existing code has three cases to
    handle:

    (i) target_stopped_by_watchpoint returns false.  This case is
        unchanged.

    (ii) target_stopped_data_address returns false.  This case is now
         calling target_stopped_data_addresses, and checks for the
	 empty vector, but otherwise is unchanged.

    (iii) target_stopped_data_address returns true, and a single
          address.  This code calls target_stopped_data_addresses, and
	  now handles the possibility of a vector containing multiple
	  entries.  We need to first loop over every watchpoint
	  setting its triggered status to 'no', then we check every
	  address in the vector setting matching watchpoint's
	  triggered status to 'yes'.  But the actual logic for if a
	  watchpoint matches an address or not is unchanged.

    The important thing to notice here is that in case (iii), before
    this patch, GDB could already set _multiple_ watchpoints to
    triggered.  For example, setting a read and write watchpoint on
    the same address would result in multiple watchpoints being marked
    as triggered.  This patch just extends this so that multiple
    watchpoints, at multiple addresses, can now be marked as
    triggered.

  * In remote.c there is an interesting change.  We need to allow
    gdbserver to pass the multiple addresses back to GDB.  To achieve
    this, I now allow multiple 'watch', 'rwatch', and 'awatch' tokens
    in a 'T' stop reply packet.  This change is largely backward
    compatible.  For old versions of GDB, GDB will just use the last
    such token as the watchpoint stop address.  For new GDBs, all of
    the addresses are collected and returned from the
    target_ops::stopped_data_addresses call.  If a new GDB connects to
    an old gdbserver then it'll only get a single watchpoint address
    in the 'T' packet, but that's no worse than we are now, and will
    not cause a GDB crash, GDB will just end up checking a restricted
    set of watchpoints (which is where we are right now).

  * In gdbserver/ the changes are pretty similar.  The API is renamed
    from ::stopped_data_address to ::stopped_data_addresses, and
    ::low_stopped_data_address to ::low_stopped_data_addresses.

  * For all targets except AArch64, the existing code is retained, we
    just wrap the single address into a vector.

  * For AArch64, we call aarch64_stopped_data_addresses, which returns
    the required vector.

For testing, I've built GDB on GNU/Linux for i386, x86-64, PPC64le,
ARM, and AArch64.  That still leaves a lot of targets possibly
impacted by this change as untested.  Which is a risk.  I certainly
wouldn't want to push this patch until after GDB 17 branches so we
have time to find and fix any regressions that are introduced.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33240
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33252
2025-08-07 14:43:49 +01:00

358 lines
10 KiB
C
Raw Blame History

/* Native-dependent code for FreeBSD/aarch64.
Copyright (C) 2017-2025 Free Software Foundation, Inc.
This file is part of GDB.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "arch-utils.h"
#include "inferior.h"
#include "regcache.h"
#include "target.h"
#include "nat/aarch64-hw-point.h"
#include "elf/common.h"
#include <sys/param.h>
#include <sys/ptrace.h>
#include <machine/armreg.h>
#include <machine/reg.h>
#include "fbsd-nat.h"
#include "aarch64-tdep.h"
#include "aarch64-fbsd-tdep.h"
#include "aarch64-nat.h"
#include "inf-ptrace.h"
#if __FreeBSD_version >= 1400005
#define HAVE_DBREG
#include <unordered_set>
#endif
#ifdef HAVE_DBREG
struct aarch64_fbsd_nat_target final
: public aarch64_nat_target<fbsd_nat_target>
#else
struct aarch64_fbsd_nat_target final : public fbsd_nat_target
#endif
{
void fetch_registers (struct regcache *, int) override;
void store_registers (struct regcache *, int) override;
const struct target_desc *read_description () override;
#ifdef HAVE_DBREG
/* Hardware breakpoints and watchpoints. */
bool stopped_by_watchpoint () override;
std::vector<CORE_ADDR> stopped_data_addresses () override;
bool stopped_by_hw_breakpoint () override;
bool supports_stopped_by_hw_breakpoint () override;
void post_startup_inferior (ptid_t) override;
void post_attach (int pid) override;
void low_new_fork (ptid_t parent, pid_t child) override;
void low_delete_thread (thread_info *) override;
void low_prepare_to_resume (thread_info *) override;
private:
void probe_debug_regs (int pid);
static bool debug_regs_probed;
#endif
};
static aarch64_fbsd_nat_target the_aarch64_fbsd_nat_target;
/* Fetch register REGNUM from the inferior. If REGNUM is -1, do this
for all registers. */
void
aarch64_fbsd_nat_target::fetch_registers (struct regcache *regcache,
int regnum)
{
fetch_register_set<struct reg> (regcache, regnum, PT_GETREGS,
&aarch64_fbsd_gregset);
fetch_register_set<struct fpreg> (regcache, regnum, PT_GETFPREGS,
&aarch64_fbsd_fpregset);
gdbarch *gdbarch = regcache->arch ();
aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
if (tdep->has_tls ())
fetch_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
&aarch64_fbsd_tls_regset, tdep->tls_regnum_base);
}
/* Store register REGNUM back into the inferior. If REGNUM is -1, do
this for all registers. */
void
aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
int regnum)
{
store_register_set<struct reg> (regcache, regnum, PT_GETREGS, PT_SETREGS,
&aarch64_fbsd_gregset);
store_register_set<struct fpreg> (regcache, regnum, PT_GETFPREGS,
PT_SETFPREGS, &aarch64_fbsd_fpregset);
gdbarch *gdbarch = regcache->arch ();
aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
if (tdep->has_tls ())
store_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
&aarch64_fbsd_tls_regset, tdep->tls_regnum_base);
}
/* Implement the target read_description method. */
const struct target_desc *
aarch64_fbsd_nat_target::read_description ()
{
if (inferior_ptid == null_ptid)
return this->beneath ()->read_description ();
aarch64_features features;
features.tls = have_regset (inferior_ptid, NT_ARM_TLS)? 1 : 0;
return aarch64_read_description (features);
}
#ifdef HAVE_DBREG
bool aarch64_fbsd_nat_target::debug_regs_probed;
/* Set of threads which need to update debug registers on next resume. */
static std::unordered_set<lwpid_t> aarch64_debug_pending_threads;
/* Implement the "stopped_data_addresses" target_ops method. */
std::vector<CORE_ADDR>
aarch64_fbsd_nat_target::stopped_data_addresses ()
{
siginfo_t siginfo;
struct aarch64_debug_reg_state *state;
if (!fbsd_nat_get_siginfo (inferior_ptid, &siginfo))
return {};
/* This must be a hardware breakpoint. */
if (siginfo.si_signo != SIGTRAP
|| siginfo.si_code != TRAP_TRACE
|| siginfo.si_trapno != EXCP_WATCHPT_EL0)
return {};
const CORE_ADDR addr_trap = (CORE_ADDR) siginfo.si_addr;
/* Check if the address matches any watched address. */
state = aarch64_get_debug_reg_state (inferior_ptid.pid ());
return aarch64_stopped_data_addresses (state, addr_trap);
}
/* Implement the "stopped_by_watchpoint" target_ops method. */
bool
aarch64_fbsd_nat_target::stopped_by_watchpoint ()
{
return !stopped_data_addresses ().empty ();
}
/* Implement the "stopped_by_hw_breakpoint" target_ops method. */
bool
aarch64_fbsd_nat_target::stopped_by_hw_breakpoint ()
{
siginfo_t siginfo;
struct aarch64_debug_reg_state *state;
if (!fbsd_nat_get_siginfo (inferior_ptid, &siginfo))
return false;
/* This must be a hardware breakpoint. */
if (siginfo.si_signo != SIGTRAP
|| siginfo.si_code != TRAP_TRACE
|| siginfo.si_trapno != EXCP_WATCHPT_EL0)
return false;
return !stopped_by_watchpoint();
}
/* Implement the "supports_stopped_by_hw_breakpoint" target_ops method. */
bool
aarch64_fbsd_nat_target::supports_stopped_by_hw_breakpoint ()
{
return true;
}
/* Fetch the hardware debug register capability information. */
void
aarch64_fbsd_nat_target::probe_debug_regs (int pid)
{
if (!debug_regs_probed)
{
struct dbreg reg;
debug_regs_probed = true;
aarch64_num_bp_regs = 0;
aarch64_num_wp_regs = 0;
if (ptrace(PT_GETDBREGS, pid, (PTRACE_TYPE_ARG3) &reg, 0) == 0)
{
switch (reg.db_debug_ver)
{
case AARCH64_DEBUG_ARCH_V8:
case AARCH64_DEBUG_ARCH_V8_1:
case AARCH64_DEBUG_ARCH_V8_2:
case AARCH64_DEBUG_ARCH_V8_4:
case AARCH64_DEBUG_ARCH_V8_8:
case AARCH64_DEBUG_ARCH_V8_9:
break;
default:
return;
}
aarch64_num_bp_regs = reg.db_nbkpts;
if (aarch64_num_bp_regs > AARCH64_HBP_MAX_NUM)
{
warning (_("Unexpected number of hardware breakpoint registers"
" reported by ptrace, got %d, expected %d."),
aarch64_num_bp_regs, AARCH64_HBP_MAX_NUM);
aarch64_num_bp_regs = AARCH64_HBP_MAX_NUM;
}
aarch64_num_wp_regs = reg.db_nwtpts;
if (aarch64_num_wp_regs > AARCH64_HWP_MAX_NUM)
{
warning (_("Unexpected number of hardware watchpoint registers"
" reported by ptrace, got %d, expected %d."),
aarch64_num_wp_regs, AARCH64_HWP_MAX_NUM);
aarch64_num_wp_regs = AARCH64_HWP_MAX_NUM;
}
}
}
}
/* Implement the virtual inf_ptrace_target::post_startup_inferior method. */
void
aarch64_fbsd_nat_target::post_startup_inferior (ptid_t ptid)
{
aarch64_remove_debug_reg_state (ptid.pid ());
probe_debug_regs (ptid.pid ());
fbsd_nat_target::post_startup_inferior (ptid);
}
/* Implement the "post_attach" target_ops method. */
void
aarch64_fbsd_nat_target::post_attach (int pid)
{
aarch64_remove_debug_reg_state (pid);
probe_debug_regs (pid);
fbsd_nat_target::post_attach (pid);
}
/* Implement the virtual fbsd_nat_target::low_new_fork method. */
void
aarch64_fbsd_nat_target::low_new_fork (ptid_t parent, pid_t child)
{
struct aarch64_debug_reg_state *parent_state, *child_state;
/* If there is no parent state, no watchpoints nor breakpoints have
been set, so there is nothing to do. */
parent_state = aarch64_lookup_debug_reg_state (parent.pid ());
if (parent_state == nullptr)
return;
/* The kernel clears debug registers in the new child process after
fork, but GDB core assumes the child inherits the watchpoints/hw
breakpoints of the parent, and will remove them all from the
forked off process. Copy the debug registers mirrors into the
new process so that all breakpoints and watchpoints can be
removed together. */
child_state = aarch64_get_debug_reg_state (child);
*child_state = *parent_state;
}
/* Mark debug register state "dirty" for all threads belonging to the
current inferior. */
void
aarch64_notify_debug_reg_change (ptid_t ptid,
int is_watchpoint, unsigned int idx)
{
for (thread_info *tp : current_inferior ()->non_exited_threads ())
{
if (tp->ptid.lwp_p ())
aarch64_debug_pending_threads.emplace (tp->ptid.lwp ());
}
}
/* Implement the virtual fbsd_nat_target::low_delete_thread method. */
void
aarch64_fbsd_nat_target::low_delete_thread (thread_info *tp)
{
gdb_assert(tp->ptid.lwp_p ());
aarch64_debug_pending_threads.erase (tp->ptid.lwp ());
}
/* Implement the virtual fbsd_nat_target::low_prepare_to_resume method. */
void
aarch64_fbsd_nat_target::low_prepare_to_resume (thread_info *tp)
{
gdb_assert(tp->ptid.lwp_p ());
if (aarch64_debug_pending_threads.erase (tp->ptid.lwp ()) == 0)
return;
struct aarch64_debug_reg_state *state =
aarch64_lookup_debug_reg_state (tp->ptid.pid ());
gdb_assert(state != nullptr);
struct dbreg reg;
memset (&reg, 0, sizeof(reg));
for (int i = 0; i < aarch64_num_bp_regs; i++)
{
reg.db_breakregs[i].dbr_addr = state->dr_addr_bp[i];
reg.db_breakregs[i].dbr_ctrl = state->dr_ctrl_bp[i];
}
for (int i = 0; i < aarch64_num_wp_regs; i++)
{
reg.db_watchregs[i].dbw_addr = state->dr_addr_wp[i];
reg.db_watchregs[i].dbw_ctrl = state->dr_ctrl_wp[i];
}
if (ptrace(PT_SETDBREGS, tp->ptid.lwp (), (PTRACE_TYPE_ARG3) &reg, 0) != 0)
error (_("Failed to set hardware debug registers"));
}
#else
/* A stub that should never be called. */
void
aarch64_notify_debug_reg_change (ptid_t ptid,
int is_watchpoint, unsigned int idx)
{
gdb_assert (true);
}
#endif
INIT_GDB_FILE (aarch64_fbsd_nat)
{
#ifdef HAVE_DBREG
aarch64_initialize_hw_point ();
#endif
add_inf_child_target (&the_aarch64_fbsd_nat_target);
}
View Git Blame Copy Permalink