Add support for kcfi-seal optimization with LTO

[samitolvanen]

Summary

With -fsanitize=kcfi, Clang emits a !kcfi_type attribute for all
global functions, making them valid indirect call targets, whether
the program ends up calling them indirectly or not. With LTO, we can
"seal" the program by dropping types from non-address-taken globals
as long as they are not visible to regular objects, thus limiting the
possible targets that can be called.

Propagate the VisibleToRegularObj property from the linker to LLVM
passes, and drop KCFI type information from globals that don't need
it.

Depends on D142163

[samitolvanen]

ClangBuiltLinux issue: https://github.com/ClangBuiltLinux/linux/issues/1737

[pcc]

Can't this be implicit if LTO is being used?

[samitolvanen]

In D138337#3972009, @pcc wrote:

Can't this be implicit if LTO is being used?

I would prefer to keep this behind a flag (similarly to -mibt-seal), so we can better control when and where the optimization enabled. For example, enabling this implicitly would currently break LTO+KCFI kernel builds as the kernel has global functions that are not address-taken in C code, but still end up being indirectly called because their addresses were taken in assembly code that wasn't visible to LTO (e.g. arm64's alternative_cb mechanism).

[pcc]

In D138337#3972138, @samitolvanen wrote:

In D138337#3972009, @pcc wrote:

Can't this be implicit if LTO is being used?

I would prefer to keep this behind a flag (similarly to -mibt-seal), so we can better control when and where the optimization enabled. For example, enabling this implicitly would currently break LTO+KCFI kernel builds as the kernel has global functions that are not address-taken in C code, but still end up being indirectly called because their addresses were taken in assembly code that wasn't visible to LTO (e.g. arm64's alternative_cb mechanism).

Isn't that just a bug in the optimization? It shouldn't be applying this to symbols where VisibleToRegularObj is set.

[samitolvanen]

In D138337#3972493, @pcc wrote:

Isn't that just a bug in the optimization? It shouldn't be applying this to symbols where VisibleToRegularObj is set.

The current patch simply replicates ibt-seal behavior, but sure, looking at symbol resolution should solve this problem. I'll take a look when have some more time.

[MaskRay]

(Sorry for my belated response.)

If we make ThinLTO properly track combined the address-taken property, and combine precise addressTaken and VisibleToRegularObj, it seems that we can use this condition to decide whether ENDBR is needed with an appropriate code model:

F.hasAddressTaken() || (!F.hasLocalLinkage() && (VisibleToRegularObj || !F.hasHiddenVisibility()))

(For AArch64, even an local linkage symbol may be reached by a range extension thunk. For x86-64 we can rule out range extension thunks, but there is a caution, as internally I know a pending direction to explore that uses range extension thunks even for small code model to decrease relocation overflow pressure.)

Then we don't even need another compiler option like -mibt-seal. The Linux kernel Makefile uses -fvisibility=hidden for Clang LTO. I think it can use -fvisibility=hidden for non-LTO as well, if it doesn't have dynamic linking semantics.

After all, the option just expresses a no-dynamic-linking semantics, and this is satisfied with -fvisibility=hidden (and the guarantee that there is no undefined symbol, which I assume is satisfied)

[nickdesaulniers]

Is https://reviews.llvm.org/D142163 a blocker for this?

[samitolvanen]

In D138337#4078843, @nickdesaulniers wrote:

Is https://reviews.llvm.org/D142163 a blocker for this?

Yes, and this patch needs to be updated to take VisibleToRegularObj into account. I'll update this patch once we figure out how the export symbol list for relocatable links should work.

[nickdesaulniers]

The test is for x86, does this also work for aarch64 with BTI?

[samitolvanen]

In D138337#4080615, @nickdesaulniers wrote:

The test is for x86, does this also work for aarch64 with BTI?

IBT/BTI doesn't affect this specific optimization, it's only about KCFI. If you're referring to ibt-seal, I assume a similar optimization could be added for BTI too.

[nickdesaulniers]

If you're referring to ibt-seal, I assume a similar optimization could be added for BTI too.

Yes, please. Does it make sense to add to this patch, or a follow up on top?

[samitolvanen]

In D138337#4080628, @nickdesaulniers wrote:

If you're referring to ibt-seal, I assume a similar optimization could be added for BTI too.

Yes, please. Does it make sense to add to this patch, or a follow up on top?

I would keep these separate. Note that ibt-seal was also reverted for now and @joaomoreira is working on fixing the issues there.

[nickdesaulniers]

ah, sorry, I think I might be conflating "kcfi-seal" with "ibt-seal?"

