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 9/20/23 06:49, Luis Machado wrote:
> On 9/19/23 21:49, Simon Marchi wrote:
>> On 9/18/23 17:26, Luis Machado wrote:
>>> New entry in v7.
>>>
>>> ---
>>
>> Note: when you use `---` in a commit message, git-am drops whatever
>> comes after that, so here we lose the commit message when applying.  I
>> think the intention is that you'd put whatever you don't want to appear
>> in the commit message (like the "new in version X" notes) after the
>> `---`.  I'll try to do that from now on.
>>
> 
> Yeah, that's a bad marker indeed. Is that configuration-specific though?
> 

Since I started playing with b4 only recently, I think I didn't run into a case of
trying to apply a series containing this marker. I get the same, and what you mentioned
makes sense.

It is pretty handy, when trying to push a series, for git to take care of removing
the update entries. Might be useful to do it as part of the gdb development workflow.

>>>
>>> Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread
>>> can potentially have distinct target descriptions/gdbarches.
>>>
>>> When loading a gcore-generated core file, at the moment GDB gives priority
>>> to the target description dumped to NT_GDB_TDESC.  Though technically correct
>>> for most targets, it doesn't work correctly for AArch64 with SVE or SME
>>> support.
>>>
>>> The correct approach for AArch64/Linux is to either have per-thread target
>>> description notes in the corefiles or to rely on the
>>> gdbarch_core_read_description hook, so it can figure out the proper target
>>> description for a given thread based on the various available register notes.
>>>
>>> The former, although more correct, doesn't address the case of existing gdb's
>>> that only output a single target description note.
>>>
>>> This patch goes for the latter, and adds a new gdbarch hook to conditionalize
>>> the use of the corefile target description note. The hook is called
>>> use_target_description_from_corefile_notes.
>>>
>>> The hook defaults to returning true, meaning targets will use the corefile
>>> target description note.  AArch64 Linux overrides the hook to return false
>>> when it detects any of the SVE or SME register notes in the corefile.
>>>
>>> Otherwise it should be fine for AArch64 Linux to use the corefile target
>>> description note.
>>>
>>> When we support per-thread target description notes, then we can augment
>>> the AArch64 Linux hook to rely on those notes.
>>>
>>> Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
>>
>> The question that came to mind when reading this is (I already asked you
>> on IRC, but perhaps someone else has an idea): if GDB has to know how to
>> infer target descriptions from the core file contents (the
>> kernel-generated core files don't include target description notes, only
>> GDB-generated core files do), what advantage do we have of storing and
>> reading the target description in the core?  Are there any scenarios
>> where the target description note conveys something that GDB wouldn't
>> figure out by analyzing the core file content?
>>
> 
> According to the original commit that added NT_GDB_TDESC, there are potential use case around
> dumping a core file from a remote session. That remote target may potentially have more registers
> than what gdb might be able to infer.
> 
>>> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
>>> index 846467b8d83..bbb9b188286 100644
>>> --- a/gdb/gdbarch_components.py
>>> +++ b/gdb/gdbarch_components.py
>>> @@ -2732,3 +2732,22 @@ Read core file mappings
>>>      predefault="default_read_core_file_mappings",
>>>      invalid=False,
>>>  )
>>> +
>>> +Method(
>>> +    comment="""
>>> +Return true if the target description for all threads should be read from the
>>> +target description core file note(s).  Return false if the target description
>>> +for all threads should be inferred from the core file contents/sections.
>>> +
>>> +The corefile's bfd is passed through OBFD.
>>> +
>>> +This hook should be used by targets that can have distinct target descriptions
>>> +for each thread when the core file only holds a single target description
>>> +note.
>>
>> I think the last paragraph should be left out.  An arch could decide not
>> to use the target description note for whatever reason it sees fit, this
>> is just one example.  The reason for AArch64 appears to be sufficiently
>> described in the AArch64-specific files.
>>
> 
> Will do, thanks.
> 
>>> +""",
>>> +    type="bool",
>>> +    name="use_target_description_from_corefile_notes",
>>> +    params=[("struct bfd *", "obfd")],
>>
>> Can you name the parameter core_bfd maybe?  Just a bit more precise.
>>
> 
> That was my first try, but unfortunately core_bfd seems to be a macro, and it doesn't
> play well in this context. Maybe a longer name that dodges the core_bfd macro?
> 
> /* Binary File Diddler for the core file.  */
> 
> #define core_bfd (current_program_space->cbfd.get ())
> 

FTR, I went with corefile_bfd to dodge the macro name.

>> Otherwise:
>>
>> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>>
>> Simon
> 

I plan to push both these series (sme1/sme2) towards the end of the week.

Thanks,
Luis