[PATCH v6 1/2] gdb: Make tagged pointer support configurable.
Luis Machado
luis.machado@arm.com
Tue Nov 5 14:51:24 GMT 2024
On 11/5/24 14:40, Schimpe, Christina wrote:
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: Tuesday, November 5, 2024 3:22 PM
>> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
>> patches@sourceware.org
>> Cc: Tiezhu Yang <yangtiezhu@loongson.cn>
>> Subject: Re: [PATCH v6 1/2] gdb: Make tagged pointer support configurable.
>>
>> On 11/5/24 14:07, Schimpe, Christina wrote:
>>> Hi Luis,
>>>
>>> Thanks a lot for the review. I have one questions to your comment, please see
>> below.
>>>
>>>> -----Original Message-----
>>>> From: Luis Machado <luis.machado@arm.com>
>>>> Sent: Monday, November 4, 2024 12:18 PM
>>>> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
>>>> patches@sourceware.org
>>>> Subject: Re: [PATCH v6 1/2] gdb: Make tagged pointer support configurable.
>>>>
>>>> Hi Christina,
>>>>
>>>> On 10/28/24 08:46, Schimpe, Christina wrote:
>>>>> From: Christina Schimpe <christina.schimpe@intel.com>
>>>>>
>>>>> The gdbarch function gdbarch_remove_non_address_bits adjusts
>>>>> addresses to enable debugging of programs with tagged pointers on
>>>>> Linux, for instance for ARM's feature top byte ignore (TBI).
>>>>> Once the function is implemented for an architecture, it adjusts
>>>>> addresses for memory access, breakpoints and watchpoints.
>>>>>
>>>>> Linear address masking (LAM) is Intel's (R) implementation of tagged
>>>>> pointer support. It requires certain adaptions to GDB's tagged
>>>>> pointer support due to the following:
>>>>> - LAM supports address tagging for data accesses only. Thus, specifying
>>>>> breakpoints on tagged addresses is not a valid use case.
>>>>> - In contrast to the implementation for ARM's TBI, the Linux kernel supports
>>>>> tagged pointers for memory access.
>>>>>
>>>>> This patch makes GDB's tagged pointer support configurable such that
>>>>> it is possible to enable the address adjustment for a specific
>>>>> feature only (e.g memory access, breakpoints or watchpoints). This
>>>>> way, one can make sure that addresses are only adjusted when
>>>>> necessary. In case of LAM, this avoids unnecessary parsing of the
>>>>> /proc/<pid>/status file to get the untag mask.
>>>>>
>>>>> Reviewed-By: Felix Willgerodt <felix.willgerodt@intel.com>
>>>>> ---
>>>>> gdb/aarch64-linux-nat.c | 2 +-
>>>>> gdb/aarch64-linux-tdep.c | 7 +++--
>>>>> gdb/aarch64-tdep.c | 23 ++++++++------
>>>>> gdb/aarch64-tdep.h | 6 ++++
>>>>> gdb/breakpoint.c | 5 +--
>>>>> gdb/gdbarch-gen.c | 66 ++++++++++++++++++++++++++++++++-------
>>>>> gdb/gdbarch-gen.h | 49 ++++++++++++++++++++++-------
>>>>> gdb/gdbarch_components.py | 53 ++++++++++++++++++++++++++-----
>>>>> gdb/loongarch-linux-nat.c | 2 +-
>>>>> gdb/target.c | 3 +-
>>>>> 10 files changed, 169 insertions(+), 47 deletions(-)
>>>>>
>>>>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c index
>>>>> 0fa5bee500b..48ab765d880 100644
>>>>> --- a/gdb/aarch64-linux-nat.c
>>>>> +++ b/gdb/aarch64-linux-nat.c
>>>>> @@ -943,7 +943,7 @@ aarch64_linux_nat_target::stopped_data_address
>>>> (CORE_ADDR *addr_p)
>>>>> kernel can potentially be tagged addresses. */
>>>>> struct gdbarch *gdbarch = thread_architecture (inferior_ptid);
>>>>> const CORE_ADDR addr_trap
>>>>> - = gdbarch_remove_non_address_bits (gdbarch, (CORE_ADDR)
>>>> siginfo.si_addr);
>>>>> + = aarch64_remove_non_address_bits (gdbarch, (CORE_ADDR)
>>>>> + siginfo.si_addr);
>>>>>
>>>>> /* Check if the address matches any watched address. */
>>>>> state = aarch64_get_debug_reg_state (inferior_ptid.pid ()); diff
>>>>> --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c index
>>>>> c608a84bc71..61c3be8b6f0 100644
>>>>> --- a/gdb/aarch64-linux-tdep.c
>>>>> +++ b/gdb/aarch64-linux-tdep.c
>>>>> @@ -2433,7 +2433,7 @@ static bool
>>>>> aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR
>>>>> address) {
>>>>> /* Remove the top byte for the memory range check. */
>>>>> - address = gdbarch_remove_non_address_bits (gdbarch, address);
>>>>> + address = aarch64_remove_non_address_bits (gdbarch, address);
>>>>>
>>>>> /* Check if the page that contains ADDRESS is mapped with PROT_MTE. */
>>>>> if (!linux_address_in_memtag_page (address)) @@ -2491,8 +2491,9
>>>>> @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
>>>>> uiout->text ("\n");
>>>>>
>>>>> std::optional<CORE_ADDR> atag
>>>>> - = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch,
>>>>> - fault_addr));
>>>>> + = aarch64_mte_get_atag (
>>>>> + aarch64_remove_non_address_bits (gdbarch, fault_addr));
>>>>> +
>>>>> gdb_byte ltag = aarch64_mte_get_ltag (fault_addr);
>>>>>
>>>>> if (!atag.has_value ())
>>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index
>>>>> 8a2a9b1e23c..91fec7879ed 100644
>>>>> --- a/gdb/aarch64-tdep.c
>>>>> +++ b/gdb/aarch64-tdep.c
>>>>> @@ -4121,7 +4121,7 @@ aarch64_memtag_matches_p (struct gdbarch
>>>>> *gdbarch,
>>>>>
>>>>> /* Fetch the allocation tag for ADDRESS. */
>>>>> std::optional<CORE_ADDR> atag
>>>>> - = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch,
>>>> addr));
>>>>> + = aarch64_mte_get_atag (aarch64_remove_non_address_bits
>>>>> + (gdbarch, addr));
>>>>>
>>>>> if (!atag.has_value ())
>>>>> return true;
>>>>> @@ -4160,7 +4160,7 @@ aarch64_set_memtags (struct gdbarch *gdbarch,
>>>> struct value *address,
>>>>> else
>>>>> {
>>>>> /* Remove the top byte. */
>>>>> - addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>>> + addr = aarch64_remove_non_address_bits (gdbarch, addr);
>>>>>
>>>>> /* With G being the number of tag granules and N the number of tags
>>>>> passed in, we can have the following cases:
>>>>> @@ -4209,7 +4209,7 @@ aarch64_get_memtag (struct gdbarch *gdbarch,
>>>> struct value *address,
>>>>> else
>>>>> {
>>>>> /* Remove the top byte. */
>>>>> - addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>>> + addr = aarch64_remove_non_address_bits (gdbarch, addr);
>>>>> std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>>>>
>>>>> if (!atag.has_value ())
>>>>> @@ -4236,10 +4236,9 @@ aarch64_memtag_to_string (struct gdbarch
>>>> *gdbarch, struct value *tag_value)
>>>>> return string_printf ("0x%s", phex_nz (tag, sizeof (tag))); }
>>>>>
>>>>> -/* AArch64 implementation of the remove_non_address_bits gdbarch hook.
>>>> Remove
>>>>> - non address bits from a pointer value. */
>>>>> +/* See aarch64-tdep.h. */
>>>>>
>>>>> -static CORE_ADDR
>>>>> +CORE_ADDR
>>>>> aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR
>>>>> pointer) {
>>>>> /* By default, we assume TBI and discard the top 8 bits plus the
>>>>> VA range @@ -4750,9 +4749,15 @@ aarch64_gdbarch_init (struct
>>>>> gdbarch_info
>>>> info, struct gdbarch_list *arches)
>>>>> tdep->ra_sign_state_regnum = ra_sign_state_offset + num_regs;
>>>>>
>>>>> /* Architecture hook to remove bits of a pointer that are not part of the
>>>>> - address, like memory tags (MTE) and pointer authentication signatures.
>> */
>>>>> - set_gdbarch_remove_non_address_bits (gdbarch,
>>>>> - aarch64_remove_non_address_bits);
>>>>> + address, like memory tags (MTE) and pointer authentication signatures.
>>>>> + Configure address adjustment for watch-, breakpoints and
>>>>> + memory
>>>>
>>>> s/watch-/watchpoints
>>>
>>> Fill fix, thanks.
>>>
>>>>> + transfer. */
>>>>> + set_gdbarch_remove_non_address_bits_watchpoint
>>>>> + (gdbarch, aarch64_remove_non_address_bits);
>>>>> + set_gdbarch_remove_non_address_bits_breakpoint
>>>>> + (gdbarch, aarch64_remove_non_address_bits);
>>>>> + set_gdbarch_remove_non_address_bits_memory
>>>>> + (gdbarch, aarch64_remove_non_address_bits);
>>>>>
>>>>> /* SME pseudo-registers. */
>>>>> if (tdep->has_sme ())
>>>>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h index
>>>>> 50166fb4f24..13ea37de7a3 100644
>>>>> --- a/gdb/aarch64-tdep.h
>>>>> +++ b/gdb/aarch64-tdep.h
>>>>> @@ -205,4 +205,10 @@ bool aarch64_displaced_step_hw_singlestep
>>>>> (struct gdbarch *gdbarch);
>>>>>
>>>>> std::optional<CORE_ADDR> aarch64_mte_get_atag (CORE_ADDR address);
>>>>>
>>>>> +/* AArch64 implementation of the remove_non_address_bits gdbarch
>> hooks.
>>>>> + Remove non address bits from a pointer value. */
>>>>> +
>>>>> +CORE_ADDR aarch64_remove_non_address_bits (struct gdbarch *gdbarch,
>>>>> + CORE_ADDR pointer);
>>>>> +
>>>>> #endif /* aarch64-tdep.h */
>>>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index
>>>>> b7e4f5d0a45..9983e905e1b 100644
>>>>> --- a/gdb/breakpoint.c
>>>>> +++ b/gdb/breakpoint.c
>>>>> @@ -2313,7 +2313,8 @@ update_watchpoint (struct watchpoint *b, bool
>>>> reparse)
>>>>> loc->gdbarch = v->type ()->arch ();
>>>>> loc->pspace = wp_pspace;
>>>>> loc->address
>>>>> - = gdbarch_remove_non_address_bits (loc->gdbarch, addr);
>>>>> + = gdbarch_remove_non_address_bits_watchpoint (loc-
>>>>> gdbarch,
>>>>> + addr);
>>>>> b->add_location (*loc);
>>>>>
>>>>> if (bitsize != 0)
>>>>> @@ -7538,7 +7539,7 @@ adjust_breakpoint_address (struct gdbarch
>>>> *gdbarch,
>>>>> }
>>>>>
>>>>> adjusted_bpaddr
>>>>> - = gdbarch_remove_non_address_bits (gdbarch, adjusted_bpaddr);
>>>>> + = gdbarch_remove_non_address_bits_breakpoint (gdbarch,
>>>>> +adjusted_bpaddr);
>>>>>
>>>>> /* An adjusted breakpoint address can significantly alter
>>>>> a user's expectations. Print a warning if an adjustment diff
>>>>> --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c index
>>>>> 0d00cd7c993..d05c7a3cbdf
>>>>> 100644
>>>>> --- a/gdb/gdbarch-gen.c
>>>>> +++ b/gdb/gdbarch-gen.c
>>>>> @@ -143,7 +143,9 @@ struct gdbarch
>>>>> int frame_red_zone_size = 0;
>>>>> gdbarch_convert_from_func_ptr_addr_ftype
>>>>> *convert_from_func_ptr_addr =
>>>> convert_from_func_ptr_addr_identity;
>>>>> gdbarch_addr_bits_remove_ftype *addr_bits_remove =
>>>>> core_addr_identity;
>>>>> - gdbarch_remove_non_address_bits_ftype *remove_non_address_bits =
>>>>> default_remove_non_address_bits;
>>>>> + gdbarch_remove_non_address_bits_watchpoint_ftype
>>>>> + *remove_non_address_bits_watchpoint =
>>>>> + default_remove_non_address_bits;
>>>>> + gdbarch_remove_non_address_bits_breakpoint_ftype
>>>>> + *remove_non_address_bits_breakpoint =
>>>>> + default_remove_non_address_bits;
>>>>> + gdbarch_remove_non_address_bits_memory_ftype
>>>>> + *remove_non_address_bits_memory = default_remove_non_address_bits;
>>>>> gdbarch_memtag_to_string_ftype *memtag_to_string =
>>>> default_memtag_to_string;
>>>>> gdbarch_tagged_address_p_ftype *tagged_address_p =
>>>> default_tagged_address_p;
>>>>> gdbarch_memtag_matches_p_ftype *memtag_matches_p =
>>>>> default_memtag_matches_p; @@ -407,7 +409,9 @@ verify_gdbarch (struct
>>>> gdbarch *gdbarch)
>>>>> /* Skip verify of frame_red_zone_size, invalid_p == 0. */
>>>>> /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0. */
>>>>> /* Skip verify of addr_bits_remove, invalid_p == 0. */
>>>>> - /* Skip verify of remove_non_address_bits, invalid_p == 0. */
>>>>> + /* Skip verify of remove_non_address_bits_watchpoint, invalid_p
>>>>> + == 0. */
>>>>> + /* Skip verify of remove_non_address_bits_breakpoint, invalid_p
>>>>> + == 0. */
>>>>> + /* Skip verify of remove_non_address_bits_memory, invalid_p == 0.
>>>>> + */
>>>>> /* Skip verify of memtag_to_string, invalid_p == 0. */
>>>>> /* Skip verify of tagged_address_p, invalid_p == 0. */
>>>>> /* Skip verify of memtag_matches_p, invalid_p == 0. */ @@ -910,8
>>>>> +914,14 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file
>>>>> +*file)
>>>>> "gdbarch_dump: addr_bits_remove = <%s>\n",
>>>>> host_address_to_string (gdbarch->addr_bits_remove));
>>>>> gdb_printf (file,
>>>>> - "gdbarch_dump: remove_non_address_bits = <%s>\n",
>>>>> - host_address_to_string (gdbarch->remove_non_address_bits));
>>>>> + "gdbarch_dump: remove_non_address_bits_watchpoint = <%s>\n",
>>>>> + host_address_to_string
>>>>> +(gdbarch->remove_non_address_bits_watchpoint));
>>>>> + gdb_printf (file,
>>>>> + "gdbarch_dump: remove_non_address_bits_breakpoint = <%s>\n",
>>>>> + host_address_to_string
>>>>> +(gdbarch->remove_non_address_bits_breakpoint));
>>>>> + gdb_printf (file,
>>>>> + "gdbarch_dump: remove_non_address_bits_memory = <%s>\n",
>>>>> + host_address_to_string
>>>>> +(gdbarch->remove_non_address_bits_memory));
>>>>> gdb_printf (file,
>>>>> "gdbarch_dump: memtag_to_string = <%s>\n",
>>>>> host_address_to_string (gdbarch->memtag_to_string)); @@
>>>>> -3198,20 +3208,54 @@ set_gdbarch_addr_bits_remove (struct gdbarch
>>>>> *gdbarch, }
>>>>>
>>>>> CORE_ADDR
>>>>> -gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR
>>>>> pointer)
>>>>> +gdbarch_remove_non_address_bits_watchpoint (struct gdbarch
>>>>> +*gdbarch, CORE_ADDR pointer) {
>>>>> + gdb_assert (gdbarch != NULL);
>>>>> + gdb_assert (gdbarch->remove_non_address_bits_watchpoint != NULL);
>>>>> + if (gdbarch_debug >= 2)
>>>>> + gdb_printf (gdb_stdlog,
>>>>> +"gdbarch_remove_non_address_bits_watchpoint called\n");
>>>>> + return gdbarch->remove_non_address_bits_watchpoint (gdbarch,
>>>>> +pointer); }
>>>>> +
>>>>> +void
>>>>> +set_gdbarch_remove_non_address_bits_watchpoint (struct gdbarch
>>>> *gdbarch,
>>>>> +
>>>> gdbarch_remove_non_address_bits_watchpoint_ftype
>>>>> +remove_non_address_bits_watchpoint)
>>>>> +{
>>>>> + gdbarch->remove_non_address_bits_watchpoint =
>>>>> +remove_non_address_bits_watchpoint;
>>>>> +}
>>>>> +
>>>>> +CORE_ADDR
>>>>> +gdbarch_remove_non_address_bits_breakpoint (struct gdbarch
>>>>> +*gdbarch, CORE_ADDR pointer) {
>>>>> + gdb_assert (gdbarch != NULL);
>>>>> + gdb_assert (gdbarch->remove_non_address_bits_breakpoint != NULL);
>>>>> + if (gdbarch_debug >= 2)
>>>>> + gdb_printf (gdb_stdlog,
>>>>> +"gdbarch_remove_non_address_bits_breakpoint called\n");
>>>>> + return gdbarch->remove_non_address_bits_breakpoint (gdbarch,
>>>>> +pointer); }
>>>>> +
>>>>> +void
>>>>> +set_gdbarch_remove_non_address_bits_breakpoint (struct gdbarch
>>>>> +*gdbarch,
>>>>> +
>>>> gdbarch_remove_non_address_bits_breakpoint_ftype
>>>>> +remove_non_address_bits_breakpoint)
>>>>> +{
>>>>> + gdbarch->remove_non_address_bits_breakpoint =
>>>>> +remove_non_address_bits_breakpoint;
>>>>> +}
>>>>> +
>>>>> +CORE_ADDR
>>>>> +gdbarch_remove_non_address_bits_memory (struct gdbarch *gdbarch,
>>>>> +CORE_ADDR pointer)
>>>>> {
>>>>> gdb_assert (gdbarch != NULL);
>>>>> - gdb_assert (gdbarch->remove_non_address_bits != NULL);
>>>>> + gdb_assert (gdbarch->remove_non_address_bits_memory != NULL);
>>>>> if (gdbarch_debug >= 2)
>>>>> - gdb_printf (gdb_stdlog, "gdbarch_remove_non_address_bits called\n");
>>>>> - return gdbarch->remove_non_address_bits (gdbarch, pointer);
>>>>> + gdb_printf (gdb_stdlog, "gdbarch_remove_non_address_bits_memory
>>>>> + called\n"); return gdbarch->remove_non_address_bits_memory
>>>>> + (gdbarch, pointer);
>>>>> }
>>>>>
>>>>> void
>>>>> -set_gdbarch_remove_non_address_bits (struct gdbarch *gdbarch,
>>>>> - gdbarch_remove_non_address_bits_ftype
>>>> remove_non_address_bits)
>>>>> +set_gdbarch_remove_non_address_bits_memory (struct gdbarch
>>>>> +*gdbarch,
>>>>> +
>>>> gdbarch_remove_non_address_bits_memory_ftype
>>>>> +remove_non_address_bits_memory)
>>>>> {
>>>>> - gdbarch->remove_non_address_bits = remove_non_address_bits;
>>>>> + gdbarch->remove_non_address_bits_memory =
>>>>> + remove_non_address_bits_memory;
>>>>> }
>>>>>
>>>>> std::string
>>>>> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index
>>>>> b982fd7cd09..9fda85f860f 100644
>>>>> --- a/gdb/gdbarch-gen.h
>>>>> +++ b/gdb/gdbarch-gen.h
>>>>> @@ -684,19 +684,46 @@ extern CORE_ADDR gdbarch_addr_bits_remove
>>>>> (struct gdbarch *gdbarch, CORE_ADDR ad extern void
>>>>> set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch,
>>>>> gdbarch_addr_bits_remove_ftype *addr_bits_remove);
>>>>>
>>>>> /* On some architectures, not all bits of a pointer are significant.
>>>>> - On AArch64, for example, the top bits of a pointer may carry a "tag", which
>>>>> - can be ignored by the kernel and the hardware. The "tag" can be regarded
>> as
>>>>> - additional data associated with the pointer, but it is not part of the
>> address.
>>>>> + On AArch64 and amd64, for example, the top bits of a pointer may carry a
>>>>> + "tag", which can be ignored by the kernel and the hardware. The
>>>>> + "tag" can
>>>> be
>>>>> + regarded as additional data associated with the pointer, but it is not part
>>>>> + of the address.
>>>>>
>>>>> Given a pointer for the architecture, this hook removes all the
>>>>> - non-significant bits and sign-extends things as needed. It gets used to
>>>> remove
>>>>> - non-address bits from data pointers (for example, removing the AArch64
>> MTE
>>>> tag
>>>>> - bits from a pointer) and from code pointers (removing the AArch64 PAC
>>>> signature
>>>>> - from a pointer containing the return address). */
>>>>> -
>>>>> -typedef CORE_ADDR (gdbarch_remove_non_address_bits_ftype) (struct
>>>>> gdbarch *gdbarch, CORE_ADDR pointer); -extern CORE_ADDR
>>>>> gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR
>>>>> pointer); -extern void set_gdbarch_remove_non_address_bits (struct
>>>>> gdbarch *gdbarch, gdbarch_remove_non_address_bits_ftype
>>>>> *remove_non_address_bits);
>>>>> + non-significant bits and sign-extends things as needed. It gets used to
>>>>> + remove non-address bits from pointers used for watchpoints. */
>>>>> +
>>>>> +typedef CORE_ADDR
>>>>> +(gdbarch_remove_non_address_bits_watchpoint_ftype)
>>>>> +(struct gdbarch *gdbarch, CORE_ADDR pointer); extern CORE_ADDR
>>>>> +gdbarch_remove_non_address_bits_watchpoint (struct gdbarch
>>>>> +*gdbarch, CORE_ADDR pointer); extern void
>>>>> +set_gdbarch_remove_non_address_bits_watchpoint (struct gdbarch
>>>>> +*gdbarch, gdbarch_remove_non_address_bits_watchpoint_ftype
>>>>> +*remove_non_address_bits_watchpoint);
>>>>> +
>>>>> +/* On some architectures, not all bits of a pointer are significant.
>>>>> + On AArch64 and amd64, for example, the top bits of a pointer may carry a
>>>>> + "tag", which can be ignored by the kernel and the hardware. The
>>>>> +"tag" can
>>>> be
>>>>> + regarded as additional data associated with the pointer, but it is not part
>>>>> + of the address.
>>>>> +
>>>>> + Given a pointer for the architecture, this hook removes all the
>>>>> + non-significant bits and sign-extends things as needed. It gets used to
>>>>> + remove non-address bits from pointers used for breakpoints. */
>>>>> +
>>>>> +typedef CORE_ADDR
>>>>> +(gdbarch_remove_non_address_bits_breakpoint_ftype)
>>>>> +(struct gdbarch *gdbarch, CORE_ADDR pointer); extern CORE_ADDR
>>>>> +gdbarch_remove_non_address_bits_breakpoint (struct gdbarch
>>>>> +*gdbarch, CORE_ADDR pointer); extern void
>>>>> +set_gdbarch_remove_non_address_bits_breakpoint (struct gdbarch
>>>>> +*gdbarch, gdbarch_remove_non_address_bits_breakpoint_ftype
>>>>> +*remove_non_address_bits_breakpoint);
>>>>> +
>>>>> +/* On some architectures, not all bits of a pointer are significant.
>>>>> + On AArch64 and amd64, for example, the top bits of a pointer may carry a
>>>>> + "tag", which can be ignored by the kernel and the hardware. The
>>>>> +"tag" can
>>>> be
>>>>> + regarded as additional data associated with the pointer, but it is not part
>>>>> + of the address.
>>>>> +
>>>>> + Given a pointer for the architecture, this hook removes all the
>>>>> + non-significant bits and sign-extends things as needed. It gets used to
>>>>> + remove non-address bits from any pointer used to access memory.
>>>>> + */
>>>>> +
>>>>> +typedef CORE_ADDR (gdbarch_remove_non_address_bits_memory_ftype)
>>>>> +(struct gdbarch *gdbarch, CORE_ADDR pointer); extern CORE_ADDR
>>>>> +gdbarch_remove_non_address_bits_memory (struct gdbarch *gdbarch,
>>>>> +CORE_ADDR pointer); extern void
>>>>> +set_gdbarch_remove_non_address_bits_memory (struct gdbarch
>>>>> +*gdbarch, gdbarch_remove_non_address_bits_memory_ftype
>>>>> +*remove_non_address_bits_memory);
>>>>>
>>>>> /* Return a string representation of the memory tag TAG. */
>>>>>
>>>>> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
>>>>> index 4006380076d..cc7c6d8677b 100644
>>>>> --- a/gdb/gdbarch_components.py
>>>>> +++ b/gdb/gdbarch_components.py
>>>>> @@ -1232,18 +1232,55 @@ possible it should be in TARGET_READ_PC
>>>> instead).
>>>>> Method(
>>>>> comment="""
>>>>> On some architectures, not all bits of a pointer are significant.
>>>>> -On AArch64, for example, the top bits of a pointer may carry a
>>>>> "tag", which -can be ignored by the kernel and the hardware. The
>>>>> "tag" can be regarded as -additional data associated with the
>>>>> pointer, but it is not part of
>>>> the address.
>>>>> +On AArch64 and amd64, for example, the top bits of a pointer may
>>>>> +carry a "tag", which can be ignored by the kernel and the hardware.
>>>>> +The "tag" can be regarded as additional data associated with the
>>>>> +pointer, but it is not part of the address.
>>>>>
>>>>> Given a pointer for the architecture, this hook removes all the
>>>>> -non-significant bits and sign-extends things as needed. It gets
>>>>> used to remove -non-address bits from data pointers (for example,
>>>>> removing the AArch64 MTE tag -bits from a pointer) and from code
>>>>> pointers (removing the AArch64 PAC signature -from a pointer
>>>>> containing the return
>>>> address).
>>>>> +non-significant bits and sign-extends things as needed. It gets
>>>>> +used to remove non-address bits from pointers used for watchpoints.
>>>>> """,
>>>>> type="CORE_ADDR",
>>>>> - name="remove_non_address_bits",
>>>>> + name="remove_non_address_bits_watchpoint",
>>>>> + params=[("CORE_ADDR", "pointer")],
>>>>> + predefault="default_remove_non_address_bits",
>>>>> + invalid=False,
>>>>> +)
>>>>> +
>>>>> +Method(
>>>>> + comment="""
>>>>> +On some architectures, not all bits of a pointer are significant.
>>>>> +On AArch64 and amd64, for example, the top bits of a pointer may
>>>>> +carry a "tag", which can be ignored by the kernel and the hardware.
>>>>> +The "tag" can be regarded as additional data associated with the
>>>>> +pointer, but it is not part of the address.
>>>>> +
>>>>> +Given a pointer for the architecture, this hook removes all the
>>>>> +non-significant bits and sign-extends things as needed. It gets
>>>>> +used to remove non-address bits from pointers used for breakpoints.
>>>>> +""",
>>>>> + type="CORE_ADDR",
>>>>> + name="remove_non_address_bits_breakpoint",
>>>>> + params=[("CORE_ADDR", "pointer")],
>>>>> + predefault="default_remove_non_address_bits",
>>>>> + invalid=False,
>>>>> +)
>>>>> +
>>>>> +Method(
>>>>> + comment="""
>>>>> +On some architectures, not all bits of a pointer are significant.
>>>>> +On AArch64 and amd64, for example, the top bits of a pointer may
>>>>> +carry a "tag", which can be ignored by the kernel and the hardware.
>>>>> +The "tag" can be regarded as additional data associated with the
>>>>> +pointer, but it is not part of the address.
>>>>> +
>>>>> +Given a pointer for the architecture, this hook removes all the
>>>>> +non-significant bits and sign-extends things as needed. It gets
>>>>> +used to remove non-address bits from any pointer used to access memory.
>>>>> +""",
>>>>> + type="CORE_ADDR",
>>>>> + name="remove_non_address_bits_memory",
>>>>> params=[("CORE_ADDR", "pointer")],
>>>>> predefault="default_remove_non_address_bits",
>>>>> invalid=False,
>>>>> diff --git a/gdb/loongarch-linux-nat.c b/gdb/loongarch-linux-nat.c
>>>>> index bc9927dd751..dc4a019614a 100644
>>>>> --- a/gdb/loongarch-linux-nat.c
>>>>> +++ b/gdb/loongarch-linux-nat.c
>>>>> @@ -613,7 +613,7 @@ loongarch_linux_nat_target::stopped_data_address
>>>> (CORE_ADDR *addr_p)
>>>>> kernel can potentially be tagged addresses. */
>>>>> struct gdbarch *gdbarch = thread_architecture (inferior_ptid);
>>>>> const CORE_ADDR addr_trap
>>>>> - = gdbarch_remove_non_address_bits (gdbarch, (CORE_ADDR)
>>>> siginfo.si_addr);
>>>>> + = aarch64_remove_non_address_bits (gdbarch, (CORE_ADDR)
>>>>> + siginfo.si_addr);
>>>>>
>>>>> /* Check if the address matches any watched address. */
>>>>> state = loongarch_get_debug_reg_state (inferior_ptid.pid ());
>>>>
>>>> I think the loongarch-linux-nat.c change is spurious. Could you
>>>> please address that?
>>>
>>> Why is it spurious? What do you mean exactly?
>>
>> The intention behind this change isn't clear to me, but the Loongson native Linux
>> layer using a AArch64 architecture function does not make sense.
>>
>> I haven't built a LoongArch native Linux target, but I'd guess that it will run into a
>> build error with this change, if built for LoongArch only.
>>
>> The right approach, in my mind, is to drop this change, but you have to touch this
>> code due to your changes. It begs the question, why is the LoongArch native code
>> calling gdbarch_remove_non_address_bits here without even registering a
>> function for the gdbarch hook to call?
>
> Right, I agree.
>
>> Without this hook, we'll invoke the dummy implementation in gdb/arch-
>> utils.c:default_remove_non_address_bits, and that just returns the unmodified
>> pointer.
>
> True, that's probably why nobody noticed before.
>
>> This is a question for the LoongArch maintainer (cc-ed). If we don't hear back, we
>> should probably change the LoongArch code to not call
>> gdbarch_remove_non_address_bits. I think that was the original intention, but
>> there might've been a copy/paste error somewhere.
>
> Ok. Let's wait a bit. Thanks for the detailed feedback.
Alternatively, if you could replace, in the LoongArch code, gdbarch_remove_non_address_bits
with one of the three new hooks, that should work as well. I think even cleaner would be to
just drop the hook for Loongarch and assign siginfo.si_addr directly to addr_trap in the code.
Then we wouldn't need to delay this change further. How does thar sound for a v7 (and hopefully last)?
More information about the Gdb-patches
mailing list