Re: [PATCH v5 12/18] dump/dump: Add section string table support

From: Steffen Eiden <seiden@linux.ibm.com>
To: Janosch Frank <frankja@linux.ibm.com>, qemu-devel@nongnu.org
Cc: marcandre.lureau@redhat.com, pbonzini@redhat.com,
	mhartmay@linux.ibm.com,  borntraeger@linux.ibm.com,
	imbrenda@linux.ibm.com, pasic@linux.ibm.com, cohuck@redhat.com,
	thuth@redhat.com, qemu-s390x@nongnu.org, scgl@linux.ibm.com
Subject: Re: [PATCH v5 12/18] dump/dump: Add section string table support
Date: Tue, 30 Aug 2022 13:35:53 +0200	[thread overview]
Message-ID: <67ed362f-2e25-0ee9-baae-5e2a0ac52749@linux.ibm.com> (raw)
In-Reply-To: <20220811121111.9878-13-frankja@linux.ibm.com>

Hi Janosch,

On 8/11/22 14:11, 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.
> 
> Signed-off-by: Janosch Frank <frankja@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
[ snip ]
>   }
>   
> +static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;
> +    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"));
I think you mixed up .strtab and .shstrtab here.
'.shstrtab' should be used here.

The ELF specs define bots as follows (from man 5 elf) :

        .shstrtab
               This section holds section names.  This section is of type
               SHT_STRTAB.  No attribute types are used.

        .strtab
               This section holds strings, most commonly the strings that
               represent the names associated with symbol table entries.
               If the file has a loadable segment that includes the
               symbol string table, the section's attributes will include
               the SHF_ALLOC bit.  Otherwise, the bit will be off.  This
               section is of type SHT_STRTAB.

However, the name lookup works, as you correctly specified that this 
section holds the section header names via the 'e_shstrndx' field in the 
elf header.

> +        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);
> +
[snip]
Also, with your patches the dump output places the headers in this ordering:
[elf hdr]
[section hdrs]
[program hdrs]

**normally** program hdrs are placed before section hdrs,
but this is just a convention IIRC.

Steffen