Re: [PATCH v7 15/18] [gdb/generic] corefile/bug: Add hook to control the use of target description notes from corefiles

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

Hi Andrew,

On 9/25/23 10:57, Andrew Burgess wrote:
> Luis Machado <luis.machado@arm.com> writes:
>> Sure, that makes sense, and I don't disagree with that. But as I mentioned above, it is an
>> orthogonal problem that might be better suited as its own patch/series. A more complete patch
>> addressing this particular situation I stumbled upon.
> 
> That's fine.  I understand the urgency you have.  For me, the only issue
> I have here is that the new hook you add is to prevent GDB loading a
> "broken" NT_GDB_TDESC. I'd also like this patch to include code that
> prevents GDB from writing out "broken" NT_GDB_TDESC.
> 
> I understand we're likely stuck with the "don't load the NT_GDB_TDESC"
> hook due to broken GDB's already existing in the wild.  That sucks, but
> it is what it is.
> 
> However, having identified some broken behaviour I think we really
> should fix that at the same time as adding the work around.
> 

Yes, I see your point and think it's reasonable.

>>> When I originally added this code my problem case was RISC-V, where the
>>> CSRs (control regs) for embedded targets can be different for each
>>> target.  The CSRs are just encoded as a blob, so unless you already know
>>> which regs are present you're stuck.  For real h/w the controller
>>> (e.g. openocd) provides a tdesc (via target.xml), but for a core file we
>>> needed a way to reacquire the tdesc.
>>>
>>> My assumption at the time that the tdesc we wrote would always be
>>> correct, but clearly this isn't the case.
>>
>> That's true. With the introduction of dynamic target descriptions like sve and sme, this
>> no longer holds true.
> 
> OK. But is it really the case that the assumption is wrong, or is it
> that the current code (that I added) is just using the wrong tdesc?

For the general case, it is the wrong assumption because you can have threads
with different tdesc's/gdbarches, and using a single NT_GDB_TDESC doesn't cover
that part.

Putting aside the threaded case, it can also be incorrect for the single-threaded
case.

Imagine you start up with a vector length of 256 bits (vg == 4). At this point the
gdbarch/target description that gets stored in the inferior (the tdesc is still a
per-inferior entry IMO) says the vector length is 256 bits.

Now imagine we executed a few instructions and now the vector length is 128 bits
(vg == 2). Though gdb will cope with it just fine during debugging, because
aarch64/linux implements the thread_architecture hook, if we attempt to output
a gcore core file, the current code (without your proposed patch) will dump
the target description/gdbarch cached in the inferior. And that one says we
have a vector length of 256 bits.

So the tdesc is incorrect when we try to load the gcore file, if we trust the
NT_GDB_TDESC note. The register data in the notes has the correct information
though.

Your attached patch fixes this because it uses the signalled thread's
gdbarch/tdesc instead of the cached inferior gdbarch/tdesc.

target_gdbarch can be a bit dangerous unfortunately, but we still have to use
it in some places.

> 
> I know you described the idea of updating gcore_elf_make_tdesc_note to
> take a gdbarch* as orthogonal, but I really think this might be the way
> to solve this problem.
> 
> I've attached a patch at the end of this email that can be applied to
> head of master BEFORE your patch series.  This patch updates the
> NT_GDB_TDESC generation to use the gdbarch of the signalled thread.
> 
> Now, if I've understood how everything works, I believe that the
> NT_GDB_TDESC that is emitted _with_ my patch should be good enough -- it
> should be the correct tdesc for the signalled thread, which, if all
> threads use the same tdesc, should be good enough.
> 
> As a test, you can apply my patch, and then TEMPORARILY update
> aarch64_use_target_description_from_corefile_notes to always return
> true.  Using such a GDB you should be able to create a core file, and
> then load it back in correctly.

Yes, that works correctly from testing on my end. So at a minimum that's
already an improvement, as changing the vector length for one (or all threads,
but with all of them still using the same vector length) during execution is a
useful use case. Like a program setting up for some sve operation, and all of a
someone wants to dump a core file.

> 
> As I already mentioned, I acknowledge that we are stuck with the
> aarch64_use_target_description_from_corefile_notes hook -- there are
> broken GDBs in the wild.

Though unfortunate, the most common core files are likely coming from crashes
outside a debugging session (so not gcore).

> 
> But, if I'm correct, then I think this is the best solution for now,
> we're emitting a NT_GDB_TDESC that is mostly correct, but AArch64 will
> continue to ignore it, just as you originally proposed.
> 

Sorry, it took me a little bit to get some tests running on the simulator. Happy to
report that I see no regressions in terms of testing with the changes you proposed for
handling the signalled thread.

Also, I checked the case I described above about a program starting with vg == 4 and then
changing it to vg == 2 before we output a gcore file.

When I load the corefile back in a patched gdb (and forcing gdb to use the tdesc, by always
returning true in the aarch64-linux hook), I can see that the tdesc is the correct
one (vector length indicating vg == 2).

Without your patch, gdb used the inferior's cached gdbarch/tdesc to dump the NT_GDB_TDESC
note, which means the vector length was vg == 4, when in reality it should've been
vg == 2, as we had changed it before dumping the core file.

>>
>>>
>>> Right now RISC-V doesn't event override gdbarch_core_read_description,
>>> so if we read the encoded tdesc after checking that gdbarch hook nothing
>>> would change.
>>
>> That was my assessment back in the earlier versions of this series.
>>
>>>
>>> For other architectures, if gdbarch_core_read_description is implemented
>>> then we'll be back to using that, and that worked fine before.
>>>
>>> ... And if, in some future world we do implement
>>> gdbarch_core_read_description for RISC-V then the solution seems
>>> obvious, if there's a section containing CSRs, and there's also a
>>> section containing a tdesc, then the RISC-V
>>> gdbarch_core_read_description can just return nullptr, safe in the
>>> knowledge the generic GDB code will load the encoded tdesc.
>>>
>>>>
>>>> Except for some different names and tweaks, your proposed approach works correctly.
>>>>
>>>> I tested this and noticed the lack of NT_GDB_TDESC for a AArch64 Linux target
>>>> with sve/sme support and one thread with a differing gdbarch. I also saw the note for
>>>> a AArch64 Linux target without sve/sme support, or with sve/sme support but all threads
>>>> having the exact same gdbarch. So that looks good to me.
>>>>
>>>> Would you like this extra code to be included as part of this
>>>> particular patch?
>>>
>>> Yeah I think something like this should be added to this patch, we
>>> should stop GDB creating incorrect core files I think.
>>
>> Got it.
>>
>> I'm afraid I'm going to need some clarification on ways forward with regards to this particular
>> series. Do you still want this (or other) potential changes as part of this series or would you
>> be ok with a follow-up patch/series to (try to, haven't gone in-depth yet) address this particular
>> core file/gdbarch/threads situation?
> 
> Here's what I propose:to output the cached gdbarch/
> 
>   1. Try the patch I've supplied here, if this works for you (see above)
>   then I think this is the best solution, we merge this, then go with v7
>   of this patch unmodified,

It did work. Thanks!

But should we keep your suggested changes to check for distinct gdbarches in the threaded case? And
not output NT_GDB_TDESC if we find multiple distinct gdbarch entries?