Home Original page

[PATCH v2] [gdb/tdep] Fix calc_ldst_datasize for strb

Alice Carlotti alice.carlotti@arm.com
Wed Aug 27 18:23:42 GMT 2025 
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).  (The existing uses of the
function certainly want the total size).

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).  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?

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