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

Luis Machado via Gdb-patches
2023-09-20T05:50:30Z

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