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

[Jan Beulich <jbeulich@suse.com>]

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.

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

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

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

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

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

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