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>]

On 10/3/23 18:23, Andrew Burgess wrote:
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 9/27/23 18:56, Andrew Burgess wrote:
>>> Luis Machado <luis.machado@arm.com> writes:
>>>
>>>> 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.
>>>
>>> I think we agree on the above.
>>>
>>>> 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.
>>>
>>> That's unfortunate.  I guess that might be because in some places we
>>> have an inferior selected but no thread?
>>>
>>> It feels like the right solution is to transition GDB to only having a
>>> per-thread gdbarch/tdesc, which would avoid problems like this in the
>>> future.
>>>
>>>>
>>>>>
>>>>> 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.
>>>
>>> This all sounds positive.
>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>> 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?
>>>
>>> I don't think so.  As you've already said, in the core file case we
>>> already don't support per-thread tdesc, even if we use the
>>> aarch64_linux_core_read_description hook - that's only called once, and
>>> will just generate a tdesc based on the main (signalled) thread as far
>>> as I can tell.
>>>
>>> Telling GDB to dump per-thread NT_GDB_TDESC is trivial, but I haven't
>>> looked at updating the core file code to read in per-thread tdesc, but I
>>> suspect you'll (eventually) need to solve that problem even when using
>>> the aarch64_linux_core_read_description hook.
>>>
>>> I know Simon has mentioned changing GDB to not write out NT_GDB_TDESC,
>>> and I think that would be fine, however, I think we would still want my
>>> proposed patch -- we should always write out the thread tdesc, not the
>>> cached global one, as that is always going to be more accurate.
>>>
>>> My proposal then would be: we merge my patch to write NT_GDB_TDESC using
>>> the per-thread tdesc, then you merge your original V7 version of this
>>> patch with no other changes.
>>>
>>> Later, if we want, we can add a new hook to avoid writing the
>>> NT_GDB_TDESC at all -- but that can come later, and would be in addition
>>> to my proposed patch, not instead of it.
>>>
>>> If you and Simon are happy with this plan then I'll go ahead and push my
>>> patch, this should unblock you, and you'll be able to merge this series.
>>>
>>> Let me know what you think,
>>
>> That sounds good to me. Thanks for going over this.
>>
>> Please let me know when you have pushed the NT_GDB_TDESC fixup.
> 
> I've pushed the patch.
> 
> I've included it again below, just for the record.
> 
> Thanks,
> Andrew

Great. Thanks for addressing this.