Re: [PATCH 1/5] readelf: Consolidate get_[32|64]bit_section_headers

On Fri, Jul 10, 2026 at 9:41 PM Jan Beulich <[email protected]> wrote:
>
> On 09.07.2026 14:40, H.J. Lu wrote:
> > Consolidate get_32bit_section_headers and get_64bit_section_headers into
> > get_section_headers.  Use BYTE_GET_SIZE to retrieve external ELF section
> > header fields.
> >
> >       PR binutils/34356
> >       * elfcomm.h (BYTE_GET_SIZE): New.
> >       * readelf.c (get_32bit_section_headers): Moved to ...
> >       (get_section_headers): This.  Use BYTE_GET_SIZE to retrieve
> >       external ELF section header fields.
> >       (get_64bit_section_headers): Removed.
> >
> > Signed-off-by: H.J. Lu <[email protected]>
> > ---
> >  binutils/elfcomm.h |   6 ++
> >  binutils/readelf.c | 146 ++++++++++++---------------------------------
> >  2 files changed, 43 insertions(+), 109 deletions(-)
>
> While this of course is a nice reduction in code size, ...

Code size reduction isn't my main motivation.   I don't like
adding duplication codes to 2 different places.

> > --- a/binutils/elfcomm.h
> > +++ b/binutils/elfcomm.h
> > @@ -43,6 +43,12 @@ extern uint64_t byte_get_big_endian (const unsigned char *, unsigned int);
> >  #define BYTE_GET(field)              byte_get (field, sizeof (field))
> >  #define BYTE_GET_SIGNED(field)       byte_get_signed (field, sizeof (field))
> >
> > +#define BYTE_GET_SIZE(var, ptr, size) \
> > +  { \
> > +    (var) = byte_get (ptr, (size)); \
> > +    ptr += (size); \
> > +  }
>
> ... I'm not quite convinced of both this and its uses further down. Here
> I find it concerning that "ptr" is updated without that being visible at
> all at use sites.

I can change it to a real function with a pointer argument if it helps.

