Re: [PATCH v3 2/9] dump: Remove the sh_info variable

[Janosch Frank]

On 3/31/22 10:58, Marc-André Lureau wrote:
> Hi
>
> On Wed, Mar 30, 2022 at 4:48 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> > There's no need to have phdr_num and sh_info at the same time. We can
> > make phdr_num 32 bit and set PN_XNUM when we write the header if
> > phdr_num >= PN_XNUM.
>
> I am not fond of this change tbh, mixing variables, casting a u32 to u16..
>
> Could you provide more motivation? This seems to contradict also your
> cleanup approach in the later patch "Add more offset variables".

I can't fully follow you there. Where do I cast to u16 or mix variables?

My idea for this change:
ph_num is made independent of the data structure that it ends up in. We 
use ph_num as the only source for decisions and elf structure 
manipulation down the line since it contains the maximum possible number 
of section headers.

This way we move the extra handling to the locations where it belongs 
and where an explanation for that behavior makes most sense:
The ehdr write function and the section write function

Without knowing the ELF spec, could you tell me what sh_info stores when 
first reading this code? Going from the name it might be section header 
information, whatever that would mean.

I'd be happy to add comments to write_elf(32|64)_header() though so it's 
a bit more clear why we need to set PN_XNUM. I've already added one to 
dump_begin but that's not where we use PN_XNUM.
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   dump/dump.c           | 34 ++++++++++++++--------------------
> >   include/sysemu/dump.h |  3 +--
> >   2 files changed, 15 insertions(+), 22 deletions(-)
> >
> > diff --git a/dump/dump.c b/dump/dump.c
> > index 58c4923fce..0e6187c962 100644
> > --- a/dump/dump.c
> > +++ b/dump/dump.c
> > @@ -124,6 +124,7 @@ static int fd_write_vmcore(const void *buf, size_t
> > size, void *opaque)
> >
> >   static void write_elf64_header(DumpState *s, Error **errp)
> >   {
> > +    uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
>
> Please use MIN()
>
>
> >       Elf64_Ehdr elf_header;
> >       int ret;
> >
> > @@ -138,9 +139,9 @@ static void write_elf64_header(DumpState *s, Error
> > **errp)
> >       elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> >       elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
> >       elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
> > -    elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> > +    elf_header.e_phnum = cpu_to_dump16(s, phnum);
> >       if (s->have_section) {
> > -        uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
> > s->sh_info;
> > +        uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
> > s->phdr_num;
> >
> >           elf_header.e_shoff = cpu_to_dump64(s, shoff);
> >           elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> > @@ -155,6 +156,7 @@ static void write_elf64_header(DumpState *s, Error
> > **errp)
> >
> >   static void write_elf32_header(DumpState *s, Error **errp)
> >   {
> > +    uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
>
> same
>
>
> >       Elf32_Ehdr elf_header;
> >       int ret;
> >
> > @@ -169,9 +171,9 @@ static void write_elf32_header(DumpState *s, Error
> > **errp)
> >       elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> >       elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
> >       elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
> > -    elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> > +    elf_header.e_phnum = cpu_to_dump16(s, phnum);
> >       if (s->have_section) {
> > -        uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
> > s->sh_info;
> > +        uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
> > s->phdr_num;
> >
> >           elf_header.e_shoff = cpu_to_dump32(s, shoff);
> >           elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> > @@ -358,12 +360,12 @@ static void write_elf_section(DumpState *s, int
> > type, Error **errp)
> >       if (type == 0) {
> >           shdr_size = sizeof(Elf32_Shdr);
> >           memset(&shdr32, 0, shdr_size);
> > -        shdr32.sh_info = cpu_to_dump32(s, s->sh_info);
> > +        shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
> >           shdr = &shdr32;
> >       } else {
> >           shdr_size = sizeof(Elf64_Shdr);
> >           memset(&shdr64, 0, shdr_size);
> > -        shdr64.sh_info = cpu_to_dump32(s, s->sh_info);
> > +        shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> >           shdr = &shdr64;
> >       }
> >
> > @@ -478,13 +480,6 @@ static void write_elf_loads(DumpState *s, Error
> > **errp)
> >       hwaddr offset, filesz;
> >       MemoryMapping *memory_mapping;
> >       uint32_t phdr_index = 1;
> > -    uint32_t max_index;
> > -
> > -    if (s->have_section) {
> > -        max_index = s->sh_info;
> > -    } else {
> > -        max_index = s->phdr_num;
> > -    }
> >
> >       QTAILQ_FOREACH(memory_mapping, &s->list.head, next) {
> >           get_offset_range(memory_mapping->phys_addr,
> > @@ -502,7 +497,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
> >               return;
> >           }
> >
> > -        if (phdr_index >= max_index) {
> > +        if (phdr_index >= s->phdr_num) {
> >               break;
> >           }
> >       }
> > @@ -1801,22 +1796,21 @@ static void dump_init(DumpState *s, int fd, bool
> > has_format,
> >           s->phdr_num += s->list.num;
> >           s->have_section = false;
> >       } else {
> > +        /* sh_info of section 0 holds the real number of phdrs */
> >           s->have_section = true;
> > -        s->phdr_num = PN_XNUM;
> > -        s->sh_info = 1; /* PT_NOTE */
> >
> >           /* the type of shdr->sh_info is uint32_t, so we should avoid
> > overflow */
> >           if (s->list.num <= UINT32_MAX - 1) {
> > -            s->sh_info += s->list.num;
> > +            s->phdr_num += s->list.num;
> >           } else {
> > -            s->sh_info = UINT32_MAX;
> > +            s->phdr_num = UINT32_MAX;
> >           }
> >       }
> >
> >       if (s->dump_info.d_class == ELFCLASS64) {
> >           if (s->have_section) {
> >               s->memory_offset = sizeof(Elf64_Ehdr) +
> > -                               sizeof(Elf64_Phdr) * s->sh_info +
> > +                               sizeof(Elf64_Phdr) * s->phdr_num +
> >                                  sizeof(Elf64_Shdr) + s->note_size;
> >           } else {
> >               s->memory_offset = sizeof(Elf64_Ehdr) +
> > @@ -1825,7 +1819,7 @@ static void dump_init(DumpState *s, int fd, bool
> > has_format,
> >       } else {
> >           if (s->have_section) {
> >               s->memory_offset = sizeof(Elf32_Ehdr) +
> > -                               sizeof(Elf32_Phdr) * s->sh_info +
> > +                               sizeof(Elf32_Phdr) * s->phdr_num +
> >                                  sizeof(Elf32_Shdr) + s->note_size;
> >           } else {
> >               s->memory_offset = sizeof(Elf32_Ehdr) +
> > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> > index 250143cb5a..b463fc9c02 100644
> > --- a/include/sysemu/dump.h
> > +++ b/include/sysemu/dump.h
> > @@ -154,8 +154,7 @@ typedef struct DumpState {
> >       GuestPhysBlockList guest_phys_blocks;
> >       ArchDumpInfo dump_info;
> >       MemoryMappingList list;
> > -    uint16_t phdr_num;
> > -    uint32_t sh_info;
> > +    uint32_t phdr_num;
> >       bool have_section;
> >       bool resume;
> >       bool detached;
> > --
> > 2.32.0