public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Fangrui Song <maskray@google.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
Date: Fri, 7 Jul 2023 09:27:18 +0200	[thread overview]
Message-ID: <82f0bb78-9a64-c614-4211-c9b05ca4719d@suse.com> (raw)
In-Reply-To: <20230707055139.3ix2gotioz6dr2b3@google.com>

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.

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

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

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

  reply	other threads:[~2023-07-07  7:27 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 [this message]
2023-07-07  8:30       ` Alan Modra
2023-07-08  5:59       ` Fangrui Song

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=82f0bb78-9a64-c614-4211-c9b05ca4719d@suse.com \
    --to=jbeulich@suse.com \
    --cc=binutils@sourceware.org \
    --cc=maskray@google.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).