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

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

[Fangrui Song]

For example, objcopy --set-section-flags .data=alloc,large will add
SHF_X86_64_LARGE to the .data section.  Omitting "large" will drop the
SHF_X86_64_LARGE flag.

The bfd_section flag is named generically, SEC_ELF_LARGE, in case other
processors want to follow SHF_X86_64_LARGE.  SEC_ELF_LARGE has the same value
as SEC_TIC54X_CLINK and SEC_MEP_VLIW.

bfd/
    * section.c: Define SEC_ELF_LARGE.
    * bfd-in2.h: Regenerate.
    * elf64-x86-64.c (elf_x86_64_section_flags, elf_x86_64_fake_sections,
    elf_x86_64_copy_private_section_data): New.

binutils/
    * NEWS: Mention the new feature for objcopy.
    * doc/binutils.texi: Mention "large".
    * objcopy.c (parse_flags): Parse "large".
    (check_new_section_flags): Error if "large" is used with a
    non-x86-64 ELF target.
    * testsuite/binutils-all/x86-64/large-sections.d: New.
    * testsuite/binutils-all/x86-64/large-sections.s: New.
    * testsuite/binutils-all/x86-64/large-sections-i386.d: New.
    * testsuite/binutils-all/x86-64/large-sections-2.d: New.
    * testsuite/binutils-all/x86-64/large-sections-2-x32.d: New.

--
Changes from v1:

* Add an entry to binutils/NEWS
* Adjust doc/binutils.texi wording
* Guard a SEC_ELF_LARGE branch with EM_X86_64 check

Changes from v2:

* Address Jan's comments https://sourceware.org/pipermail/binutils/2023-July/128303.html

Changes from v3:

* Address Alan's comments. Define elf_x86_64_section_flags, elf_x86_64_fake_sections,
  elf_x86_64_copy_private_section_data instead of adding x86-64 flags to generic code.
* Add an error when 'large' is used by other targets (e.g. EM_386,
  EM_AARCH64).
---
 bfd/bfd-in2.h                                 |  3 ++
 bfd/elf64-x86-64.c                            | 36 +++++++++++++++++++
 bfd/section.c                                 |  3 ++
 binutils/NEWS                                 |  3 ++
 binutils/doc/binutils.texi                    | 15 ++++----
 binutils/objcopy.c                            | 22 ++++++++++--
 .../x86-64/large-sections-2-x32.d             | 15 ++++++++
 .../binutils-all/x86-64/large-sections-2.d    | 15 ++++++++
 .../binutils-all/x86-64/large-sections-i386.d |  6 ++++
 .../binutils-all/x86-64/large-sections.d      | 14 ++++++++
 .../binutils-all/x86-64/large-sections.s      |  8 +++++
 11 files changed, 130 insertions(+), 10 deletions(-)
 create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections-2-x32.d
 create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections-2.d
 create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections-i386.d
 create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections.d
 create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections.s

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index b34c8ef9fc9..1b9a801966c 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -625,6 +625,9 @@ typedef struct bfd_section
      TMS320C54X only.  */
 #define SEC_TIC54X_BLOCK           0x10000000
 
+  /* This section has the SHF_X86_64_LARGE flag.  This is ELF x86-64 only.  */
+#define SEC_ELF_LARGE              0x10000000
+
   /* Conditionally link this section; do not link if there are no
      references found to any symbol in the section.  This is for TI
      TMS320C54X only.  */
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index f926464d812..2d7092c42c0 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -5281,6 +5281,36 @@ elf_x86_64_merge_symbol (struct elf_link_hash_entry *h,
   return true;
 }
 
