Bug 92424 - [aarch64] Broken code with -fpatchable-function-entry and BTI
Summary: [aarch64] Broken code with -fpatchable-function-entry and BTI
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: 10.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2019-11-08 14:11 UTC by ktkachov
Modified: 2020-02-24 17:34 UTC (History)
0 users

See Also:
Host:
Target: aarch64
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-01-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description ktkachov 2019-11-08 14:11:33 UTC
-fpatchable-function-entry seems to interact badly with -mbranch-protection=standard (used to insert BTI markers)

void g(void);

void f(void)
{ g(); g(); }

aarch64-linux-gnu-gcc  -mbranch-protection=standard -fpatchable-function-entry=2,1 -O3 -c foo.c -save-temps


Produces:
        .arch armv8-a
        .file   "foo.c"
        .text
        .align  2
        .p2align 3,,7
        .global f
        .section        __patchable_function_entries,"aw",@progbits
        .8byte  .LPFE1
        .text
.LPFE1:
        nop
        .type   f, %function
f:
        nop  // <------- Function entry has NOP rather than PACIASP landing pad
.LFB0:
        .cfi_startproc
        hint    25 // paciasp
        .cfi_window_save
        stp     x29, x30, [sp, -16]!
        .cfi_def_cfa_offset 16
        .cfi_offset 29, -16
        .cfi_offset 30, -8
        mov     x29, sp
        bl      g
        ldp     x29, x30, [sp], 16
        .cfi_restore 30
        .cfi_restore 29
        .cfi_def_cfa_offset 0
        hint    29 // autiasp
        .cfi_window_save
        b       g
        .cfi_endproc
.LFE0:
        .size   f, .-f


Could a possible solution be to emit ofr -mbranch-protection={standard or bti} -fpatchable-function-entry=N,M:

f-4*M:
	.rept M
	nop
	.endr
f:
	bti c
	.rept N-M
	nop
	.endr
	// paciasp may be used from here on in

Unless we fix the codegen we should perhaps error out when the two options are combined rather than producing silently wrong code?
Comment 1 Will Deacon 2019-12-10 10:08:59 UTC
I just ran into this with GCC 9.2 and reproduced on trunk. The combination of -fpatchable-function-entry with -mbranch-protection={standard,bti} is likely to be used for building the Linux kernel when in-kernel BTI is used in conjunction with dynamic function tracing. I think this will be a common configuration, so although forcing the options to be mutually exclusive is definitely better than broken codegen, ultimately we're going to need a way to combine them.

I suspect that placing the PACIASP before the NOPs is going to be problematic unless it's done for all functions (including leaf functions), otherwise hooking the NOPs at runtime is difficult because you can't tell whether or not x30 is signed. You may end up needing to use BTI for the target instead.

The Clang folks are starting to look at -fpatchable-function-entry for AArch64 and will need to follow whatever you end up doing here.
Comment 2 Nick Desaulniers 2020-01-06 18:14:41 UTC
https://reviews.llvm.org/D72215 is the discussion on the interaction between the two in LLVM, FWIW.
Comment 3 ktkachov 2020-01-14 17:22:40 UTC
Marking as confirmed
Comment 4 nsz 2020-01-15 10:32:38 UTC
also affects x86 with -fcf-protection=branch -fpatchable-function-entry=N

that's the same issue so this should not be target specific.
Comment 5 CVS Commits 2020-01-21 15:55:56 UTC
The master branch has been updated by Szabolcs Nagy <nsz@gcc.gnu.org>:

https://gcc.gnu.org/g:c292cfe539cd7c060caad826d362ed5e845bfbef

