[ARM][AArch64] SLSHardening: make non-comdat thunks possible

[danielkiss]

Summary

Linker scripts might not handle COMDAT sections. SLSHardeing adds
new section for each __llvm_slsblr_thunk_xN. This new option allows
the generation of the thunks into the normal text section to handle these
exceptional cases.
,comdat or ,noncomdat can be added to harden-sls to control the codegen.
-mharden-sls=[all|retbr|blr],nocomdat.

[tamas.petz]

llvm/include/llvm/CodeGen/IndirectThunks.h

56

NIT: tabs to spaces

[kristof.beyls]

Apart from maybe needing to run clang-format on the patch, the code changes look good to me.
Before this could be committed, this needs tests.

llvm/include/llvm/CodeGen/IndirectThunks.h

43–44

I'm not entirely sure, but is this how clang-format formats these lines?

[danielkiss]

Adding a fronted flag and tests.
comdat is still the default, feels fine but I'm not sure.

[danielkiss]

llvm/include/llvm/CodeGen/IndirectThunks.h

43–44

yes, patch went thru git clang-format

[tamas.petz]

I have just a few small comments.

llvm/include/llvm/CodeGen/IndirectThunks.h

31

Is the default argument really required?

57

There are still TABs there.

[danielkiss]

llvm/include/llvm/CodeGen/IndirectThunks.h

31

yes, this function is called from x86 backed and I'd not change that.

57

I think these are spaces, could you please take a look at the raw diff?

[kristof.beyls]

clang/lib/Driver/ToolChains/Arch/ARM.cpp

837–840 ↗