+static bool
+elf_x86_64_section_flags (const Elf_Internal_Shdr *hdr)
+{
+  if ((hdr->sh_flags & SHF_X86_64_LARGE) != 0)
+    hdr->bfd_section->flags |= SEC_ELF_LARGE;
+
+  return true;
+}
+
+static bool
+elf_x86_64_fake_sections (bfd *abfd ATTRIBUTE_UNUSED,
+			  Elf_Internal_Shdr *hdr, asection *sec)
+{
+  if (sec->flags & SEC_ELF_LARGE)
+    hdr->sh_flags |= SHF_X86_64_LARGE;
+
+  return true;
+}
+
+static bool
+elf_x86_64_copy_private_section_data (bfd *ibfd ATTRIBUTE_UNUSED,
+				      asection *isec,
+				      bfd *obfd ATTRIBUTE_UNUSED,
+				      asection *osec) {
+  /* objcopy --set-section-flags without "large" drops SHF_X86_64_LARGE.  */
+  elf_section_flags (osec) = (elf_section_flags (isec) & ~SHF_X86_64_LARGE);
+
+  return true;
+}
+
 static int
 elf_x86_64_additional_program_headers (bfd *abfd,
 				       struct bfd_link_info *info ATTRIBUTE_UNUSED)
@@ -5408,6 +5438,8 @@ elf_x86_64_special_sections[]=
 
 #define elf_info_to_howto		    elf_x86_64_info_to_howto
 
+#define bfd_elf64_bfd_copy_private_section_data \
+  elf_x86_64_copy_private_section_data
 #define bfd_elf64_bfd_reloc_type_lookup	    elf_x86_64_reloc_type_lookup
 #define bfd_elf64_bfd_reloc_name_lookup \
   elf_x86_64_reloc_name_lookup
@@ -5448,6 +5480,8 @@ elf_x86_64_special_sections[]=
   elf_x86_64_merge_symbol
 #define elf_backend_special_sections \
   elf_x86_64_special_sections
+#define elf_backend_section_flags	    elf_x86_64_section_flags
+#define elf_backend_fake_sections	    elf_x86_64_fake_sections
 #define elf_backend_additional_program_headers \
   elf_x86_64_additional_program_headers
 #define elf_backend_setup_gnu_properties \
@@ -5564,6 +5598,8 @@ elf64_x86_64_copy_solaris_special_section_fields (const bfd *ibfd ATTRIBUTE_UNUS
 #undef	ELF_TARGET_OS
 #undef	ELF_OSABI
 
+#define bfd_elf32_bfd_copy_private_section_data \
+  elf_x86_64_copy_private_section_data
 #define bfd_elf32_bfd_reloc_type_lookup	\
   elf_x86_64_reloc_type_lookup
 #define bfd_elf32_bfd_reloc_name_lookup \
diff --git a/bfd/section.c b/bfd/section.c
index 73770294e56..e9af59dfd15 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -359,6 +359,9 @@ CODE_FRAGMENT
 .     TMS320C54X only.  *}
 .#define SEC_TIC54X_BLOCK           0x10000000
 .
+.  {* This section has the SHF_X86_64_LARGE flag.  This is ELF x86-64 only.  *}
+.#define SEC_ELF_LARGE              0x10000000
+.
 .  {* Conditionally link this section; do not link if there are no
 .     references found to any symbol in the section.  This is for TI
 .     TMS320C54X only.  *}
diff --git a/binutils/NEWS b/binutils/NEWS
index 563062c6904..cad877b943a 100644
--- a/binutils/NEWS
+++ b/binutils/NEWS
@@ -1,5 +1,8 @@
 -*- text -*-
 
+* objcopy's --set-section-flags now support "large" to set SHF_X86_64_LARGE
+  for ELF x86-64 objects.
+
 Changes in 2.41:
 
 * The MIPS port now supports the Sony Interactive Entertainment Allegrex
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 8314cb57562..309bedf6110 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -1745,13 +1745,14 @@ Set the flags for any sections matching @var{sectionpattern}.  The
 @var{flags} argument is a comma separated string of flag names.  The
 recognized names are @samp{alloc}, @samp{contents}, @samp{load},
 @samp{noload}, @samp{readonly}, @samp{code}, @samp{data}, @samp{rom},
