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

Time to add a bit more descriptiveness to the dumps.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 dump/dump.c           | 106 ++++++++++++++++++++++++++++++++++++------
 include/sysemu/dump.h |   1 +
 2 files changed, 94 insertions(+), 13 deletions(-)

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

>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  dump/dump.c           | 106 ++++++++++++++++++++++++++++++++++++------
>  include/sysemu/dump.h |   1 +
>  2 files changed, 94 insertions(+), 13 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 467d934bc1..31e2a85372 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s)
>      close(s->fd);
>      g_free(s->guest_note);
>      g_free(s->elf_header);
> +    g_array_unref(s->string_table_buf);
>      s->guest_note = NULL;
>      if (s->resume) {
>          if (s->detached) {
> @@ -359,14 +360,47 @@ static size_t write_elf_section_hdr_zero(DumpState *s, void *buff)
>      return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
>  }
>
> +static void write_elf_section_hdr_string(DumpState *s, void *buff)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;
> +    int shdr_size;
> +    void *shdr = buff;
> +
> +    if (dump_is_64bit(s)) {
> +        shdr_size = sizeof(Elf64_Shdr);
> +        memset(&shdr64, 0, shdr_size);
> +        shdr64.sh_type = SHT_STRTAB;
> +        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
> +        shdr64.sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> +        shdr64.sh_size = s->string_table_buf->len;
> +        shdr = &shdr64;
> +    } else {
> +        shdr_size = sizeof(Elf32_Shdr);
> +        memset(&shdr32, 0, shdr_size);
> +        shdr32.sh_type = SHT_STRTAB;
> +        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
> +        shdr32.sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> +        shdr32.sh_size = s->string_table_buf->len;
> +        shdr = &shdr32;
> +    }
> +
> +    memcpy(buff, shdr, shdr_size);
> +}
> +
>  static void prepare_elf_section_hdrs(DumpState *s)
>  {
>      uint8_t *buff_hdr;
> -    size_t len, sizeof_shdr;
> +    size_t len, size = 0, sizeof_shdr;
> +    Elf64_Ehdr *hdr64 = s->elf_header;
> +    Elf32_Ehdr *hdr32 = s->elf_header;
>
>      /*
>       * Section ordering:
>       * - HDR zero (if needed)
> +     * - String table hdr
>       */
>      sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
>      len = sizeof_shdr * s->shdr_num;
> @@ -377,6 +411,22 @@ static void prepare_elf_section_hdrs(DumpState *s)
>      if (s->phdr_num == PN_XNUM) {
>              write_elf_section_hdr_zero(s, buff_hdr);
>      }
> +    buff_hdr += size;
> +
> +    if (s->shdr_num < 2) {
> +        return;
> +    }
> +
> +    /*
> +     * String table needs to be last section since strings are added
> +     * via arch_sections_write_hdr().
> +     */
> +    write_elf_section_hdr_string(s, buff_hdr);
> +    if (dump_is_64bit(s)) {
> +        hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> +    } else {
> +        hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> +    }
>  }
>
>  static void prepare_elf_sections(DumpState *s, Error **errp)
> @@ -405,11 +455,18 @@ static void write_elf_sections(DumpState *s, Error **errp)
>  {
>      int ret;
>
> -    /* Write section zero */
> +    /* Write section zero and arch sections */
>      ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "dump: failed to write section data");
>      }
> +
> +    /* Write string table data */
> +    ret = fd_write_vmcore(s->string_table_buf->data,
> +                          s->string_table_buf->len, s);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "dump: failed to write string table data");
> +    }
>  }
>
>  static void write_data(DumpState *s, void *buf, int length, Error **errp)
> @@ -592,6 +649,9 @@ static void dump_begin(DumpState *s, Error **errp)
>       *   --------------
>       *   |  memory     |
>       *   --------------
> +     *   |  sectn data |
> +     *   --------------
> +
>       *
>       * we only know where the memory is saved after we write elf note into
>       * vmcore.
> @@ -677,6 +737,7 @@ static void create_vmcore(DumpState *s, Error **errp)
>          return;
>      }
>
> +    /* Iterate over memory and dump it to file */
>      dump_iterate(s, errp);
>      if (*errp) {
>          return;
> @@ -1659,6 +1720,13 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      s->has_filter = has_filter;
>      s->begin = begin;
>      s->length = length;
> +    /* First index is 0, it's the special null name */
> +    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> +    /*
> +     * Allocate the null name, due to the clearing option set to true
> +     * it will be 0.
> +     */
> +    g_array_set_size(s->string_table_buf, 1);
>
>      memory_mapping_list_init(&s->list);
>
> @@ -1819,19 +1887,31 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          }
>      }
>
> -    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.

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

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

Hi

On Thu, Jul 14, 2022 at 3:54 PM Janosch Frank <frankja@linux.ibm.com> wrote:

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

lgtm, thanks


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