Re: [PATCH 1/5] gdb/aarch64: Implement software single stepping for MOPS instructions

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Christophe,

Thanks for the patch review!

Christophe Lyon <christophe.lyon@linaro.org> writes:

> On Sat, 4 May 2024 at 02:05, Thiago Jung Bauermann
> <thiago.bauermann@linaro.org> wrote:
>>
>> @@ -3808,8 +3909,10 @@ aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch,
>>    if (aarch64_decode_insn (insn, &inst, 1, NULL) != 0)
>>      return NULL;
>>
>> -  /* Look for a Load Exclusive instruction which begins the sequence.  */
>> -  if (inst.opcode->iclass == ldstexcl && bit (insn, 22))
>> +  /* Look for a Load Exclusive instruction which begins the sequence,
>> +     or for a MOPS instruction.  */
>> +  if ((inst.opcode->iclass == ldstexcl && bit (insn, 22))
>> +      || AARCH64_CPU_HAS_FEATURE (*inst.opcode->avariant, MOPS))
>
> Sorry for the naive question: doesn't this check that the CPU supports
> MOPS, rather than the instruction sequence is a MOPS one?

It's an interesting question actually, at least for me because it's my
first time calling opcode functions.  You're right that
AARCH64_CPU_HAS_FEATURE checks whether the given CPU supports the given
feature.  But the call above uses the inst.opcode->avariant "CPU":

  /* Which architecture variant provides this instruction.  */
  const aarch64_feature_set *avariant;

For MOPS instructions that field points to aarch64_feature_mops, defined
in opcodes/aarch64-tbl.h as:

static const aarch64_feature_set aarch64_feature_mops =
  AARCH64_FEATURE (MOPS);

Which defines a CPU containing only the MOPS feature.

I also checked — just to be sure — that whether or not the VM's CPU
support the MOPS feature, the above description holds and the check
returns true for a MOPS instruction and false otherwise.

>>      {
>>        /* We can't displaced step atomic sequences.  */
>>        return NULL;

-- 
Thiago