[samitolvanen]

Don't drop type hashes from VisibleToRegularObj symbols.

llvm/include/llvm/CodeGen/AsmPrinter.h

public:
 
   void emitStackSizeSection(const MachineFunction &MF);
 
   void emitStackUsage(const MachineFunction &MF);
 
   void emitBBAddrMapSection(const MachineFunction &MF);
 
   void emitKCFITrapEntry(const MachineFunction &MF, const MCSymbol *Symbol);
+  bool needsKCFITypeId(const MachineFunction &MF);
   virtual void emitKCFITypeId(const MachineFunction &MF);
 
   void emitPseudoProbe(const MachineInstr &MI);
 
   void emitRemarksSection(remarks::RemarkStreamer &RS);
 
   /// Emits a label as reference for PC sections.
   void emitPCSectionsLabel(const MachineFunction &MF, const MDNode &MD);

llvm/include/llvm/IR/Function.h

private:
    * bits 14    : HasGC
    * bits 15 : [reserved]
    */
 
   /// Bits from GlobalObject::GlobalObjectSubclassData.
   enum {
     /// Whether this function is materializable.
     IsMaterializableBit = 0,
+    /// Whether this function is visible to regular objects with LTO,
+    IsVisibleToRegularObjBit,
   };
 
   friend class SymbolTableListTraits<Function>;
 
   /// hasLazyArguments/CheckLazyArguments - The argument list of a function is
   /// built on demand, so that the list isn't allocated until the first client
   /// needs it.  The hasLazyArguments predicate returns true if the arg list
   /// hasn't been set up yet.
bool isMaterializable() const {
     return getGlobalObjectSubClassData() & (1 << IsMaterializableBit);
   }
   void setIsMaterializable(bool V) {
     unsigned Mask = 1 << IsMaterializableBit;
     setGlobalObjectSubClassData((~Mask & getGlobalObjectSubClassData()) |
                                 (V ? Mask : 0u));
   }
 
+  bool isVisibleToRegularObj() const {
+    return getGlobalObjectSubClassData() & (1 << IsVisibleToRegularObjBit);
+  }
+  void setIsVisibleToRegularObj(bool V) {
+    unsigned Mask = 1 << IsVisibleToRegularObjBit;
+    setGlobalObjectSubClassData((~Mask & getGlobalObjectSubClassData()) |
+                                (V ? Mask : 0u));
+  }
+
   /// getIntrinsicID - This method returns the ID number of the specified
   /// function, or Intrinsic::not_intrinsic if the function is not an
   /// intrinsic, or if the pointer is null.  This value is always defined to be
   /// zero to allow easy checking for whether a function is intrinsic or not.
   /// The particular intrinsic functions which correspond to this value are
   /// defined in llvm/Intrinsics.h.
   Intrinsic::ID getIntrinsicID() const LLVM_READONLY { return IntID; }
 

llvm/include/llvm/LTO/LTO.h

struct GlobalResolution {
     /// The unmangled name of the global.
     std::string IRName;
 
     /// Keep track if the symbol is visible outside of a module with a summary
     /// (i.e. in either a regular object or a regular LTO module without a
     /// summary).
     bool VisibleOutsideSummary = false;
 
+    /// This symbol is visible to regular objects.
+    bool VisibleToRegularObj = false;
+
     /// The symbol was exported dynamically, and therefore could be referenced
     /// by a shared library not visible to the linker.
     bool ExportDynamic = false;
 
     bool UnnamedAddr = true;
 
     /// True if module contains the prevailing definition.
     bool Prevailing = false;

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitKCFITrapEntry(const MachineFunction &MF,
 
   MCSymbol *Loc = OutContext.createLinkerPrivateTempSymbol();
   OutStreamer->emitLabel(Loc);
   OutStreamer->emitAbsoluteSymbolDiff(Symbol, Loc, 4);
 
   OutStreamer->popSection();
 }
 
+bool AsmPrinter::needsKCFITypeId(const MachineFunction &MF) {
+  const Function &F = MF.getFunction();
+  const Module &M = *F.getParent();
+
+  if (!F.hasMetadata(LLVMContext::MD_kcfi_type))
+    return false;
+
+  // With LTO, we can drop type information from globals that are not
+  // address-taken, as long as they are not visible to regular objects.
+  if (!M.getModuleFlag("LTOPostLink") || F.isVisibleToRegularObj())
+    return true;
+
+  // Disable the optimization for ThinLTO as the address-taken property
+  // is not combined.
+  if (auto *MD =
+          mdconst::extract_or_null<ConstantInt>(M.getModuleFlag("ThinLTO")))
+    if (MD->getZExtValue())
+      return true;
+
+  return F.hasAddressTaken();
+}
+
 void AsmPrinter::emitKCFITypeId(const MachineFunction &MF) {
+  if (!needsKCFITypeId(MF))
+    return;
   const Function &F = MF.getFunction();
   if (const MDNode *MD = F.getMetadata(LLVMContext::MD_kcfi_type))
     emitGlobalConstant(F.getParent()->getDataLayout(),
                        mdconst::extract<ConstantInt>(MD->getOperand(0)));
 }
 
