Re: [PATCH v5 17/25] AArch64: Report tag violation error information

Luis Machado via Gdb-patches
2021-02-05T14:59:56Z

On 2/5/21 1:22 AM, Simon Marchi wrote:
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index 1063dd7a82..4ccee9c34e 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -1700,6 +1700,69 @@ aarch64_linux_memtag_to_string (struct gdbarch *gdbarch,
>> return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>> }
>>
>> +/* AArch64 Linux implementation of the report_signal_info gdbarch
>> + hook. Displays information about possible memory tag violations. */
>> +
>> +static void
>> +aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
>> + struct ui_out *uiout,
>> + enum gdb_signal siggnal)
>> +{
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> + if (!tdep->has_mte () || siggnal != GDB_SIGNAL_SEGV)
>> + return;
>> +
>> + CORE_ADDR fault_addr = 0;
>> + long si_code = 0;
>> +
>> + try
>> + {
>> + /* Sigcode tells us if the segfault is actually a memory tag
>> + violation. */
>> + si_code = parse_and_eval_long ("$_siginfo.si_code");
>> +
>> + fault_addr
>> + = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
>> + }
>> + catch (const gdb_exception_error &exception)
>> + {
>> + exception_print (gdb_stderr, exception);
>> + return;
>> + }
>> +
>> + /* If this is not a memory tag violation, just return. */
>> + if (si_code != SEGV_MTEAERR && si_code != SEGV_MTESERR)
>> + return;
>> +
>> + uiout->text ("\n");
>> +
>> + uiout->field_string ("sigcode-meaning", _("Memory tag violation"));
>> +
>> + /* For synchronous faults, show additional information. */
>> + if (si_code == SEGV_MTESERR)
>> + {
>> + uiout->text (_(" while accessing address "));
>> + uiout->field_core_addr ("fault-addr", gdbarch, fault_addr);
>> + uiout->text ("\n");
>
> I guess there's no easy way of getting the logical address that was used
> to attempt the memory access? I think it would be nice to say something
> like "Your pointer had logical tag X, but the pointed memory has
> allocation tag Y". But if we can't, we can't.
>

Yeah. I raised this last year with the kernel folks. It was a
possibility back then, but not the way it was designed at first.

I checked again and it isn't clear if this is exposed in a convenient
way, as it was enabled via a sigaction argument. GDB may need special
handling to filter out the top byte from siginfo's si_addr.

I'll check with the latest RC.

>> +
>> + gdb::optional<CORE_ADDR> atag = aarch64_mte_get_atag (fault_addr);
>> +
>> + if (!atag.has_value ())
>> + uiout->text (_("Allocation tag unavailable"));
>> + else
>> + {
>> + uiout->text (_("Allocation tag "));
>> + uiout->field_string ("allocation-tag", hex_string (*atag));
>> + }
>> + }
>> + else
>> + {
>> + uiout->text ("\n");
>> + uiout->text (_("Fault address unavailable"));
>> + }
>> +}
>
> I guess all of the `uiout->field_*` will appear as the stop event in MI?
> I suppose that's fine, but we need to be careful to treat it as API and
> not change it in a backwards-incompatible way. Can you give it a try
> with MI to see the result, see if it makes sense (if you haven't
> already)?

Right. This is being exposed so MI can potentially use the information
(as is done for i386-linux). Though I wouldn't say it is guaranteed to
be kept the same.

Honestly, the goal is not to enable an MI MTE API right now. When the
functionality is pushed and working well, then we can focus on making
the MI layer MTE-aware.

The new commands, for example, don't have MI counterparts at this point.

Given only a handful of architectures are using tagged memory, I'm not
even sure if it is worth coming up with an MI API yet.

>
> And if this indeed adds new MI fields, they should be documented, in
> theory. Although I realize that there are equivalent fields in
> i386-linux-tdep.c and sparc64-linux-tdep.c which aren't.

I have no problem documenting the current ones.

>
> But otherwise, that LGTM.

Thanks!