public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Fangrui Song <maskray@google.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: binutils@sourceware.org, Alan Modra <amodra@gmail.com>
Subject: Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
Date: Fri, 7 Jul 2023 22:59:36 -0700	[thread overview]
Message-ID: <CAFP8O3+wuV91_qbURmgDwD1dQt3v-pMvNqo87aXJcypK9ta-wg@mail.gmail.com> (raw)
In-Reply-To: <82f0bb78-9a64-c614-4211-c9b05ca4719d@suse.com>

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


-- 
宋方睿

      parent reply	other threads:[~2023-07-08  5:59 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-28 23:16 Fangrui Song
2023-07-06 12:15 ` Jan Beulich
2023-07-07  5:51   ` Fangrui Song
2023-07-07  7:27     ` Jan Beulich
2023-07-07  8:30       ` Alan Modra
2023-07-08  5:59       ` Fangrui Song [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFP8O3+wuV91_qbURmgDwD1dQt3v-pMvNqo87aXJcypK9ta-wg@mail.gmail.com \
    --to=maskray@google.com \
    --cc=amodra@gmail.com \
    --cc=binutils@sourceware.org \
    --cc=jbeulich@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).