Re: [PATCH v5 13/16] [gdb/generic] corefile/bug: Fixup (gcore) core file target description reading order

[Luis Machado <luis.machado@arm.com>]

On 9/13/23 14:50, Simon Marchi wrote:
>>> I'm not convinced that this is right.  The current role of
>>> gdbarch_core_read_description (AFAIU) is to provide a fallback to the
>>> note method.  Usually, the note method is preferred, because it's
>>> precise, but if there's no note, maybe the gdbarch can derive a tdesc
>>> from what it sees in the core.  Naturally, this use of
>>> gdbarch_core_read_description (as a fallback) has to go after trying the
>>> note method.
>>>
>>> Now, you want to to use gdbarch_core_read_description as an override to
>>> the note method, which is why you want to call it before trying the note
>>> method.  I don't think the same gdbarch method can be used for both
>>> fallback and override.  With your change, with an arch that defines a
>>> gdbarch_core_read_description hook, where we would have used the note
>>> before, we will now always use the hook.  Not what we want.
>>
>> Yeah, it seems that way unfortunately. Looking back, maybe it would've been better to define the gdb tdesc note as a per-thread entry instead.
>>
>> I suppose we can still do it from now on, but the previous core files generated by gdb would
>> still need to be handled in a special way for AArch64.
>>
>>>
>>> Some options I see:
>>>
>>>  - Add another gdbarch hook, so one is called before trying the note,
>>>    and one after.
>>>  - Add another gdbarch hook that allows the arch to modify the target
>>>    desc read from the note.  So the flow would be:
>>>    core_target::read_description creates a target from the note, then
>>>    calls the gdbarch hook.  The latter could return the same tdesc, or a
>>>    new tdesc.  The AArch64 hook could then create a new tdesc based on
>>>    the one read from the note, but with the SVE/SME bits tweaked.
>>
>> I think the first solution would work. But I see it as a temporary measure until we update the core file target description note to be per-thread.
>>
>> After that, I believe the gdbarch hook wouldn't be too useful.
> Well, as you mentioned above, there will still be the case of supporting
> old cores that only have one global target description note, right?

Yes, that's true. Though I'd expect the most common case of generated
core files to be ones generated by a crash as opposed to gdb's gcore command. And
the core files generated by the Linux kernel, for instance, wouldn't carry the target
description note.

But, as I mentioned on IRC, even with the introduction of such a gdbarch hook, we'd still
run into other issues that need to be fixed before threaded core files will work correctly
for AArch64's SVE and SME. It is something that is broken currently. But those would be
best addressed with a different patch set.

> 
> Simon
>