[PATCH v2] [gdb/tdep] Fix calc_ldst_datasize for strb
Tom de Vries
tdevries@suse.de
Wed Aug 27 19:57:33 GMT 2025
On 8/27/25 20:23, Alice Carlotti wrote:
> On Tue, Aug 19, 2025 at 12:13:04PM +0200, Tom de Vries wrote:
>> Consider aarch64 instruction strb w1, [x0].
>>
>> It stores the least significant byte of 32-bit register w1 to memory, at an
>> address indicated by x0.
>>
>> Currently, function calc_ldst_datasize returns 4 for this insn.
>>
>> The way it calculates load/store data size, is to accumulate the size of the
>> operands before the address operand, and indeed that's 4 in this case.
>>
>> But obviously, for strb the data size is 1.
>>
>> The function seems to have been written specifically for FEAT_LRCPC3
>> (Load-Acquire RCpc instructions version 3) instructions [1].
>>
>> Fix this by:
>> - getting the access size from the address operand, if possible, and
>> - otherwise falling back to the current implementation.
>
> It's not clear to me whether your planned usage in GDB should be using the
> total size of data loaded, or the size of the individual memory accesses (when
> an instruction can access elements separately).
Hi,
thanks for the review.
The intended usage in GDB is to determine the size of the access in
terms of how far the base address of a hardware watchpoint can be away
from the byte that triggered the watchpoint.
See this RFC (
https://sourceware.org/pipermail/gdb-patches/2025-August/219952.html ).
It contains a list of inst.opcode->iclass values for which we use the
function. Perhaps that should be moved into the calc_ldst_datasize to
limit the class of instruction for which we use the new approach.
> (The existing uses of the
> function certainly want the total size).
>
If there are different concepts of size, perhaps we should cater for
this in different functions with names making clear which of the
concepts we're calculating, or add a parameter to the function to tell
which concept we want, or return all concepts in the same function.
> In any case, your updated function certainly doesn't return the correct size in
> all cases (although it might still be an improvement compared to the status
> quo, and I don't know if any of the broken cases matter to you).
I'm looking for a solution that's better than assuming a hardcoded size
of 16 for every single instruction.
I know it's hard to get this right for all instructions, because of SVE.
My goal is get this right for the simple cases, like a strb. I don't
know whether it's feasible but getting the right sizes for say all
instructions on an M1 (one of my test platforms) would be already great.
> Some possible
> issues that I'm aware of include:
>
> - SVE gather loads/stores with operands like SVE_ADDR_RZ_XTW_22 use the address
> operand qualifier to indicate the size of the address elements, not the data
> elements. I've considered changing this to be more consistent with other
> address operands, but that's so far just an idea for a future cleanup.
> - Instructions like ldff1sb with operands like SVE_ADDR_RR use QLF_NIL for the
> address operand, so would pick up the width of the widened vector elements
> instead.
> - Summing qualifier sizes or multiplying by the number of non-address operands
> would be incorrect for RMW atomic instructions.
> - Multiplying by the number of non-address operands would also be incorrect if
> one of those operands is an SVE predicate.
> - Would we need to care about FEAT_MOPS instructions?
>
Thanks for listing these.
Just to be clear about this, I'm not sure yet if the use I want to add
to GDB will make it.
So, does a patch like this on its own trying to change the
calc_ldst_datasize for strb from 4 to 1 make sense, or is this
irrelevant or even incorrect from binutils point of view?
Thanks,
- Tom
> Alice
>
>
>>
>> Add unit tests for calling calc_ldst_datasize with:
>> - strb,
>> - ldp/stp, for which the access size from the address operand needs to be
>> multiplied by two,
>> - ldap1, an insn from gas/testsuite/gas/aarch64/rcpc3-fp.s,
>> - stilp, an insn from gas/testsuite/gas/aarch64/rcpc3.s, and
>> - mov, for which 0 should be returned.
>>
>> Tested gdb and gas testsuite on aarch64-linux.
>>
>> Approved-By: Luis Machado <luis.machado.foss@gmail.com> (gdb)
>>
>> PR tdep/33283
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33283
>>
>> [1] https://sourceware.org/pipermail/binutils/2024-January/131805.html
>> ---
>> gdb/aarch64-tdep.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>> opcodes/aarch64-opc.c | 32 ++++++++++++++++++++----------
>> 2 files changed, 67 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index f2e3ce2a36c..4dd3e25955c 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -4914,6 +4914,49 @@ static void aarch64_process_record_test (void);
>> }
>> #endif
>>
>> +#if GDB_SELF_TEST
>> +namespace selftests {
>> +
>> +static void
>> +aarch64_insn_analysis (void)
>> +{
>> + uint32_t insn;
>> + aarch64_inst inst;
>> +
>> + /* 0x39000001 == strb w1, [x0]. */
>> + insn = 0x39000001;
>> + SELF_CHECK (aarch64_decode_insn (insn, &inst, true, NULL) == 0);
>> + SELF_CHECK (calc_ldst_datasize (inst.operands) == 1);
>> +
>> + /* 0xa8c17bfd == ldp x29, x30, [sp], #16. */
>> + insn = 0xa8c17bfd;
>> + SELF_CHECK (aarch64_decode_insn (insn, &inst, true, NULL) == 0);
>> + SELF_CHECK (calc_ldst_datasize (inst.operands) == 16);
>> +
>> + /* 0xa9bf7bf0 == stp x16, x30, [sp, #-16]!. */
>> + insn = 0xa9bf7bf0;
>> + SELF_CHECK (aarch64_decode_insn (insn, &inst, true, NULL) == 0);
>> + SELF_CHECK (calc_ldst_datasize (inst.operands) == 16);
>> +
>> + /* 0x0d4187e1 == ldap1 {v1.d}[0], [sp]. */
>> + insn = 0x0d4187e1;
>> + SELF_CHECK (aarch64_decode_insn (insn, &inst, true, NULL) == 0);
>> + SELF_CHECK (calc_ldst_datasize (inst.operands) == 8);
>> +
>> + /* 0xd9011860 == stilp x0, x1, [x3]. */
>> + insn = 0xd9011860;
>> + SELF_CHECK (aarch64_decode_insn (insn, &inst, true, NULL) == 0);
>> + SELF_CHECK (calc_ldst_datasize (inst.operands) == 16);
>> +
>> + /* 0x52800000 == mov w0, #0x0. */
>> + insn = 0x52800000;
>> + SELF_CHECK (aarch64_decode_insn (insn, &inst, true, NULL) == 0);
>> + SELF_CHECK (calc_ldst_datasize (inst.operands) == 0);
>> +}
>> +
>> +}
>> +#endif
>> +
>> INIT_GDB_FILE (aarch64_tdep)
>> {
>> gdbarch_register (bfd_arch_aarch64, aarch64_gdbarch_init,
>> @@ -4933,6 +4976,8 @@ When on, AArch64 specific debugging is enabled."),
>> selftests::aarch64_analyze_prologue_test);
>> selftests::register_test ("aarch64-process-record",
>> selftests::aarch64_process_record_test);
>> + selftests::register_test ("aarch64-insn-analysis",
>> + selftests::aarch64_insn_analysis);
>> #endif
>> }
>>
>> diff --git a/opcodes/aarch64-opc.c b/opcodes/aarch64-opc.c
>> index 5980841c394..8cdce6f82cc 100644
>> --- a/opcodes/aarch64-opc.c
>> +++ b/opcodes/aarch64-opc.c
>> @@ -1750,24 +1750,36 @@ check_za_access (const aarch64_opnd_info *opnd,
>> return true;
>> }
>>
>> -/* Given a load/store operation, calculate the size of transferred data via a
>> - cumulative sum of qualifier sizes preceding the address operand in the
>> - OPNDS operand list argument. */
>> +/* Given a load/store operation, calculate the size of transferred data. */
>> +
>> int
>> calc_ldst_datasize (const aarch64_opnd_info *opnds)
>> {
>> - unsigned num_bytes = 0; /* total number of bytes transferred. */
>> - enum aarch64_operand_class opnd_class;
>> - enum aarch64_opnd type;
>> + int addr_opnd = -1;
>>
>> for (int i = 0; i < AARCH64_MAX_OPND_NUM; i++)
>> {
>> - type = opnds[i].type;
>> - opnd_class = aarch64_operands[type].op_class;
>> + enum aarch64_opnd opnd_type = opnds[i].type;
>> + enum aarch64_operand_class opnd_class
>> + = aarch64_operands[opnd_type].op_class;
>> if (opnd_class == AARCH64_OPND_CLASS_ADDRESS)
>> - break;
>> - num_bytes += aarch64_get_qualifier_esize (opnds[i].qualifier);
>> + {
>> + addr_opnd = i;
>> + break;
>> + }
>> }
>> +
>> + if (addr_opnd == -1)
>> + return 0;
>> +
>> + enum aarch64_opnd_qualifier addr_opnd_qualifier
>> + = opnds[addr_opnd].qualifier;
>> + if (operand_variant_qualifier_p (addr_opnd_qualifier))
>> + return addr_opnd * aarch64_get_qualifier_esize (addr_opnd_qualifier);
>> +
>> + unsigned num_bytes = 0; /* total number of bytes transferred. */
>> + for (int i = 0; i < addr_opnd; i++)
>> + num_bytes += aarch64_get_qualifier_esize (opnds[i].qualifier);
>> return num_bytes;
>> }
>>
>>
>> base-commit: 09292f4ae2ccb46130652f6b310ee7a5227326d3
>> --
>> 2.43.0
>>
More information about the Binutils
mailing list