[PATCH 1/5] readelf: Consolidate get_[32|64]bit_section_headers
H.J. Lu
hjl.tools@gmail.com
Fri Jul 17 08:32:23 GMT 2026
On Fri, Jul 10, 2026 at 9:41 PM Jan Beulich <jbeulich@suse.com> 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 <hjl.tools@gmail.com>
> > ---
> > 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.
More information about the Binutils
mailing list