> As a nit, I also consider parenthesization of macro arguments inconsistent
> here. In the byte_get() invocation, neither argument should require extra
> parentheses. All other uses of the macro parameters might better be
> consistently parenthesized, even if the use of parentheses on lvalues is
> somewhat debatable.
>
> Finally for a macro put in a header please properly use do {} while (0),
> such that semicolons put at the end of use sites don't end up stray, and
> won't be at risk of breaking code like this:
>
>     if ( x )
>         BYTE_GET_SIZE (...);
>     else
>         ...;
>
> > @@ -8031,96 +8039,28 @@ get_32bit_section_headers (Filedata * filedata, bool probe)
> >    filedata->orig_section_headers = (Elf_Internal_Shdr **)
> >      xcalloc2 (num, sizeof (Elf_Internal_Shdr *));
> >
> > -  orig_internal = filedata->orig_section_headers;
> > -  for (i = 0, internal = filedata->section_headers;
> > -       i < num;
> > -       i++, internal++, orig_internal++)
> > -    {
> > -      internal->sh_name      = BYTE_GET (shdrs[i].sh_name);
> > -      internal->sh_type      = BYTE_GET (shdrs[i].sh_type);
> > -      internal->sh_flags     = BYTE_GET (shdrs[i].sh_flags);
> > -      internal->sh_addr      = BYTE_GET (shdrs[i].sh_addr);
> > -      internal->sh_offset    = BYTE_GET (shdrs[i].sh_offset);
> > -      internal->sh_size      = BYTE_GET (shdrs[i].sh_size);
> > -      internal->sh_link      = BYTE_GET (shdrs[i].sh_link);
> > -      internal->sh_info      = BYTE_GET (shdrs[i].sh_info);
> > -      internal->sh_addralign = BYTE_GET (shdrs[i].sh_addralign);
> > -      internal->sh_entsize   = BYTE_GET (shdrs[i].sh_entsize);
> > -      validate_section_info (internal, orig_internal, i, filedata,
> > -                          false, probe);
> > -    }
> > -
> > -  free (shdrs);
> > -  return true;
> > -}
> > -
> > -/* Like get_32bit_section_headers, except that it fetches 64-bit headers.  */
> > -
> > -static bool
> > -get_64bit_section_headers (Filedata * filedata, bool probe)
> > -{
> > -  Elf64_External_Shdr *  shdrs;
> > -  Elf_Internal_Shdr *    internal;
> > -  Elf_Internal_Shdr **   orig_internal;
> > -  unsigned int           i;
> > -  unsigned int           size = filedata->file_header.e_shentsize;
> > -  unsigned int           num = probe ? 1 : filedata->file_header.e_shnum;
> > -
> > -  /* PR binutils/17531: Cope with unexpected section header sizes.  */
> > -  if (size == 0 || num == 0)
> > -    return false;
> > -
> > -  /* The section header cannot be at the start of the file - that is
> > -     where the ELF file header is located.  A file with absolutely no
> > -     sections in it will use a shoff of 0.  */
> > -  if (filedata->file_header.e_shoff == 0)
> > -    return false;
> > -
> > -  if (size < sizeof * shdrs)
> > -    {
> > -      if (! probe)
> > -     error (_("The e_shentsize field in the ELF header is less than the size of an ELF section header\n"));
> > -      return false;
> > -    }
> > -
> > -  if (! probe && size > sizeof * shdrs)
> > -    warn (_("The e_shentsize field in the ELF header is larger than the size of an ELF section header\n"));
> > +  void *ptr = shdrs;
> > +  uint32_t elf_class_size = is_32bit_elf ? 4 : 8;
> >
> > -  shdrs = (Elf64_External_Shdr *) get_data (NULL, filedata,
> > -                                         filedata->file_header.e_shoff,
> > -                                            size, num,
> > -                                         probe ? NULL : _("section headers"));
> > -  if (shdrs == NULL)
> > -    return false;
> > -
> > -  filedata->section_headers = (Elf_Internal_Shdr *)
> > -    cmalloc (num, sizeof (Elf_Internal_Shdr));
> > -  if (filedata->section_headers == NULL)
> > -    {
> > -      if (! probe)
> > -     error (_("Out of memory reading %u section headers\n"), num);
> > -      free (shdrs);
> > -      return false;
> > -    }
> > -
> > -  filedata->orig_section_headers = (Elf_Internal_Shdr **)
> > -    xcalloc2 (num, sizeof (Elf_Internal_Shdr *));
> > +  Elf_Internal_Shdr *internal;
> > +  Elf_Internal_Shdr **orig_internal;
> > +  unsigned int i;
> >
> >    orig_internal = filedata->orig_section_headers;
> >    for (i = 0, internal = filedata->section_headers;
> >         i < num;
> >         i++, internal++, orig_internal++)
> >      {
> > -      internal->sh_name      = BYTE_GET (shdrs[i].sh_name);
> > -      internal->sh_type      = BYTE_GET (shdrs[i].sh_type);
> > -      internal->sh_flags     = BYTE_GET (shdrs[i].sh_flags);
> > -      internal->sh_addr      = BYTE_GET (shdrs[i].sh_addr);
> > -      internal->sh_size      = BYTE_GET (shdrs[i].sh_size);
> > -      internal->sh_entsize   = BYTE_GET (shdrs[i].sh_entsize);
> > -      internal->sh_link      = BYTE_GET (shdrs[i].sh_link);
> > -      internal->sh_info      = BYTE_GET (shdrs[i].sh_info);
> > -      internal->sh_offset    = BYTE_GET (shdrs[i].sh_offset);
> > -      internal->sh_addralign = BYTE_GET (shdrs[i].sh_addralign);
> > +      BYTE_GET_SIZE (internal->sh_name, ptr, 4);
> > +      BYTE_GET_SIZE (internal->sh_type, ptr, 4);
> > +      BYTE_GET_SIZE (internal->sh_flags, ptr, elf_class_size);
> > +      BYTE_GET_SIZE (internal->sh_addr, ptr, elf_class_size);
> > +      BYTE_GET_SIZE (internal->sh_offset, ptr, elf_class_size);
> > +      BYTE_GET_SIZE (internal->sh_size, ptr, elf_class_size);
> > +      BYTE_GET_SIZE (internal->sh_link, ptr, 4);
> > +      BYTE_GET_SIZE (internal->sh_info, ptr, 4);
> > +      BYTE_GET_SIZE (internal->sh_addralign, ptr, elf_class_size);
> > +      BYTE_GET_SIZE (internal->sh_entsize, ptr, elf_class_size);
>
> Here is my main concern with this approach: This way you're open-coding
> the structure layout and field types of Elf32_Shdr / Elf64_Shdr (and at
> the same time the distinction between Xword, Addr, and Off is lost).
> While those clearly can't change, that's still at least very close to a
> no-go imo.

Since it is close to no-go to you,  Alan and Nick have no opinion,
I am dropping this patch set.

> I'm curious what others think.
>
> Jan



-- 
H.J.