[PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE

[Fangrui Song]

On Fri, Jul 7, 2023 at 12:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.07.2023 07:51, Fangrui Song wrote:
> > On 2023-07-06, Jan Beulich wrote:
> >
> > Thanks for the review!
> > Uploaded PATCH v3 https://sourceware.org/pipermail/binutils/2023-July/128306.html
>
> It is perhaps worth considering to wait with sending a new version until
> the discussion on the earlier one has settled.

OK. I was used to other review systems where providing quick update
can allow other reviewers to skip the to-be-improved parts in the old
patch...
I guess in this case I could have waited a bit.

> >>> @@ -7940,6 +7947,9 @@ _bfd_elf_init_private_section_data (bfd *ibfd,
> >>>    elf_section_flags (osec) = (elf_section_flags (isec)
> >>>                           & (SHF_MASKOS | SHF_MASKPROC));
> >>>
> >>> +  if (get_elf_backend_data (ibfd)->elf_machine_code == EM_X86_64)
> >>> +    elf_section_flags (osec) = (elf_section_flags (isec) & ~SHF_X86_64_LARGE);
> >>
> >> What is this about? You're overwriting what the previous statement has
> >> written.
> >
> > This behavior is tested by large-sections-2.d: objcopy
> > --set-section-flags ... without "large" should drop SHF_X86_64_LARGE.
> > If SEC_ELF_LARGE is set ("large" is included), elf_fake_section will add back
> > SHF_X86_64_LARGE.
> >
> >
> > (
> > Note that we don't clear SHF_EXCLUDE (unfortunately part of
> > SHF_MASKPROC), so --set-section-flags without "exclude" doesn't drop
> > SHF_EXCLUDE.
> > rg 'SHF.*0x80000000' include/ has many occurrences. I have no idea how
> > to treat them yet...
> > )
>
> Which is part of my earlier comment: You should not undo what the
> immediately preceding line does. It also doesn't feel right to do the
> masking-off of SHF_X86_64_LARGE right here, as the function has (aiui)
> purposes beyond its use in the course of handling --set-section-flags.

Thanks for Alan's hints. Addressed in v4. The SHF_X86_64_LARGE bits
are all moved to bfd/elf64-x86-64.c in PATCH v4:
https://sourceware.org/pipermail/binutils/2023-July/128332.html

> >>> --- a/binutils/objcopy.c
> >>> +++ b/binutils/objcopy.c
> >>> @@ -797,6 +797,7 @@ parse_flags (const char *s)
> >>>        PARSE_FLAG ("contents", SEC_HAS_CONTENTS);
> >>>        PARSE_FLAG ("merge", SEC_MERGE);
> >>>        PARSE_FLAG ("strings", SEC_STRINGS);
> >>> +      PARSE_FLAG ("large", SEC_ELF_LARGE);
> >>>  #undef PARSE_FLAG
> >>>        else
> >>>     {
> >>> @@ -807,7 +808,7 @@ parse_flags (const char *s)
> >>>       copy[len] = '\0';
> >>>       non_fatal (_("unrecognized section flag `%s'"), copy);
> >>>       fatal (_("supported flags: %s"),
> >>> -            "alloc, load, noload, readonly, debug, code, data, rom, exclude, share, contents, merge, strings");
> >>> +            "alloc, load, noload, readonly, debug, code, data, rom, exclude, share, contents, merge, strings, large");
> >>>     }
> >>
> >> So what about someone specifying "large" for a target other the x86-64/ELF?
> >> Aiui there'll be no indication whatsoever that the flag specification didn't
> >> take any effect.
> >
> > Changed this to look like
> >
> > "..."
> > "share, contents, merge, strings, (ELF x86-64 specific) large"
> >
> > Ideally we should report an error for other targets, but I don't find a
> > convenient way to detect ELF x86-64... I think at the option parsing
> > time the target isn't known yet.
>
> The error can well be reported later, but potentially (and silently)
> affecting unrelated flags on other targets because of a wrong use of
> a command line option is a no-go imo.

OK, I managed to find check_new_section_flags. I have played with
different -O values and decided to
implement the error check under the condition `bfd_get_flavour (abfd)
== bfd_target_elf_flavour`.

objcopy --set-section-flags .data=alloc,large -O binary a.o  # fine
objcopy --set-section-flags .data=alloc,large -O elf32-i386 a.o  # error

+  /* Report a fatal error if 'large' is used with a non-x86-64 ELF target.
+     Suppress the error for non-ELF targets to allow -O binary and formats that
+     use the bit value SEC_ELF_LARGE for other purposes.  */
+  if ((flags & SEC_ELF_LARGE) != 0
+      && bfd_get_flavour (abfd) == bfd_target_elf_flavour
+      && get_elf_backend_data (abfd)->elf_machine_code != EM_X86_64)
+    {
+      fatal (_ ("%s[%s]: 'large' flag is ELF x86-64 specific"),
+     bfd_get_filename (abfd), secname);
+      flags &= ~SEC_ELF_LARGE;
+    }



> >>> --- a/include/elf/common.h
> >>> +++ b/include/elf/common.h
> >>> @@ -588,6 +588,8 @@
> >>>
> >>>  #define SHF_GNU_MBIND      0x01000000      /* Mbind section.  */
> >>>
> >>> +#define SHF_X86_64_LARGE 0x10000000
> >>
> >> elf/x86-64.h already has such a #define, and that's imo the only place
> >> where it should live.
> >
> > This is a bit unfortunate, but bfd/elf.c only includes elf/common.h.
> > I think bfd/elf.c likely cannot include target-specific headers.
> > elf/common.h already has machine-specific NT_*_* and GNU_PROPERTY_X86_*, so I
> > hope that defining SHF_X86_64_LARGE isn't too bad.
>
> I'm afraid I have to defer to Nick or Alan here; personally I wouldn't
> approve a change adding such a duplicate definition. To me it suggests
> that use of such constants in bfd/elf.c isn't intended, and hence things
> need arranging differently (which may mean a larger overall change is
> needed, to properly abstract the handling of arch-specific flags).
>
> Jan

Removed all x86-64-specific extra code from bfd/elf.c in PATCH v4.

Playing with objcopy.c:parse_flags and BFD hooks
bfd_elf64_bfd_copy_private_section_data/elf_backend_section_flags/elf_backend_fake_sections
makes me feel that the UI of --set-section-flags isn't convenient.
Filed https://sourceware.org/bugzilla/show_bug.cgi?id=30623 ("objcopy
--set-section-flags: support toggling a flag").


-- 
宋方睿