[PATCH v6 1/2] gdb: Make tagged pointer support configurable.
Luis Machado
luis.machado@arm.com
Tue Nov 5 15:14:07 GMT 2024
On 11/5/24 15:09, Schimpe, Christina wrote:
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: Tuesday, November 5, 2024 3:51 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: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.
>
> I am also in favour of your second suggestion to drop the hook here.
> In that case, it should be separate patch I think.
> I could post it independently of this series and quickly merge before posting a v7 of my series.
Yes, a separate one would be nice, please.
>
> LoongArch doesn't supported tagged addresses, does it? If it's not supported,
> I would suggest to fix it as follows:
>
> diff --git a/gdb/loongarch-linux-nat.c b/gdb/loongarch-linux-nat.c
> index dc4a019614a..fd3581bbd30 100644
> --- a/gdb/loongarch-linux-nat.c
> +++ b/gdb/loongarch-linux-nat.c
> @@ -608,17 +608,11 @@ loongarch_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p)
> if (siginfo.si_signo != SIGTRAP || (siginfo.si_code & 0xffff) != TRAP_HWBKPT)
> return false;
>
> - /* Make sure to ignore the top byte, otherwise we may not recognize a
> - hardware watchpoint hit. The stopped data addresses coming from the
> - kernel can potentially be tagged addresses. */
> - struct gdbarch *gdbarch = thread_architecture (inferior_ptid);
> - const CORE_ADDR addr_trap
> - = 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 ());
>
> - return loongarch_stopped_data_address (state, addr_trap, addr_p);
> + return
> + loongarch_stopped_data_address (state, (CORE_ADDR) siginfo.si_addr, addr_p);
>
> Does that make sense to you?
>
It does. Thanks!
More information about the Gdb-patches
mailing list