[PATCH] D76900: Enable IBT(Indirect Branch Tracking) in JIT with CET(Control-flow Enforcement Technology)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/X86/X86IndirectBranchTracking.cpp:128
   // unless nocf_check attribute is used.
-  if ((MF.getFunction().hasAddressTaken() ||
+  if ((isJITwithCET || MF.getFunction().hasAddressTaken() ||
        !MF.getFunction().hasLocalLinkage()) &&
----------------
hjl.tools wrote:
> xiangzhangllvm wrote:
> > efriedma wrote:
> > > xiangzhangllvm wrote:
> > > > efriedma wrote:
> > > > > This looks wrong; you can't get the address of a function with internal linkage, whether or not the JIT is involved.
> > > > > 
> > > > > That said, it's possible there something else going wrong here; the JIT defaults to the large code model, which is rarely used otherwise.
> > > > The isInternalLinkage will be checked in line 129, for isInternalLinkage,  !MF.getFunction().hasLocalLinkage() will get true, and then we mainly check the indirect branch tracking requirement at line 130.
> > > The logic without your patch is, if a function has its address taken, or the symbol is visible to other modules, emit ENDBR, because it could be the target of an indirect call.  Otherwise, we don't emit an ENDBR: all possible callers of the function call it directly, and direct calls will be lowered to a "call" instruction, which doesn't need ENDBR.
> > > 
> > > Your patch overrides that logic and says, if we're compiling for a JIT target, emit ENDBR anyway.  That doesn't make sense: the control flow rules are the same whether or not the compiled ELF file is written to disk.
> > Make sense! here should not check isJIT, Thank you very much!
> As
> 
> https://bugs.llvm.org/show_bug.cgi?id=45182
> 
> shows,  MF.getFunction().hasAddressTaken() isn't sufficient for IBT.  Assembly dump has
> landing pad address taken, but MF.getFunction().hasAddressTaken()  returns false.  Without
> 
>   if ((isJITwithCET ||
>        MF.getFunction().hasAddressTaken() ||
>        !MF.getFunction().hasLocalLinkage()) &&
>       !MF.getFunction().doesNoCfCheck()) {
>     auto MBB = MF.begin();
>     Changed |= addENDBR(*MBB, MBB->begin());
>   }
> 
> VNC crashes.  A jitted function has to be referenced.  There are 3 ways to reference a function
> 
> 1. Direct call.
> 2. Indirect call.
> 3. Take address.
> 
> 2 and 3 are mostly likely in x86 jitted codes.
> 
> 
I'm not saying there isn't a bug somewhere. I'm just saying this isn't the right way to write whatever check is necessary.

This code is checking whether all calls must be direct at the IR level.  If all calls are direct at the IR level, they must all be part of the module we're currently JIT'ing.  The code that lowers calls does not check IsJIT.  Therefore, the code here also shouldn't use IsJIT.

LIke I said earlier, I suspect the issue is related to the large code model.


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

https://reviews.llvm.org/D76900