Re: [PATCH v2 05/11] dump/dump: Add section string table support

[Janosch Frank <frankja@linux.ibm.com>]

On 7/13/22 17:58, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 13, 2022 at 5:07 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>> Time to add a bit more descriptiveness to the dumps.
> 
> Please add some more description & motivation to the patch (supposedly
> necessary for next patches), and explain that it currently doesn't
> change the dump (afaict).

How about:

As sections don't have a type like the notes do we need another way to 
determine their contents. The string table allows us to assign each 
section an identification string which architectures can then use to tag 
their sections with.

There will be no string table if the architecture doesn't add custom 
sections which are introduced in a following patch.


>>
>> -    if (dump_is_64bit(s)) {
>> -        s->phdr_offset = sizeof(Elf64_Ehdr);
>> -        s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
>> -        s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
>> -        s->memory_offset = s->note_offset + s->note_size;
>> -    } else {
>> -
>> -        s->phdr_offset = sizeof(Elf32_Ehdr);
>> -        s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
>> -        s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
>> -        s->memory_offset = s->note_offset + s->note_size;
>> +    /*
>> +     * calculate shdr_num and elf_section_data_size so we know the offsets and
>> +     * sizes of all parts.
>> +     *
>> +     * If phdr_num overflowed we have at least one section header
>> +     * More sections/hdrs can be added by the architectures
>> +     */
>> +    if (s->shdr_num > 1) {
>> +        /* Reserve the string table */
>> +        s->shdr_num += 1;
>>       }
>>
>> +    tmp = (s->phdr_num == PN_XNUM) ? s->sh_info : s->phdr_num;
>> +    if (dump_is_64bit(s)) {
>> +        s->shdr_offset = sizeof(Elf64_Ehdr);
>> +        s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
>> +        s->note_offset = s->phdr_offset + sizeof(Elf64_Phdr) * tmp;
>> +    } else {
>> +        s->shdr_offset = sizeof(Elf32_Ehdr);
>> +        s->phdr_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
>> +        s->note_offset = s->phdr_offset + sizeof(Elf32_Phdr) * tmp;
>> +    }
>> +    s->memory_offset = s->note_offset + s->note_size;
> 
> I suggest to split this in a different patch. It's not obvious that
> you can change phdr_offset / shdr_offset, it deserves a comment.

Right, will do

> 
>> +    s->section_offset = s->memory_offset + s->total_size;
>> +
>>       return;
>>
>>   cleanup:
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index 8379e29ef6..2c25c7d309 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -178,6 +178,7 @@ typedef struct DumpState {
>>       void *elf_section_hdrs;
>>       uint64_t elf_section_data_size;
>>       void *elf_section_data;
>> +    GArray *string_table_buf;  /* String table section */
>>
>>       uint8_t *note_buf;          /* buffer for notes */
>>       size_t note_buf_offset;     /* the writing place in note_buf */
>> --
>> 2.34.1
>>
>