[PATCH] D73070: Add function attribute "patchable-function-prefix" to support -fpatchable-function-entry=N,M where M>0

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a subscriber: hans.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

In D73070#1836446 <https://reviews.llvm.org/D73070#1836446>, @peter.smith wrote:

> > -fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.
>
> I've left a comment in D72222 <https://reviews.llvm.org/D72222> but thought it would be worth drawing attention to it here as D72222 <https://reviews.llvm.org/D72222> is quite old and easily missed, CI has found errors in 3 linux files for an allmodconfig build on a clang 10. The details are in the comment in D72222 <https://reviews.llvm.org/D72222>, which isn't at fault itself but it is the first that enables -fpatchable-functions to be selected by the kernel make system.


linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/?h=next-20200123 (ie, kernel staging tree) test for support in arch/arm64/Kconfig via seeing if the compiler recognizes `-fpatchable-function-entry=2` (if so, tries to use it).

Failing asserts should be fixed first before this series, then this series should be rebased on top.

In D73070#1834520 <https://reviews.llvm.org/D73070#1834520>, @MaskRay wrote:

> -fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.


Did it land before the branch, or was it cherry-picked over by @hans ?  I worry that that might make feature detection in the Linux kernel for this feature difficult.  We (kernel, @mrutland) may need to at least keep in mind that there may be separate checks for `cc-option` for `-fpatchable-function-entry=M` vs `-fpatchable-function-entry=M,N`.

In D73070#1834291 <https://reviews.llvm.org/D73070#1834291>, @kristof.beyls wrote:

> In D73070#1834196 <https://reviews.llvm.org/D73070#1834196>, @nickdesaulniers wrote:
>
> > I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up.  I just request we don't try to land this in the clang-10 release unless that is also resolved.  Otherwise clang-11.
>
>
> I think these "patchable function entry" sequences are effectively binary interface, i.e. ABI, as other binary processing tools that use these patchable function entries will make assumptions about the structure of these sequences.
>  Therefore, I think it's important that clang and gcc do produce the same sequences, otherwise we're causing de-facto ABI incompatibility.


I agree 100%; if there's no rush to get this into clang-10, then I'd rather take the series as is and iterate on fixes.  Or maybe a final patch on top in this series as a final fixup to patchable function entry and BTI interwork would be satisfactory before landing the whole set?  Either way, I consider this patch reviewed, so I'm signing off on it, but I would appreciate it @maskray if you did *NOT* land the series without @kristof.beyls ' explicit sign off and response to this suggestion (and with it rebased on fixes for the failed assertions @peter.smith cites above).  I don't feel strongly about it though, so I won't be offended if anyone strongly disagrees.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73070