-@samp{exclude}, @samp{share}, and @samp{debug}.  You can set the
-@samp{contents} flag for a section which does not have contents, but it
-is not meaningful to clear the @samp{contents} flag of a section which
-does have contents--just remove the section instead.  Not all flags are
-meaningful for all object file formats.  In particular the
-@samp{share} flag is only meaningful for COFF format files and not for
-ELF format files.
+@samp{exclude}, @samp{share}, @samp{debug}, and @samp{large}.
+You can set the @samp{contents} flag for a section which does not have
+contents, but it is not meaningful to clear the @samp{contents} flag of a
+section which does have contents--just remove the section instead.  Not all
+flags are meaningful for all object file formats.  In particular the
+@samp{share} flag is only meaningful for COFF format files and not for ELF
+format files.  The ELF x86-64 specific flag @samp{large} corresponds to
+SHF_X86_64_LARGE.
 
 @item --set-section-alignment @var{sectionpattern}=@var{align}
 Set the alignment for any sections matching @var{sectionpattern}.
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 3569b890c7d..c931f67a224 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -803,6 +803,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
 	{
@@ -812,8 +813,10 @@ parse_flags (const char *s)
 	  strncpy (copy, s, len);
 	  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");
+	  fatal (_ ("supported flags: %s"),
+		 "alloc, load, noload, readonly, debug, code, data, rom, "
+		 "exclude, contents, merge, strings, (COFF specific) share, "
+		 "(ELF x86-64 specific) large");
 	}
 
       s = snext;
@@ -2618,7 +2621,7 @@ merge_gnu_build_notes (bfd *          abfd,
 }
 
 static flagword
-check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
+check_new_section_flags (flagword flags, bfd *abfd, const char * secname)
 {
   /* Only set the SEC_COFF_SHARED flag on COFF files.
      The same bit value is used by ELF targets to indicate
@@ -2631,6 +2634,19 @@ check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
 		 bfd_get_filename (abfd), secname);
       flags &= ~ SEC_COFF_SHARED;
     }
+
+  /* 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;
+    }
+
   return flags;
 }
 
diff --git a/binutils/testsuite/binutils-all/x86-64/large-sections-2-x32.d b/binutils/testsuite/binutils-all/x86-64/large-sections-2-x32.d
new file mode 100644
index 00000000000..fb28d08e6b7
--- /dev/null
+++ b/binutils/testsuite/binutils-all/x86-64/large-sections-2-x32.d
@@ -0,0 +1,15 @@
+#source: large-sections.s
+#PROG: objcopy
+#as: --x32
+#objcopy: --set-section-flags .ldata=alloc
+#readelf: -S -W
+
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AX[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#...
+  \[[ 0-9]+\] \.ltext.*[ \t]+PROGBITS[ \t0-9a-f]+AXl[ \t]+.*
+#...
+  \[[ 0-9]+\] \.ldata.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#pass
diff --git a/binutils/testsuite/binutils-all/x86-64/large-sections-2.d b/binutils/testsuite/binutils-all/x86-64/large-sections-2.d
new file mode 100644
index 00000000000..29ace42cc9e
--- /dev/null
+++ b/binutils/testsuite/binutils-all/x86-64/large-sections-2.d
@@ -0,0 +1,15 @@
+#source: large-sections.s
+#PROG: objcopy
+#as: --64
+#objcopy: --set-section-flags .ldata=alloc
+#readelf: -S -W
+
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AX[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#...
+  \[[ 0-9]+\] \.ltext.*[ \t]+PROGBITS[ \t0-9a-f]+AXl[ \t]+.*
+#...
+  \[[ 0-9]+\] \.ldata.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#pass
diff --git a/binutils/testsuite/binutils-all/x86-64/large-sections-i386.d b/binutils/testsuite/binutils-all/x86-64/large-sections-i386.d
new file mode 100644
index 00000000000..b06c7c993c2
--- /dev/null
+++ b/binutils/testsuite/binutils-all/x86-64/large-sections-i386.d
@@ -0,0 +1,6 @@
+#source: large-sections.s
+#PROG: objcopy
+#as: --64
+#objcopy: -O elf32-i386 --set-section-flags .data=alloc,large
+#target: x86_64-*-linux*
+#error: \A[^[]*\[.data\]: 'large' flag is ELF x86-64 specific
diff --git a/binutils/testsuite/binutils-all/x86-64/large-sections.d b/binutils/testsuite/binutils-all/x86-64/large-sections.d
new file mode 100644
index 00000000000..5d945e46ba3
--- /dev/null
+++ b/binutils/testsuite/binutils-all/x86-64/large-sections.d
@@ -0,0 +1,14 @@
+#PROG: objcopy
+#as: --64
+#objcopy: --set-section-flags .text=alloc,readonly,code,large --set-section-flags .data=alloc,large
+#readelf: -S -W
+
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AXl[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WAl[ \t]+.*
+#...
+  \[[ 0-9]+\] \.ltext.*[ \t]+PROGBITS[ \t0-9a-f]+AXl[ \t]+.*
+#...
+  \[[ 0-9]+\] \.ldata.*[ \t]+PROGBITS[ \t0-9a-f]+WAl[ \t]+.*
+#pass
diff --git a/binutils/testsuite/binutils-all/x86-64/large-sections.s b/binutils/testsuite/binutils-all/x86-64/large-sections.s
new file mode 100644
index 00000000000..072e456a1ed
--- /dev/null
+++ b/binutils/testsuite/binutils-all/x86-64/large-sections.s
@@ -0,0 +1,8 @@
+	.section .text, "ax"
+	nop
+	.section .data, "aw"
+	.byte 1
+	.section .ltext, "axl"
+	nop
+	.section .ldata, "awl"
+	.byte 1
-- 
2.41.0.255.g8b1d071c50-goog

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

[Alan Modra]

On Fri, Jul 07, 2023 at 10:30:35PM -0700, Fangrui Song via Binutils wrote:
> +static bool
> +elf_x86_64_copy_private_section_data (bfd *ibfd ATTRIBUTE_UNUSED,
> +				      asection *isec,
> +				      bfd *obfd ATTRIBUTE_UNUSED,
> +				      asection *osec) {
> +  /* objcopy --set-section-flags without "large" drops SHF_X86_64_LARGE.  */
> +  elf_section_flags (osec) = (elf_section_flags (isec) & ~SHF_X86_64_LARGE);
> +
> +  return true;
> +}
> +

