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

[Fangrui Song <maskray@google.com>]

On 2023-07-06, Jan Beulich wrote:

Thanks for the review!
Uploaded PATCH v3 https://sourceware.org/pipermail/binutils/2023-July/128306.html

>On 29.06.2023 01:16, Fangrui Song via Binutils wrote:
>> --- a/bfd/bfd-in2.h
>> +++ b/bfd/bfd-in2.h
>> @@ -633,6 +633,9 @@ typedef struct bfd_section
>>    /* This section contains vliw code.  This is for Toshiba MeP only.  */
>>  #define SEC_MEP_VLIW               0x20000000
>>
>> +  /* This section has the SHF_X86_64_LARGE flag.  This is ELF x86-64 only.  */
>> +#define SEC_ELF_LARGE              0x20000000
>
>Is this taking the same value as SEC_MEP_VLIW and SEC_TIC54X_CLINK
>deliberate? If so, the comment below on binutils/objcopy.c is yet
>more relevant than I first thought, as the flag could then
>unintentionally become set from (mis)using "large" there.

Yes, updated the description in v3.

>Also this is a generated file; you want to primarily edit section.c
>if I recall correctly.

Ack. `make -C $build/bfd headers` updates bfd-in2.h. Changed.

>> --- a/bfd/elf.c
>> +++ b/bfd/elf.c
>> @@ -1034,6 +1034,10 @@ _bfd_elf_make_section_from_shdr (bfd *abfd,
>>    if ((hdr->sh_flags & SHF_EXCLUDE) != 0)
>>      flags |= SEC_EXCLUDE;
>>
>> +  if (get_elf_backend_data (abfd)->elf_machine_code == EM_X86_64)
>> +    if ((hdr->sh_flags & SHF_X86_64_LARGE) != 0)
>> +      flags |= SEC_ELF_LARGE;
>
>Such two if()-s want to be joined to one, imo. Also style-wise it
>would be nice if this and ...

Updated.

>> @@ -3351,6 +3355,9 @@ elf_fake_sections (bfd *abfd, asection *asect, void *fsarg)
>>      }
>>    if ((asect->flags & (SEC_GROUP | SEC_EXCLUDE)) == SEC_EXCLUDE)
>>      this_hdr->sh_flags |= SHF_EXCLUDE;
>> +  if (asect->flags & SEC_ELF_LARGE)
>> +    if (get_elf_backend_data (abfd)->elf_machine_code == EM_X86_64)
>> +      this_hdr->sh_flags |= SHF_X86_64_LARGE;
>
>... this could match (in order of checks as well as in whether or not
>!= 0 is used on the result of &.

Updated.

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

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

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

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.