[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D72222#1838988 <https://reviews.llvm.org/D72222#1838988>, @mrutland wrote:

> In D72222#1837536 <https://reviews.llvm.org/D72222#1837536>, @MaskRay wrote:
>
> >
>
>
>
>
> > For `BTI c` issue, GCC has several releases that do not work with -mbranch-protection=bti. The Linux kernel has to develop some mechanism to detect the undesirable placement of `bti c`, if there are -mbranch-protection=bti users. So I don't think that inconsistency in clang 10.0.0 with GCC will be a problem.
>
> Speaking as the person implementing the Linux side of things, I think that will be a problem. Kernel-side we want consistency across compilers.
>
> For Linux we were planning to do a very simple build-time test to rule out compilers with the broken behaviour. We would expect functional compilers to have a consistent layout for a given combination of options, and we would consider this layout to be ABI.
>
> The compile time check would compile a trivial test file, e.g.
>
>   void function(void) { }
>
>
> ... with flags:
>
>   -fpatchable-function-entry=2 -mbranch-protection=bti
>
>
> ... and if the function entry point is a NOP, mark that compiler as broken. In practice, this will mean that the kernel build system will disable BTI support for those broken compilers.
>
> Trying to support different layout approaches within Linux will add a fair amount of unnecessary complexity which we cannot contain in one place, and makes it more likely that support for either case will be broken in future.
>
> For the M=0 case, I would prefer to avoid runtime detection unless really necessary.
>
> For the M!=0 case, which I believe Linux will need to use in the near future, I realise that a runtime check will be necessary to detect the absence of a BTI. Regardless, consistent behaviour across compilers will make this significantly simpler and less error-prone.
>
> Thanks,
>  Mark.


When -mbranch-protection=bti is used, due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424 (existing GCC releases have the issue), Linux/arch/arm64 has to make more checks to detect the combination.

I changed `nop; nop; bti c` to `bti c; nop; nop` with 2 commits not included in LLVM release/10.x (a72d15e37c5e066f597f13a8ba60aff214ac992d <https://reviews.llvm.org/rGa72d15e37c5e066f597f13a8ba60aff214ac992d> and 9a24488cb67a90f889529987275c5e411ce01dda <https://reviews.llvm.org/rG9a24488cb67a90f889529987275c5e411ce01dda>). They make Clang's M!=0 placement (.Lpatch; nop; func: bti c; nop) consistent with GCC. If @hans allows me to cherry pick them to release/10.x, I'll be happy to do that.

For the M=0 case, Clang does (`func: .Lpatch; bti c; nop; nop`), which is inconsistent with GCC (`func: bti c; .Lpatch: nop; nop`). I'll be happy to try updating the placement of .Lpatch if future M>0 usage does not obsolete M=0 usage (a proof that the proposed GCC layout is indeed superior, not just for simplicity for a not-so-common configuration), otherwise I would feel the clang side is just making changes to appease GCC/Linux compatibility, which would make me sad. I shall also mention that we are essentially making decisions for x86 people's endbr32/endbr64. I hope you can engage in x86 maintainers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72222/new/

https://reviews.llvm.org/D72222