Unlike the other two functions, this one replaces the standard
_bfd_elf_copy_private_section_data.  You'll want to call it, and
distinguish between a call from objcopy.c and a call from ldwrite.c.
I think you're OK accessing elf_section_flags for osec without first
checking for an elf bfd, because bfd_copy_private_section_data invokes
the obfd xvec function.  Like this:

static bool
elf_x86_64_copy_private_section_data (bfd *ibfd, asection *isec,
				      bfd *obfd, asection *osec)
{
  if (!_bfd_elf_copy_private_section_data (ibfd, isec, obfd, osec))
    return false;

  /* objcopy --set-section-flags without "large" drops SHF_X86_64_LARGE.  */
  if (ibfd != obfd)
    elf_section_flags (osec) &= ~SHF_X86_64_LARGE;

  return true;
}

The patch looks OK to me with the above replacement.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Fangrui Song]

On 2023-07-09, Alan Modra wrote:
>On Fri, Jul 07, 2023 at 10:30:35PM -0700, Fangrui Song via Binutils wrote:
>> +static bool
>> +elf_x86_64_copy_private_section_data (bfd *ibfd ATTRIBUTE_UNUSED,
>> +				      asection *isec,
>> +				      bfd *obfd ATTRIBUTE_UNUSED,
>> +				      asection *osec) {
>> +  /* objcopy --set-section-flags without "large" drops SHF_X86_64_LARGE.  */
>> +  elf_section_flags (osec) = (elf_section_flags (isec) & ~SHF_X86_64_LARGE);
>> +
>> +  return true;
>> +}
>> +
>
>Unlike the other two functions, this one replaces the standard
>_bfd_elf_copy_private_section_data.  You'll want to call it, and
>distinguish between a call from objcopy.c and a call from ldwrite.c.
>I think you're OK accessing elf_section_flags for osec without first
>checking for an elf bfd, because bfd_copy_private_section_data invokes
>the obfd xvec function.  Like this:
>
>static bool
>elf_x86_64_copy_private_section_data (bfd *ibfd, asection *isec,
>				      bfd *obfd, asection *osec)
>{
>  if (!_bfd_elf_copy_private_section_data (ibfd, isec, obfd, osec))
>    return false;
>
>  /* objcopy --set-section-flags without "large" drops SHF_X86_64_LARGE.  */
>  if (ibfd != obfd)
>    elf_section_flags (osec) &= ~SHF_X86_64_LARGE;
>
>  return true;
>}
>
>The patch looks OK to me with the above replacement.

