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/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?

>>
>> 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 ())

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