(On Diff #344079)

I think it would be better if comdat/nocomdat were also communicated through Features.
It keeps the interface on this part of the code base consistent (no need to introduce a CmdArgs feature); and also makes sure that LTO vs non-LTO builds keep on behaving the same way, as the feature is set per function, not per translation unit.

I haven't really thought about how to best specify that feature. Maybe just have an extra 2 features: +harden-sls-comdat and +harden-sls-nocomdat?

llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll

218–220 ↗

(On Diff #344079)

Would it be possible to also have check lines for HARDEN-COMDAT-OFF, i.e. look for lines that HARDEN-COMDAT-OFF should produce?

llvm/test/CodeGen/ARM/speculation-hardening-sls.ll

249–251 ↗

(On Diff #344079)

Would it be possible to also have check lines for HARDEN-COMDAT-OFF, i.e. look for lines that HARDEN-COMDAT-OFF should produce?

[danielkiss]

clang/lib/Driver/ToolChains/Arch/ARM.cpp

837–840 ↗

(On Diff #344079)

I almost added a feature for the harden-sls-noncomdat but that is not really a HW feature so felt the backed flag is better.
One feature bit seems enough to describe the two states.

[danielkiss]

address review comments.

[kristof.beyls]

Thanks @danielkiss !
I only have a few nit picky remarks.
Overall looks good!

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

275 ↗

(On Diff #346151)

I'm wondering if the '(EnableRetBr || EnableBlr)' condition is needed here?
Just dropping it would make this code a little bit simpler.
I'm assuming here of course that the semantics of the +harden-sls-comdat target feature is to not produce comdat sections in case any of the other sls-hardening target features would produce thunks.
I agree this is a bit of a nit pick though.

clang/lib/Driver/ToolChains/Arch/ARM.cpp

837 ↗

(On Diff #346151)

Same comment/thought on the "(EnableRetBr || EnableBlr)" condition as on the AArch64 side.

llvm/lib/Target/AArch64/AArch64.td

528 ↗

(On Diff #346151)

I think it would be more accurate if the word "thunk" would be present in the documentation.
What about the following:
"Generate thunk code for SLS mitigation in the normal text section"?

llvm/lib/Target/ARM/ARM.td

578 ↗

(On Diff #346151)

Same comment as on the AArch64 side on using the word "thunk" somewhere in the help text.

[danielkiss]

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

275 ↗

(On Diff #346151)

the check is a bit overkill, the backend won't do anything anyway without the EnableRetBr || EnableBlr

[kristof.beyls]

LGTM.

llvm/include/llvm/CodeGen/IndirectThunks.h

 namespace llvm {
 
 template <typename Derived> class ThunkInserter {
   Derived &getDerived() { return *static_cast<Derived *>(this); }
 
 protected:
   bool InsertedThunks;
   void doInitialization(Module &M) {}
-  void createThunkFunction(MachineModuleInfo &MMI, StringRef Name);
+  void createThunkFunction(MachineModuleInfo &MMI, StringRef Name,
+                           bool Comdat = true);
 
 public:
   void init(Module &M) {
     InsertedThunks = false;
     getDerived().doInitialization(M);
   }
   // return `true` if `MMI` or `MF` was modified
   bool run(MachineModuleInfo &MMI, MachineFunction &MF);
 };
 
 template <typename Derived>
 void ThunkInserter<Derived>::createThunkFunction(MachineModuleInfo &MMI,
-                                                 StringRef Name) {
+                                                 StringRef Name, bool Comdat) {
   assert(Name.startswith(getDerived().getThunkPrefix()) &&
          "Created a thunk with an unexpected prefix!");
 
   Module &M = const_cast<Module &>(*MMI.getModule());
   LLVMContext &Ctx = M.getContext();
   auto Type = FunctionType::get(Type::getVoidTy(Ctx), false);
-  Function *F =
-      Function::Create(Type, GlobalValue::LinkOnceODRLinkage, Name, &M);
+  Function *F = Function::Create(Type,
+                                 Comdat ? GlobalValue::LinkOnceODRLinkage
+                                        : GlobalValue::InternalLinkage,
+                                 Name, &M);
+  if (Comdat) {
     F->setVisibility(GlobalValue::HiddenVisibility);
     F->setComdat(M.getOrInsertComdat(Name));
+  }
 
   // Add Attributes so that we don't create a frame, unwind information, or
   // inline.
   AttrBuilder B;
   B.addAttribute(llvm::Attribute::NoUnwind);
   B.addAttribute(llvm::Attribute::Naked);
   F->addAttributes(llvm::AttributeList::FunctionIndex, B);
 

llvm/lib/Target/AArch64/AArch64SLSHardening.cpp

 #include <cassert>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "aarch64-sls-hardening"
 
 #define AARCH64_SLS_HARDENING_NAME "AArch64 sls hardening pass"
 
+static cl::opt<bool>
+    ComdatThunks("aarch64-sls-hardening-comdat",
+                 cl::desc("Output SLS hardening thunks in comdats"),
+                 cl::init(true), cl::Hidden);
 namespace {
 
 class AArch64SLSHardening : public MachineFunctionPass {
 public:
   const TargetInstrInfo *TII;
   const TargetRegisterInfo *TRI;
   const AArch64Subtarget *ST;
 

 };
 } // namespace
 
 void SLSBLRThunkInserter::insertThunks(MachineModuleInfo &MMI) {
   // FIXME: It probably would be possible to filter which thunks to produce
   // based on which registers are actually used in BLR instructions in this
   // function. But would that be a worthwhile optimization?
   for (auto T : SLSBLRThunks)
-    createThunkFunction(MMI, T.Name);
+    createThunkFunction(MMI, T.Name, ComdatThunks);
 }
 
 void SLSBLRThunkInserter::populateThunk(MachineFunction &MF) {
   // FIXME: How to better communicate Register number, rather than through
   // name and lookup table?
   assert(MF.getName().startswith(getThunkPrefix()));
   auto ThunkIt = llvm::find_if(
       SLSBLRThunks, [&MF](auto T) { return T.Name == MF.getName(); });

llvm/lib/Target/ARM/ARMSLSHardening.cpp

 #include <cassert>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "arm-sls-hardening"
 
 #define ARM_SLS_HARDENING_NAME "ARM sls hardening pass"
 
+static cl::opt<bool>
+    ComdatThunks("arm-sls-hardening-comdat",
+                 cl::desc("Output SLS hardening thunks in comdats"),
+                 cl::init(true), cl::Hidden);
+
 namespace {
 
 class ARMSLSHardening : public MachineFunctionPass {
 public:
   const TargetInstrInfo *TII;
   const ARMSubtarget *ST;
 
   static char ID;

 };
 } // namespace
 
 void SLSBLRThunkInserter::insertThunks(MachineModuleInfo &MMI) {
   // FIXME: It probably would be possible to filter which thunks to produce
   // based on which registers are actually used in indirect calls in this
   // function. But would that be a worthwhile optimization?
   for (auto T : SLSBLRThunks)
-    createThunkFunction(MMI, T.Name);
+    createThunkFunction(MMI, T.Name, ComdatThunks);
 }
 
 void SLSBLRThunkInserter::populateThunk(MachineFunction &MF) {
   // FIXME: How to better communicate Register number, rather than through
   // name and lookup table?
   assert(MF.getName().startswith(getThunkPrefix()));
   auto ThunkIt = llvm::find_if(
       SLSBLRThunks, [&MF](auto T) { return T.Name == MF.getName(); });