Thank you.  I see that we need to call _bfd_elf_copy_private_section_data  first to
respect a few other properties including sh_entsize and "readonly".

For objcopy -I binary -O elf64-x86-64 pr25749-2.c a.o,
ibfd may not be an ELF bfd, but obfd is guaranteed to be an ELF bfd.

ld/ldwrite.c calls bfd_copy_private_section_data with ibfd == obfd.

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

[Jan Beulich]

On 08.07.2023 07:30, Fangrui Song via Binutils wrote:
> --- a/bfd/section.c
> +++ b/bfd/section.c
> @@ -359,6 +359,9 @@ CODE_FRAGMENT
>  .     TMS320C54X only.  *}
>  .#define SEC_TIC54X_BLOCK           0x10000000
>  .
> +.  {* This section has the SHF_X86_64_LARGE flag.  This is ELF x86-64 only.  *}
> +.#define SEC_ELF_LARGE              0x10000000
> +.
>  .  {* Conditionally link this section; do not link if there are no
>  .     references found to any symbol in the section.  This is for TI
>  .     TMS320C54X only.  *}
> @@ -2618,7 +2621,7 @@ merge_gnu_build_notes (bfd *          abfd,
>  }
>  
>  static flagword
> -check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
> +check_new_section_flags (flagword flags, bfd *abfd, const char * secname)

Nit: Stray (and inconsistent) change?

