On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote: > 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.
Why? Is there any harm in always having a (possibly empty) string table? Can't put garbage into sh_name then but that's not relevant. Seems like it would make the code a bit simpler. I would also expect the string data to be written in this patch, instead you do that in the next. With that and Steffen's .shstrtab addressed: Reviewed-by: Janis Schoetterl-Glausch <s...@linux.ibm.com> Some minor suggestions below. > > Signed-off-by: Janosch Frank <fran...@linux.ibm.com> > --- > dump/dump.c | 71 +++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 4 +++ > 2 files changed, 75 insertions(+) > > diff --git a/dump/dump.c b/dump/dump.c > index 31eb20108c..0d6dbf453a 100644 > --- a/dump/dump.c > +++ b/dump/dump.c [...] > @@ -393,17 +400,50 @@ static void prepare_elf_section_hdr_zero(DumpState *s) > } > } > > +static void prepare_elf_section_hdr_string(DumpState *s, void *buff) > +{ > + Elf32_Shdr shdr32; > + Elf64_Shdr shdr64; Could just = {} those and drop the memset below. > + int shdr_size; > + void *shdr; > + > + 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")); Could put the ".shstrtab" in a char[] variable. > + 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); > +} [...] > @@ -1844,6 +1903,18 @@ static void dump_init(DumpState *s, int fd, bool > has_format, > } > } > > + /* > + * calculate shdr_num and elf_section_data_size so we know the offsets > and What is the elf_section_data_size thing about? > + * 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; > + } > + > if (dump_is_64bit(s)) { > s->shdr_offset = sizeof(Elf64_Ehdr); > s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num; [...]