 void AsmPrinter::emitPseudoProbe(const MachineInstr &MI) {
   if (PP) {

llvm/lib/LTO/LTO.cpp

if (Res.LinkerRedefined || Res.VisibleToRegularObj || Sym.isUsed() ||
       // First recorded reference, save the current partition.
       GlobalRes.Partition = Partition;
 
     // Flag as visible outside of summary if visible from a regular object or
     // from a module that does not have a summary.
     GlobalRes.VisibleOutsideSummary |=
         (Res.VisibleToRegularObj || Sym.isUsed() || !InSummary);
 
+    GlobalRes.VisibleToRegularObj |= Res.VisibleToRegularObj;
     GlobalRes.ExportDynamic |= Res.ExportDynamic;
   }
 }
 
 static void writeToResolutionFile(raw_ostream &OS, InputFile *Input,
                                   ArrayRef<SymbolResolution> Res) {
   StringRef Path = Input->getName();
   OS << Path << '\n';
for (const auto &R : GlobalResolutions) {
       // Ignore symbols defined in other partitions.
       // Also skip declarations, which are not allowed to have internal linkage.
       if (!GV || GV->hasLocalLinkage() || GV->isDeclaration())
         continue;
       GV->setUnnamedAddr(R.second.UnnamedAddr ? GlobalValue::UnnamedAddr::Global
                                               : GlobalValue::UnnamedAddr::None);
       if (EnableLTOInternalization && R.second.Partition == 0)
         GV->setLinkage(GlobalValue::InternalLinkage);
+
+      if (auto *F = dyn_cast<Function>(GV))
+        F->setIsVisibleToRegularObj(R.second.VisibleToRegularObj);
     }
 
     RegularLTO.CombinedModule->addModuleFlag(Module::Error, "LTOPostLink", 1);
 
     if (Conf.PostInternalizeModuleHook &&
         !Conf.PostInternalizeModuleHook(0, *RegularLTO.CombinedModule))
       return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
   }

llvm/lib/Target/X86/X86AsmPrinter.cpp

 /// emitKCFITypeId - Emit the KCFI type information in architecture specific
 /// format.
 void X86AsmPrinter::emitKCFITypeId(const MachineFunction &MF) {
   const Function &F = MF.getFunction();
   if (!F.getParent()->getModuleFlag("kcfi"))
     return;
 
   ConstantInt *Type = nullptr;
+  if (needsKCFITypeId(MF))
     if (const MDNode *MD = F.getMetadata(LLVMContext::MD_kcfi_type))
       Type = mdconst::extract<ConstantInt>(MD->getOperand(0));
 
   // If we don't have a type to emit, just emit padding if needed to maintain
   // the same alignment for all functions.
   if (!Type) {
     EmitKCFITypePadding(MF, /*HasType=*/false);
     return;
   }
 

llvm/test/LTO/Resolution/X86/kcfi-seal.ll

+; RUN: opt %s -o %t1.o
+; RUN: llvm-lto2 run -O0 -o %t2.o %t1.o -r %t1.o,a,px -r %t1.o,b,p -r %t1.o,c,p -r %t1.o,p,p
+; RUN: llvm-nm --numeric-sort %t2.o.0 | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+;; VisibleToRegularObj && !AddressTaken
+; CHECK: T __cfi_a
+; CHECK: T a
+define void @a() !kcfi_type !1 {
+  ret void
+}
+
+;; !VisibleToRegularObj && !AddressTaken
+; CHECK-NOT: __cfi_b
+; CHECK: t b
+define void @b() !kcfi_type !1 {
+  ret void
+}
+
+;; !VisibleToRegularObj && AddressTaken
+; CHECK: t __cfi_c
+; CHECK: t c
+define void @c() !kcfi_type !1 {
+  ret void
+}
+
+@p = global ptr @c
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 4, !"kcfi", i32 1}
+!1 = !{i32 12345678}