> @@ -2631,6 +2634,19 @@ check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
>  		 bfd_get_filename (abfd), secname);
>        flags &= ~ SEC_COFF_SHARED;
>      }
> +
> +  /* 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)

DYM

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

?

Jan

> +    {
> +      fatal (_ ("%s[%s]: 'large' flag is ELF x86-64 specific"),
> +	     bfd_get_filename (abfd), secname);
> +      flags &= ~SEC_ELF_LARGE;
> +    }
> +
>    return flags;
>  }
>  

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

[Fangrui Song]

On 2023-07-10, Jan Beulich wrote:
>On 08.07.2023 07:30, Fangrui Song via Binutils wrote:
>> --- a/bfd/section.c
>> +++ b/bfd/section.c
>> @@ -359,6 +359,9 @@ CODE_FRAGMENT
>>  .     TMS320C54X only.  *}
>>  .#define SEC_TIC54X_BLOCK           0x10000000
>>  .
>> +.  {* This section has the SHF_X86_64_LARGE flag.  This is ELF x86-64 only.  *}
>> +.#define SEC_ELF_LARGE              0x10000000
>> +.
>>  .  {* Conditionally link this section; do not link if there are no
>>  .     references found to any symbol in the section.  This is for TI
>>  .     TMS320C54X only.  *}
>> @@ -2618,7 +2621,7 @@ merge_gnu_build_notes (bfd *          abfd,
>>  }
>>
>>  static flagword
>> -check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
>> +check_new_section_flags (flagword flags, bfd *abfd, const char * secname)
>
>Nit: Stray (and inconsistent) change?

Sorry, this was an unneeded change.

I guess I formatted this line either in editor or via .clang-format but
I reverted `const char *secname` but not `bfd *abfd`...

>> @@ -2631,6 +2634,19 @@ check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
>>  		 bfd_get_filename (abfd), secname);
>>        flags &= ~ SEC_COFF_SHARED;
>>      }
>> +
>> +  /* 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)
>
>DYM
>
>  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))
>
>?
>
>Jan

I do mean that this check is fired only when the output bfd (abfd) is
ELF and the architecture is different from x86-64.

The bit value SEC_ELF_LARGE is shared with other object file formats
(COFF SEC_TIC54X_BLOCK; I don't know what this target is).
If TMS320C54X has a section with the SEC_TIC54X_BLOCK flag, we
don't want to report a fatal error.

In addition, abfd may specify a non-ELF bfd, e.g.,

     objcopy --set-section-flags .data=alloc,large -I elf64-x86-64 -O binary a.o a.bin

I assume that this case should receive no warning/error, just like how
many other flags like 'load', 'noload' behave today.

>> +    {
>> +      fatal (_ ("%s[%s]: 'large' flag is ELF x86-64 specific"),
>> +	     bfd_get_filename (abfd), secname);
>> +      flags &= ~SEC_ELF_LARGE;
>> +    }
>> +
>>    return flags;
>>  }
>>
>
>

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

[Jan Beulich]

On 10.07.2023 20:07, Fangrui Song wrote:
> On 2023-07-10, Jan Beulich wrote:
>> On 08.07.2023 07:30, Fangrui Song via Binutils wrote:
>>> @@ -2631,6 +2634,19 @@ check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
>>>  		 bfd_get_filename (abfd), secname);
>>>        flags &= ~ SEC_COFF_SHARED;
>>>      }
>>> +
>>> +  /* 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)
>>
>> DYM
>>
>>  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))
>>
>> ?
> 
> I do mean that this check is fired only when the output bfd (abfd) is
> ELF and the architecture is different from x86-64.
> 
> The bit value SEC_ELF_LARGE is shared with other object file formats
> (COFF SEC_TIC54X_BLOCK; I don't know what this target is).
> If TMS320C54X has a section with the SEC_TIC54X_BLOCK flag, we
> don't want to report a fatal error.

Hmm, I think I see what you mean. May I then suggest to re-order like
this:

  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour
      && (flags & SEC_ELF_LARGE) != 0
      && get_elf_backend_data (abfd)->elf_machine_code != EM_X86_64)

?

Jan

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

[Fangrui Song]

On Wed, Jul 12, 2023 at 7:32 AM Jan Beulich via Binutils
<binutils@sourceware.org> wrote:
>
> On 10.07.2023 20:07, Fangrui Song wrote:
> > On 2023-07-10, Jan Beulich wrote:
> >> On 08.07.2023 07:30, Fangrui Song via Binutils wrote:
> >>> @@ -2631,6 +2634,19 @@ check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
> >>>              bfd_get_filename (abfd), secname);
> >>>        flags &= ~ SEC_COFF_SHARED;
> >>>      }
> >>> +
> >>> +  /* 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)
> >>
> >> DYM
> >>
> >>  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))
> >>
> >> ?
> >
> > I do mean that this check is fired only when the output bfd (abfd) is
> > ELF and the architecture is different from x86-64.
> >
> > The bit value SEC_ELF_LARGE is shared with other object file formats
> > (COFF SEC_TIC54X_BLOCK; I don't know what this target is).
> > If TMS320C54X has a section with the SEC_TIC54X_BLOCK flag, we
> > don't want to report a fatal error.
>
> Hmm, I think I see what you mean. May I then suggest to re-order like
> this:
>
>   if (bfd_get_flavour (abfd) == bfd_target_elf_flavour
>       && (flags & SEC_ELF_LARGE) != 0
>       && get_elf_backend_data (abfd)->elf_machine_code != EM_X86_64)
>
> ?
>
> Jan

Yes, I think we can do this, but the current order likely leads to
better performance (does not matter, though)
since `flags` is a variable that's immediately available. and matches
the style of  flags & SEC_COFF_SHARED.
That said, I do not insist that we insist on doing this.  Feel free to
change if you feel strongly:)