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

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

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

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.

I'm curious what others think.

Jan