Re: [PATCH] varasm: Fix up __patchable_function_entries handling
On 12/16/20 6:47 AM, Jakub Jelinek wrote: > On Wed, Dec 16, 2020 at 05:36:04AM -0800, H.J. Lu wrote: >> gas is correct. >> >>> comdat or for retain), then we should do something like the following >>> untested change, but if it is gas bug, it should be fixed there. >>> >>> 2020-12-15 Jakub Jelinek >>> >>> * varasm.c (default_elf_asm_named_section): Always force >>> section flags even for sections with SECTION_LINK_ORDER flag. >>> >> LGTM. But I can't approve it. > Bootstrapped/regtested on x86_64-linux and i686-linux successfully. > Ok for trunk? Yes. I probably wasn't clear about that yesterday ;-) jeff
Re: [PATCH] varasm: Fix up __patchable_function_entries handling
On Wed, Dec 16, 2020 at 05:36:04AM -0800, H.J. Lu wrote: > gas is correct. > > > comdat or for retain), then we should do something like the following > > untested change, but if it is gas bug, it should be fixed there. > > > > 2020-12-15 Jakub Jelinek > > > > * varasm.c (default_elf_asm_named_section): Always force > > section flags even for sections with SECTION_LINK_ORDER flag. > > > LGTM. But I can't approve it. Bootstrapped/regtested on x86_64-linux and i686-linux successfully. Ok for trunk? Jakub
Re: [PATCH] varasm: Fix up __patchable_function_entries handling
On Wed, Dec 16, 2020 at 5:05 AM Jakub Jelinek wrote:
>
> On Wed, Dec 02, 2020 at 05:15:21AM -0800, H.J. Lu via Gcc-patches wrote:
> > gcc/
> >
> > PR middle-end/93195
> > PR middle-end/93197
> > * configure.ac (HAVE_GAS_SECTION_LINK_ORDER): New. Define 1 if
> > the assembler supports the section flag 'o' for specifying
> > section with link-order.
> > * output.h (SECTION_LINK_ORDER): New. Defined to 0x800.
> > (SECTION_MACH_DEP): Changed from 0x800 to 0x1000.
> > * targhooks.c (default_print_patchable_function_entry): Pass
> > SECTION_LINK_ORDER to switch_to_section if the section flag 'o'
> > works. Pass current_function_decl to switch_to_section.
> > * varasm.c (default_elf_asm_named_section): Use 'o' flag for
> > SECTION_LINK_ORDER if assembler supports it.
> > * config.in: Regenerated.
> > * configure: Likewise.
>
> Dunno if it is an assembler bug or gcc bug, but this SECTION_LINK_ORDER
> stuff doesn't seem to work properly.
>
> If I compile:
> static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
> __attribute__((patchable_function_entry(0, 0))) int foo (int x)
> {
> return x + 1;
> }
>
> static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
> __attribute__((patchable_function_entry(0, 0))) int bar (int x)
> {
> return x + 2;
> }
>
> int
> baz (int x)
> {
> return foo (x) + 1;
> }
>
> int
> qux (int x)
> {
> return bar (x) + 2;
> }
> (distilled from aarch64 Linux kernel) with
> -O2 -fpatchable-function-entry=2 on aarch64 compiler configured against
> latest binutils, I get:
> ...
> .section__patchable_function_entries,"awo",@progbits,baz
> ...
> .section__patchable_function_entries
> ...
> in the assembly, but when it is assembled, one gets:
> [ 4] __patchable_function_entries PROGBITS 60
> 08 00 WAL 1 0 8
> [ 5] .rela__patchable_function_entries RELA
> 000280 18 18 I 12 4 8
> [ 6] __patchable_function_entries PROGBITS 68
> 08 00 0 0 8
> [ 7] .rela__patchable_function_entries RELA
> 000298 18 18 I 12 6 8
> i.e. one writable allocated section with SHF_LINK_ORDER and another
> non-allocated non-writable without link order. In the kernel case there is
> always one entry in the WAL section and then dozens or more in the
> non-allocated one.
> The kernel then fails to link:
> WARNING: modpost: vmlinux.o (__patchable_function_entries): unexpected
> non-allocatable section.
> Did you forget to use "ax"/"aw" in a .S file?
> Note that for example contains
> section definitions for use in .S files.
> ld: .init.data has both ordered [`__patchable_function_entries' in
> init/main.o] and unordered [`.init.data' in
> ./drivers/firmware/efi/libstub/vsprintf.stub.o] sections
> ld: final link failed: bad value
> make: *** [Makefile:1175: vmlinux] Error 1
>
> If it is correct that the assembler requires full section flags for the
> SECTION_LINK_ORDER .section directives in every case (like it does for
gas is correct.
> comdat or for retain), then we should do something like the following
> untested change, but if it is gas bug, it should be fixed there.
>
> 2020-12-15 Jakub Jelinek
>
> * varasm.c (default_elf_asm_named_section): Always force
> section flags even for sections with SECTION_LINK_ORDER flag.
>
> --- gcc/varasm.c.jj 2020-12-13 17:07:53.910477664 +0100
> +++ gcc/varasm.c2020-12-15 21:33:35.169314414 +0100
> @@ -6781,10 +6781,10 @@ default_elf_asm_named_section (const cha
>
>/* If we have already declared this section, we can use an
> abbreviated form to switch back to it -- unless this section is
> - part of a COMDAT groups or with SHF_GNU_RETAIN, in which case GAS
> - requires the full declaration every time. */
> + part of a COMDAT groups or with SHF_GNU_RETAIN or with SHF_LINK_ORDER,
> + in which case GAS requires the full declaration every time. */
>if (!(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
> - && !(flags & SECTION_RETAIN)
> + && !(flags & (SECTION_RETAIN | SECTION_LINK_ORDER))
>&& (flags & SECTION_DECLARED))
> {
>fprintf (asm_out_file, "\t.section\t%s\n", name);
>
>
> Jakub
>
LGTM. But I can't approve it.
Thanks.
--
H.J.
[PATCH] varasm: Fix up __patchable_function_entries handling
On Wed, Dec 02, 2020 at 05:15:21AM -0800, H.J. Lu via Gcc-patches wrote:
> gcc/
>
> PR middle-end/93195
> PR middle-end/93197
> * configure.ac (HAVE_GAS_SECTION_LINK_ORDER): New. Define 1 if
> the assembler supports the section flag 'o' for specifying
> section with link-order.
> * output.h (SECTION_LINK_ORDER): New. Defined to 0x800.
> (SECTION_MACH_DEP): Changed from 0x800 to 0x1000.
> * targhooks.c (default_print_patchable_function_entry): Pass
> SECTION_LINK_ORDER to switch_to_section if the section flag 'o'
> works. Pass current_function_decl to switch_to_section.
> * varasm.c (default_elf_asm_named_section): Use 'o' flag for
> SECTION_LINK_ORDER if assembler supports it.
> * config.in: Regenerated.
> * configure: Likewise.
Dunno if it is an assembler bug or gcc bug, but this SECTION_LINK_ORDER
stuff doesn't seem to work properly.
If I compile:
static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
__attribute__((patchable_function_entry(0, 0))) int foo (int x)
{
return x + 1;
}
static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
__attribute__((patchable_function_entry(0, 0))) int bar (int x)
{
return x + 2;
}
int
baz (int x)
{
return foo (x) + 1;
}
int
qux (int x)
{
return bar (x) + 2;
}
(distilled from aarch64 Linux kernel) with
-O2 -fpatchable-function-entry=2 on aarch64 compiler configured against
latest binutils, I get:
...
.section__patchable_function_entries,"awo",@progbits,baz
...
.section__patchable_function_entries
...
in the assembly, but when it is assembled, one gets:
[ 4] __patchable_function_entries PROGBITS 60
08 00 WAL 1 0 8
[ 5] .rela__patchable_function_entries RELA
000280 18 18 I 12 4 8
[ 6] __patchable_function_entries PROGBITS 68
08 00 0 0 8
[ 7] .rela__patchable_function_entries RELA
000298 18 18 I 12 6 8
i.e. one writable allocated section with SHF_LINK_ORDER and another
non-allocated non-writable without link order. In the kernel case there is
always one entry in the WAL section and then dozens or more in the
non-allocated one.
The kernel then fails to link:
WARNING: modpost: vmlinux.o (__patchable_function_entries): unexpected
non-allocatable section.
Did you forget to use "ax"/"aw" in a .S file?
Note that for example contains
section definitions for use in .S files.
ld: .init.data has both ordered [`__patchable_function_entries' in init/main.o]
and unordered [`.init.data' in ./drivers/firmware/efi/libstub/vsprintf.stub.o]
sections
ld: final link failed: bad value
make: *** [Makefile:1175: vmlinux] Error 1
If it is correct that the assembler requires full section flags for the
SECTION_LINK_ORDER .section directives in every case (like it does for
comdat or for retain), then we should do something like the following
untested change, but if it is gas bug, it should be fixed there.
2020-12-15 Jakub Jelinek
* varasm.c (default_elf_asm_named_section): Always force
section flags even for sections with SECTION_LINK_ORDER flag.
--- gcc/varasm.c.jj 2020-12-13 17:07:53.910477664 +0100
+++ gcc/varasm.c2020-12-15 21:33:35.169314414 +0100
@@ -6781,10 +6781,10 @@ default_elf_asm_named_section (const cha
/* If we have already declared this section, we can use an
abbreviated form to switch back to it -- unless this section is
- part of a COMDAT groups or with SHF_GNU_RETAIN, in which case GAS
- requires the full declaration every time. */
+ part of a COMDAT groups or with SHF_GNU_RETAIN or with SHF_LINK_ORDER,
+ in which case GAS requires the full declaration every time. */
if (!(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
- && !(flags & SECTION_RETAIN)
+ && !(flags & (SECTION_RETAIN | SECTION_LINK_ORDER))
&& (flags & SECTION_DECLARED))
{
fprintf (asm_out_file, "\t.section\t%s\n", name);
Jakub