commit r10-6114-gc292cfe539cd7c060caad826d362ed5e845bfbef
Author: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date:   Wed Jan 15 12:23:40 2020 +0000

    [AArch64] PR92424: Fix -fpatchable-function-entry=N,M with BTI
    
    This is a workaround that emits a BTI after the function label if that
    is followed by a patch area. We try to remove the BTI that follows the
    patch area (this may fail e.g. if the first instruction is a PACIASP).
    
    So before this commit -fpatchable-function-entry=3,1 with bti generates
    
        .section __patchable_function_entries
        .8byte .LPFE
        .text
      .LPFE:
        nop
      foo:
        nop
        nop
        bti c // or paciasp
        ...
    
    and after this commit
    
        .section __patchable_function_entries
        .8byte .LPFE
        .text
      .LPFE:
        nop
      foo:
        bti c
        nop
        nop
        // may be paciasp
        ...
    
    and with -fpatchable-function-entry=1 (M=0) the code now is
    
      foo:
        bti c
        .section __patchable_function_entries
        .8byte .LPFE
        .text
      .LPFE:
        nop
        // may be paciasp
        ...
    
    There is a new bti insn in the middle of the patchable area users need
    to be aware of unless M=0 (patch area is after the new bti) or M=N
    (patch area is before the label, no new bti). Note: bti is not added to
    all functions consistently (it can be turned off per function using a
    target attribute or the compiler may detect that the function is never
    called indirectly), so if bti is inserted in the middle of a patch area
    then user code needs to deal with detecting it.
    
    Tested on aarch64-none-linux-gnu.
    
    gcc/ChangeLog:
    
    	PR target/92424
    	* config/aarch64/aarch64.c (aarch64_declare_function_name): Set
    	cfun->machine->label_is_assembled.
    	(aarch64_print_patchable_function_entry): New.
    	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
    	* config/aarch64/aarch64.h (struct machine_function): New field,
    	label_is_assembled.
    
    gcc/testsuite/ChangeLog:
    
    	PR target/92424
    	* gcc.target/aarch64/pr92424-1.c: New test.
    	* gcc.target/aarch64/pr92424-2.c: New test.
    	* gcc.target/aarch64/pr92424-3.c: New test.
Comment 6 CVS Commits 2020-01-29 14:38:31 UTC
The releases/gcc-9 branch has been updated by Szabolcs Nagy <nsz@gcc.gnu.org>:

https://gcc.gnu.org/g:a1f8dca201ee08f526342ca9cdf022a9ea92e1b3

commit r9-8188-ga1f8dca201ee08f526342ca9cdf022a9ea92e1b3
Author: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date:   Wed Jan 15 12:23:40 2020 +0000

    [AArch64] PR92424: Fix -fpatchable-function-entry=N,M with BTI
    
    This is a workaround that emits a BTI after the function label if that
    is followed by a patch area. We try to remove the BTI that follows the
    patch area (this may fail e.g. if the first instruction is a PACIASP).
    
    So before this commit -fpatchable-function-entry=3,1 with bti generates
    
        .section __patchable_function_entries
        .8byte .LPFE
        .text
      .LPFE:
        nop
      foo:
        nop
        nop
        bti c // or paciasp
        ...
    
    and after this commit
    
        .section __patchable_function_entries
        .8byte .LPFE
        .text
      .LPFE:
        nop
      foo:
        bti c
        nop
        nop
        // may be paciasp
        ...
    
    and with -fpatchable-function-entry=1 (M=0) the code now is
    
      foo:
        bti c
        .section __patchable_function_entries
        .8byte .LPFE
        .text
      .LPFE:
        nop
        // may be paciasp
        ...
    
    There is a new bti insn in the middle of the patchable area users need
    to be aware of unless M=0 (patch area is after the new bti) or M=N
    (patch area is before the label, no new bti). Note: bti is not added to
    all functions consistently (it can be turned off per function using a
    target attribute or the compiler may detect that the function is never
    called indirectly), so if bti is inserted in the middle of a patch area
    then user code needs to deal with detecting it.
    
    Tested on aarch64-none-linux-gnu.
    
    gcc/ChangeLog:
    
    	PR target/92424
    	* config/aarch64/aarch64.c (aarch64_declare_function_name): Set
    	cfun->machine->label_is_assembled.
    	(aarch64_print_patchable_function_entry): New.
    	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
    	* config/aarch64/aarch64.h (struct machine_function): New field,
    	label_is_assembled.
    
    gcc/testsuite/ChangeLog:
    
    	PR target/92424
    	* gcc.target/aarch64/pr92424-2.c: New test.
    	* gcc.target/aarch64/pr92424-3.c: New test.
Comment 7 nsz 2020-01-29 15:09:12 UTC
fixed for gcc-10 and gcc-9.3, opened bug 93492 for the x86 case.
Comment 8 Fangrui Song 2020-01-31 00:01:01 UTC
Where shall we place .cfi_startproc?

Clang HEAD (and clang 10)'s placement is:

foo:
.loc 1 3 0      # line number
.cfi_startproc  # CFI
  bti c
.Lpatch0:       # __patchable_function_entries label
  nop

Not placing .cfi_startproc (as GCC current does) will make addr2line on the function entry address print ??:0

For M>0, clang does not attach line number information for NOPs before the function entry label.