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

Wed Jan 29 06:26:32 PST 2020

mrutland added a comment.

In D72222#1839207 <https://reviews.llvm.org/D72222#1839207>, @MaskRay wrote:

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

As covered in D72222#1838988 <https://reviews.llvm.org/D72222#1838988> the plan is to detect broken compilers at  build-time and reject the combination of options. The Linux patching code will not handle varied behaviours at runtime.

> 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.
>  Edit: cherry-picked to release/10.x

That's great; thanks!

> 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 do not expect future M>0 usage to obsolete M=0 usage, and these will be used by distinct configurations of Linux with distinct code paths. I expect that we will need M>0 in future to handle very large kernel images and/or live-patching where we may need to place PLTs or other metadata close to a function. I expect that other cases will continue to use M=0.

I can appreciate the frustration here, but I do think that this is an ABI issue if the compilers diverge in behaviour here. Especially as future additions to the architecture or PCS could complicate matters, and what could seem benign divergence now could turn out to be unreconcilable.

Aligning with the GCC M=0 case here will mean that patching code that works with GCC should just work without modification for clang, and I do think that's a better position to be in generally.

> (Again: this is our ~5th time adding an option specific to Linux kernel, with a good intention to make it general, but in reality it doesn't https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html) I shall also mention that we are essentially making decisions for x86 people's endbr32/endbr64. I hope you can engage in x86 maintainers. Clang's current layout is recorded at D73071 <https://reviews.llvm.org/D73071>.

That's a good point w.r.t. x86; I will get in touch with the people working on ftrace and live-patching there

Thanks,
Mark.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72222