* [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
@ 2023-06-28 23:16 Fangrui Song
2023-07-06 12:15 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Fangrui Song @ 2023-06-28 23:16 UTC (permalink / raw)
To: binutils; +Cc: 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.
bfd/
* bfd-in2.h: Define SEC_ELF_LARGE.
* elf.c (_bfd_elf_make_section_from_shdr): Check SHF_X86_64_LARGE.
(elf_fake_sections): Check SEC_ELF_LARGE.
(_bfd_elf_init_private_section_data): Drop SHF_X86_64_LARGE for
x86-64.
binutils/
* NEWS: Mention the new feature for objcopy.
* doc/binutils.texi: Mention "large".
* objcopy.c (parse_flags): Parse "large".
* 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-2.d: New.
* testsuite/binutils-all/x86-64/large-sections-2.s: New.
include/
* elf/common.h: Define SHF_X86_64_LARGE to be used by elf.c.
--
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
---
bfd/bfd-in2.h | 3 +++
bfd/elf.c | 10 ++++++++++
binutils/NEWS | 3 +++
binutils/doc/binutils.texi | 15 ++++++++-------
binutils/objcopy.c | 3 ++-
.../binutils-all/x86-64/large-sections-2.d | 15 +++++++++++++++
.../binutils-all/x86-64/large-sections-2.s | 4 ++++
.../binutils-all/x86-64/large-sections.d | 14 ++++++++++++++
.../binutils-all/x86-64/large-sections.s | 8 ++++++++
include/elf/common.h | 2 ++
10 files changed, 69 insertions(+), 8 deletions(-)
create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections-2.d
create mode 100644 binutils/testsuite/binutils-all/x86-64/large-sections-2.s
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 c4fa0c1f9bf..17f3da907e0 100644
--- 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
+
/* All symbols, sizes and relocations in this section are octets
instead of bytes. Required for DWARF debug sections as DWARF
information is organized in octets, not bytes. */
diff --git a/bfd/elf.c b/bfd/elf.c
index 8f6d7d1adba..112e8c8e5ca 100644
--- 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;
+
switch (elf_elfheader (abfd)->e_ident[EI_OSABI])
{
/* FIXME: We should not recognize SHF_GNU_MBIND for ELFOSABI_NONE,
@@ -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;
/* If the section has relocs, set up a section header for the
SHT_REL[A] section. If two relocation sections are required for
@@ -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);
+
/* Copy sh_info from input for mbind section. */
if ((elf_tdata (ibfd)->has_gnu_osabi & elf_gnu_osabi_mbind) != 0
&& elf_section_flags (isec) & SHF_GNU_MBIND)
diff --git a/binutils/NEWS b/binutils/NEWS
index 00ef7c43610..1b61cce92e3 100644
--- a/binutils/NEWS
+++ b/binutils/NEWS
@@ -13,6 +13,9 @@
updates the ABI in an incompatible way: this includes removal of
sframe_get_funcdesc_with_addr API, change in the behavior of
sframe_fre_get_ra_offset and sframe_fre_get_fp_offset APIs.
+
+* objcopy's --set-section-flags now support "large" to set SHF_X86_64_LARGE
+ for ELF x86-64 objects.
Changes in 2.40:
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 8c14d1121d9..a497f9087a3 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -1740,13 +1740,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 414007780a8..40496f8f792 100644
--- 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");
}
s = snext;
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-2.s b/binutils/testsuite/binutils-all/x86-64/large-sections-2.s
new file mode 100644
index 00000000000..6f31aa93701
--- /dev/null
+++ b/binutils/testsuite/binutils-all/x86-64/large-sections-2.s
@@ -0,0 +1,4 @@
+ .section .text, "axl"
+ nop
+ .section .data, "awl"
+ .byte 1
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
diff --git a/include/elf/common.h b/include/elf/common.h
index ffa6b60bd2b..1397d60402e 100644
--- 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
+
/* Compression types. */
#define ELFCOMPRESS_ZLIB 1 /* Compressed with zlib. */
#define ELFCOMPRESS_ZSTD 2 /* Compressed with zstd */
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
2023-06-28 23:16 [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE Fangrui Song
@ 2023-07-06 12:15 ` Jan Beulich
2023-07-07 5:51 ` Fangrui Song
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2023-07-06 12:15 UTC (permalink / raw)
To: Fangrui Song; +Cc: binutils
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
2023-07-06 12:15 ` Jan Beulich
@ 2023-07-07 5:51 ` Fangrui Song
2023-07-07 7:27 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Fangrui Song @ 2023-07-07 5:51 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
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
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2023-07-07 7:27 UTC (permalink / raw)
To: Fangrui Song; +Cc: binutils
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
2023-07-07 7:27 ` Jan Beulich
@ 2023-07-07 8:30 ` Alan Modra
2023-07-08 5:59 ` Fangrui Song
1 sibling, 0 replies; 6+ messages in thread
From: Alan Modra @ 2023-07-07 8:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Fangrui Song, binutils
On Fri, Jul 07, 2023 at 09:27:18AM +0200, Jan Beulich via Binutils wrote:
> On 07.07.2023 07:51, Fangrui Song wrote:
> > 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).
See https://sourceware.org/pipermail/binutils/2023-July/128311.html
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE
2023-07-07 7:27 ` Jan Beulich
2023-07-07 8:30 ` Alan Modra
@ 2023-07-08 5:59 ` Fangrui Song
1 sibling, 0 replies; 6+ messages in thread
From: Fangrui Song @ 2023-07-08 5:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils, Alan Modra
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").
--
宋方睿
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-08 5:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-28 23:16 [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE 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 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).