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

[Mark Rutland via Phabricator via cfe-commits]

mrutland added a comment.

In D7#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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mark Rutland via Phabricator via cfe-commits]

mrutland added a comment.

In D7#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 D7#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 
>  and 
> 9a24488cb67a90f889529987275c5e411ce01dda 
> ). 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 .

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/D7/new/

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mark Rutland via Phabricator via cfe-commits]

mrutland added a comment.

In D7#1846796 , @mrutland wrote:

> In D7#1839207 , @MaskRay wrote:
>
> > 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 .
>
>
> That's a good point w.r.t. x86; I will get in touch with the people working 
> on ftrace and live-patching there


I spoke with Steven Rostedt (ftrace maintainer), and Josh Poimboeuf 
(livepatching maintainer) in the OFTC/#linux-rt IRC channel. Today x86 uses 
`-mfentry`, and they have no reason to move to `-fpatchable-function-entry` so 
long as the `ENDBR32/ENDBR64` instructions compose nicely with `-mfentry`.

Given that, I don't think x86 kernel folk care either way.

On the GCC side I was under the impression that x86 would go the same way as 
arm64, per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424#c4

There's a GCC ticket for x86: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93492

Thanks,
Mark.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits