X86: Don't emit zero-byte functions on Windows

[hans]

Summary

Empty functions can lead to duplicate entries in the Guard CF Function Table of a binary due to multiple functions sharing the same RVA, causing the kernel to refuse to load that binary.

We had a terrific bug due to this in Chromium.

[MatzeB]

Sounds like a good story :)

• We have various MachineInstr that do not result in actual code getting created (DEBUG_INFO, Labels, etc.) this wouldn't catch them.
• There is a very similar situation handled in AsmPrinter.cpp already around line 1049:

// If the function is empty and the object file uses .subsections_via_symbols,
  // then we need to emit *something* to the function body to prevent the
  // labels from collapsing together.  Just emit a noop.

maybe you can do some refactoring there to do the COFF empty function handling as well?

[mkuper]

What does MSVC do for this? Is MSVC's behavior predicated on /guard:cf? Is it ok to generate empty functions without /guard:cf? Crash on calling an empty function seems like somewhat non-intuitive behavior.
Also, what about thumbv7-windows?

[hans]

In D32330#733021, @MatzeB wrote:

Sounds like a good story :)

• We have various MachineInstr that do not result in actual code getting created (DEBUG_INFO, Labels, etc.) this wouldn't catch them.
• There is a very similar situation handled in AsmPrinter.cpp already around line 1049:

// If the function is empty and the object file uses .subsections_via_symbols,
  // then we need to emit *something* to the function body to prevent the
  // labels from collapsing together.  Just emit a noop.

maybe you can do some refactoring there to do the COFF empty function handling as well?

Nice! I'll try that.

In D32330#733026, @mkuper wrote:

What does MSVC do for this? Is MSVC's behavior predicated on /guard:cf? Is it ok to generate empty functions without /guard:cf? Crash on calling an empty function seems like somewhat non-intuitive behavior.
Also, what about thumbv7-windows?

I haven't been able to lure MSVC into emitting a zero-byte function. For example, for

__declspec(naked) void f() {}

they emit an int3.

We can't put this behind the /guard:cf flag (in fact, clang-cl doesn't even support it right now), because the problem happens when the linker is invoked with /guard:cf, it builds the guard table even if the compiled code isn't instrumented with control-flow checks.

I don't think crashing on calling a zero-byte function is non-intuitive; the function has no ret instruction, so anything could happen. Since Matthias pointed out we already have a function that inserts nops in similar situations, I'm happy to use that instead.

I don't know if CFG is a thing on thumbv7-windows, but if we're doing it in the asm printer now, it's easy to just enable for all windows targets.

[hans]

Use the already existing functionality in AsmPrinter to insert a nop.

[hans]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1056 ↗

(On Diff #96167)

I checked, and there are no in-tree targets that "opt out" in this way.
It would be awkward, because getNoopForMachoTarget() is also used for other things, e.g. ARMAsmPrinter::EmitSled.

[MatzeB]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1053–1054 ↗

(On Diff #96167)

This whole code feels a bit out of place, as we have macho and windows specific handling as part of the generic AsmPrinter now. Should at least have a FIXME or better a new interface to hide this behind some API. So you can query MCAsmInfo or MCTargetStreamer (not sure what is best with my limited MC experience) whether empty functions should be avoided.

1056 ↗

(On Diff #96167)

You can just rename as part of this commit instead of adding a FIXME. getNoopForEmptyFunction() maybe?

[hans]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1053–1054 ↗

(On Diff #96167)

I'll add a FIXME for now.

1056 ↗

(On Diff #96167)

I'll rename to just getNoop(). It seems the main use is for empty functions, but ARMAsmPrinter::EmitSled() also uses it for XRay instrumentation so it seems it really is just used for getting nop instructions.

[hans]

Renamed getNoopForMachoTarget() to getNoop() and added FIXME.

Please take another look.

[MatzeB]

LGTM

lib/Target/ARM/ARMInstrInfo.h

28 ↗

(On Diff #96222)

Could have just removed the "getNoop -" part as we discourage that style nowadays.

[hans]

lib/Target/ARM/ARMInstrInfo.h

28 ↗

(On Diff #96222)

Good point. Will do that when committing.

llvm/trunk/include/llvm/Target/TargetInstrInfo.h

public:
   }
 
   /// Insert a noop into the instruction stream at the specified point.
   virtual void insertNoop(MachineBasicBlock &MBB,
                           MachineBasicBlock::iterator MI) const;
 
 
   /// Return the noop instruction to use for a noop.
-  virtual void getNoopForMachoTarget(MCInst &NopInst) const;
+  virtual void getNoop(MCInst &NopInst) const;
 
   /// Return true for post-incremented instructions.
   virtual bool isPostIncrement(const MachineInstr &MI) const {
     return false;
   }
 
   /// Returns true if the instruction is already predicated.
   virtual bool isPredicated(const MachineInstr &MI) const {

llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

MachineOptimizationRemarkAnalysis R(DEBUG_TYPE, "InstructionCount",
                                       &MF->front());
   R << ore::NV("NumInstructions", NumInstsInFunction)
     << " instructions in function";
   ORE->emit(R);
 
   // If the function is empty and the object file uses .subsections_via_symbols,
   // then we need to emit *something* to the function body to prevent the
   // labels from collapsing together.  Just emit a noop.
-  if ((MAI->hasSubsectionsViaSymbols() && !HasAnyRealCode)) {
+  // Similarly, don't emit empty functions on Windows either. It can lead to
+  // duplicate entries (two functions with the same RVA) in the Guard CF Table
+  // after linking, causing the kernel not to load the binary:
+  // https://developercommunity.visualstudio.com/content/problem/45366/vc-linker-creates-invalid-dll-with-clang-cl.html
+  // FIXME: Hide this behind some API in e.g. MCAsmInfo or MCTargetStreamer.
+  if (!HasAnyRealCode &&
+      (MAI->hasSubsectionsViaSymbols() || TM.getTargetTriple().isOSWindows())) {
     MCInst Noop;
-    MF->getSubtarget().getInstrInfo()->getNoopForMachoTarget(Noop);
+    MF->getSubtarget().getInstrInfo()->getNoop(Noop);
     OutStreamer->AddComment("avoids zero-length function");
-
-    // Targets can opt-out of emitting the noop here by leaving the opcode
-    // unspecified.
-    if (Noop.getOpcode())
-      OutStreamer->EmitInstruction(Noop, getSubtargetInfo());
   }
 
   const Function *F = MF->getFunction();
   for (const auto &BB : *F) {
     if (!BB.hasAddressTaken())
       continue;
     MCSymbol *Sym = GetBlockAddressSymbol(&BB);
     if (Sym->isDefined())

llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp

static const TargetRegisterClass *canFoldCopy(const MachineInstr &MI,
 
   if (RC->hasSubClassEq(MRI.getRegClass(LiveReg)))
     return RC;
 
   // FIXME: Allow folding when register classes are memory compatible.
   return nullptr;
 }
 
-void TargetInstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
-  llvm_unreachable("Not a MachO target");
+void TargetInstrInfo::getNoop(MCInst &NopInst) const {
+  llvm_unreachable("Not implemented");
 }
 
 static MachineInstr *foldPatchpoint(MachineFunction &MF, MachineInstr &MI,
                                     ArrayRef<unsigned> Ops, int FrameIndex,
                                     const TargetInstrInfo &TII) {
   unsigned StartIdx = 0;
   switch (MI.getOpcode()) {
   case TargetOpcode::STACKMAP: {

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h

public:
   bool
   reverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const override;
   bool canInsertSelect(const MachineBasicBlock &, ArrayRef<MachineOperand> Cond,
                        unsigned, unsigned, int &, int &, int &) const override;
   void insertSelect(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
                     const DebugLoc &DL, unsigned DstReg,
                     ArrayRef<MachineOperand> Cond, unsigned TrueReg,
                     unsigned FalseReg) const override;
-  void getNoopForMachoTarget(MCInst &NopInst) const override;
+  void getNoop(MCInst &NopInst) const override;
 
   /// analyzeCompare - For a comparison instruction, return the source registers
   /// in SrcReg and SrcReg2, and the value it compares against in CmpValue.
   /// Return true if the comparison instruction can be analyzed.
   bool analyzeCompare(const MachineInstr &MI, unsigned &SrcReg,
                       unsigned &SrcReg2, int &CmpMask,
                       int &CmpValue) const override;
   /// optimizeCompareInstr - Convert the instruction supplying the argument to

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp

if (Status & AArch64FrameOffsetCanUpdate) {
 
     MI.getOperand(ImmIdx).ChangeToImmediate(NewOffset);
     return Offset == 0;
   }
 
   return false;
 }
 
-void AArch64InstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
+void AArch64InstrInfo::getNoop(MCInst &NopInst) const {
   NopInst.setOpcode(AArch64::HINT);
   NopInst.addOperand(MCOperand::createImm(0));
 }
 
 // AArch64 supports MachineCombiner.
 bool AArch64InstrInfo::useMachineCombiner() const {
 
   return true;

llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.h

protected:
   MachineInstr *commuteInstructionImpl(MachineInstr &MI, bool NewMI,
                                        unsigned OpIdx1,
                                        unsigned OpIdx2) const override;
 
 public:
   // Return whether the target has an explicit NOP encoding.
   bool hasNOP() const;
 
-  virtual void getNoopForElfTarget(MCInst &NopInst) const {
-    getNoopForMachoTarget(NopInst);
-  }
-
   // Return the non-pre/post incrementing version of 'Opc'. Return 0
   // if there is not such an opcode.
   virtual unsigned getUnindexedOpcode(unsigned Opc) const = 0;
 
   MachineInstr *convertToThreeAddress(MachineFunction::iterator &MFI,
                                       MachineInstr &MI,
                                       LiveVariables *LV) const override;
 

llvm/trunk/lib/Target/ARM/ARMInstrInfo.h

 namespace llvm {
   class ARMSubtarget;
 
 class ARMInstrInfo : public ARMBaseInstrInfo {
   ARMRegisterInfo RI;
 public:
   explicit ARMInstrInfo(const ARMSubtarget &STI);
 
-  /// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-  void getNoopForMachoTarget(MCInst &NopInst) const override;
+  /// Return the noop instruction to use for a noop.
+  void getNoop(MCInst &NopInst) const override;
 
   // Return the non-pre/post incrementing version of 'Opc'. Return 0
   // if there is not such an opcode.
   unsigned getUnindexedOpcode(unsigned Opc) const override;
 
   /// getRegisterInfo - TargetInstrInfo is a superset of MRegister info.  As
   /// such, whenever a client has an instance of instruction info, it should
   /// always be able to get register info as well (through this method).

llvm/trunk/lib/Target/ARM/ARMInstrInfo.cpp

 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCInst.h"
 using namespace llvm;
 
 ARMInstrInfo::ARMInstrInfo(const ARMSubtarget &STI)
     : ARMBaseInstrInfo(STI), RI() {}
 
-/// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-void ARMInstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
+/// Return the noop instruction to use for a noop.
+void ARMInstrInfo::getNoop(MCInst &NopInst) const {
   if (hasNOP()) {
     NopInst.setOpcode(ARM::HINT);
     NopInst.addOperand(MCOperand::createImm(0));
     NopInst.addOperand(MCOperand::createImm(ARMCC::AL));
     NopInst.addOperand(MCOperand::createReg(0));
   } else {
     NopInst.setOpcode(ARM::MOVr);
     NopInst.addOperand(MCOperand::createReg(ARM::R0));

llvm/trunk/lib/Target/ARM/ARMMCInstLower.cpp

void ARMAsmPrinter::EmitSled(const MachineInstr &MI, SledKind Kind)
   // register pc is 8 bytes ahead of the jump instruction by the moment CPU
   // is executing it).
   // By analogy to ARMAsmPrinter::emitPseudoExpansionLowering() |case ARM::B|.
   // It is not clear why |addReg(0)| is needed (the last operand).
   EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::Bcc).addImm(20)
     .addImm(ARMCC::AL).addReg(0));
 
   MCInst Noop;
-  Subtarget->getInstrInfo()->getNoopForElfTarget(Noop);
+  Subtarget->getInstrInfo()->getNoop(Noop);
   for (int8_t I = 0; I < NoopsInSledCount; I++)
-  {
     OutStreamer->EmitInstruction(Noop, getSubtargetInfo());
-  }
 
   OutStreamer->EmitLabel(Target);
   recordSled(CurSled, MI, Kind);
 }
 
 void ARMAsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI)
 {
   EmitSled(MI, SledKind::FUNCTION_ENTER);

llvm/trunk/lib/Target/ARM/Thumb1InstrInfo.h

 namespace llvm {
   class ARMSubtarget;
 
 class Thumb1InstrInfo : public ARMBaseInstrInfo {
   ThumbRegisterInfo RI;
 public:
   explicit Thumb1InstrInfo(const ARMSubtarget &STI);
 
-  /// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-  void getNoopForMachoTarget(MCInst &NopInst) const override;
+  /// Return the noop instruction to use for a noop.
+  void getNoop(MCInst &NopInst) const override;
 
   // Return the non-pre/post incrementing version of 'Opc'. Return 0
   // if there is not such an opcode.
   unsigned getUnindexedOpcode(unsigned Opc) const override;
 
   /// getRegisterInfo - TargetInstrInfo is a superset of MRegister info.  As
   /// such, whenever a client has an instance of instruction info, it should
   /// always be able to get register info as well (through this method).

llvm/trunk/lib/Target/ARM/Thumb1InstrInfo.cpp

 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/MC/MCInst.h"
 
 using namespace llvm;
 
 Thumb1InstrInfo::Thumb1InstrInfo(const ARMSubtarget &STI)
     : ARMBaseInstrInfo(STI), RI() {}
 
-/// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-void Thumb1InstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
+/// Return the noop instruction to use for a noop.
+void Thumb1InstrInfo::getNoop(MCInst &NopInst) const {
   NopInst.setOpcode(ARM::tMOVr);
   NopInst.addOperand(MCOperand::createReg(ARM::R8));
   NopInst.addOperand(MCOperand::createReg(ARM::R8));
   NopInst.addOperand(MCOperand::createImm(ARMCC::AL));
   NopInst.addOperand(MCOperand::createReg(0));
 }
 
 unsigned Thumb1InstrInfo::getUnindexedOpcode(unsigned Opc) const {

llvm/trunk/lib/Target/ARM/Thumb2InstrInfo.h

 class ARMSubtarget;
 class ScheduleHazardRecognizer;
 
 class Thumb2InstrInfo : public ARMBaseInstrInfo {
   ThumbRegisterInfo RI;
 public:
   explicit Thumb2InstrInfo(const ARMSubtarget &STI);
 
-  /// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-  void getNoopForMachoTarget(MCInst &NopInst) const override;
+  /// Return the noop instruction to use for a noop.
+  void getNoop(MCInst &NopInst) const override;
 
   // Return the non-pre/post incrementing version of 'Opc'. Return 0
   // if there is not such an opcode.
   unsigned getUnindexedOpcode(unsigned Opc) const override;
 
   void ReplaceTailWithBranchTo(MachineBasicBlock::iterator Tail,
                                MachineBasicBlock *NewDest) const override;
 

llvm/trunk/lib/Target/ARM/Thumb2InstrInfo.cpp

 static cl::opt<bool>
 OldT2IfCvt("old-thumb2-ifcvt", cl::Hidden,
            cl::desc("Use old-style Thumb2 if-conversion heuristics"),
            cl::init(false));
 
 Thumb2InstrInfo::Thumb2InstrInfo(const ARMSubtarget &STI)
     : ARMBaseInstrInfo(STI), RI() {}
 
-/// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-void Thumb2InstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
+/// Return the noop instruction to use for a noop.
+void Thumb2InstrInfo::getNoop(MCInst &NopInst) const {
   NopInst.setOpcode(ARM::tHINT);
   NopInst.addOperand(MCOperand::createImm(0));
   NopInst.addOperand(MCOperand::createImm(ARMCC::AL));
   NopInst.addOperand(MCOperand::createReg(0));
 }
 
 unsigned Thumb2InstrInfo::getUnindexedOpcode(unsigned Opc) const {
   // FIXME

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.h

bool optimizeCompareInstr(MachineInstr &CmpInstr, unsigned SrcReg,
                             unsigned SrcReg2, int Mask, int Value,
                             const MachineRegisterInfo *MRI) const override;
 
   /// GetInstSize - Return the number of bytes of code the specified
   /// instruction may be.  This returns the maximum number of bytes.
   ///
   unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
-  void getNoopForMachoTarget(MCInst &NopInst) const override;
+  void getNoop(MCInst &NopInst) const override;
 
   std::pair<unsigned, unsigned>
   decomposeMachineOperandsTargetFlags(unsigned TF) const override;
 
   ArrayRef<std::pair<unsigned, const char *>>
   getSerializableDirectMachineOperandTargetFlags() const override;
 
   ArrayRef<std::pair<unsigned, const char *>>

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp

void PPCInstrInfo::insertNoop(MachineBasicBlock &MBB,
   // FIXME: Update when POWER9 scheduling model is ready.
   case PPC::DIR_PWR9: Opcode = PPC::NOP_GT_PWR7; break;
   }
 
   DebugLoc DL;
   BuildMI(MBB, MI, DL, get(Opcode));
 }
 
-/// getNoopForMachoTarget - Return the noop instruction to use for a noop.
-void PPCInstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
+/// Return the noop instruction to use for a noop.
+void PPCInstrInfo::getNoop(MCInst &NopInst) const {
   NopInst.setOpcode(PPC::NOP);
 }
 
 // Branch analysis.
 // Note: If the condition register is set to CTR or CTR8 then this is a
 // BDNZ (imm == 1) or BDZ (imm == 0) branch.
 bool PPCInstrInfo::analyzeBranch(MachineBasicBlock &MBB,
                                  MachineBasicBlock *&TBB,

llvm/trunk/lib/Target/X86/X86InstrInfo.h

public:
   /// together. This function takes two integers that represent the load offsets
   /// from the common base address. It returns true if it decides it's desirable
   /// to schedule the two loads together. "NumLoads" is the number of loads that
   /// have already been scheduled after Load1.
   bool shouldScheduleLoadsNear(SDNode *Load1, SDNode *Load2,
                                int64_t Offset1, int64_t Offset2,
                                unsigned NumLoads) const override;
 
-  void getNoopForMachoTarget(MCInst &NopInst) const override;
+  void getNoop(MCInst &NopInst) const override;
 
   bool
   reverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const override;
 
   /// isSafeToMoveRegClassDefs - Return true if it's safe to move a machine
   /// instruction that defines the specified register class.
   bool isSafeToMoveRegClassDefs(const TargetRegisterClass *RC) const override;
 

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

if (!table) { // try the AVX512DQMasked table
     if (table && Domain == 3 && (dom == 1 || table[3] == MI.getOpcode()))
       Domain = 4;
   }
   assert(table && "Cannot change domain");
   MI.setDesc(get(table[Domain - 1]));
 }
 
 /// Return the noop instruction to use for a noop.
-void X86InstrInfo::getNoopForMachoTarget(MCInst &NopInst) const {
+void X86InstrInfo::getNoop(MCInst &NopInst) const {
   NopInst.setOpcode(X86::NOOP);
 }
 
 bool X86InstrInfo::isHighLatencyDef(int opc) const {
   switch (opc) {
   default: return false;
   case X86::DIVPDrm:
   case X86::DIVPDrr:

llvm/trunk/test/CodeGen/X86/empty-function.ll

+; RUN: llc < %s -mtriple=i686-pc-win32   | FileCheck -check-prefix=CHECK -check-prefix=WIN32 %s
+; RUN: llc < %s -mtriple=x86_64-pc-win32 | FileCheck -check-prefix=CHECK -check-prefix=WIN64 %s
+; RUN: llc < %s -mtriple=i386-linux-gnu  | FileCheck -check-prefix=LINUX %s
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc18.0.0"
+
+; Don't emit empty functions on Windows; it can lead to duplicate entries
+; (multiple functions sharing the same RVA) in the Guard CF Function Table which
+; the kernel refuses to load.
+
+define void @f() {
+entry:
+  unreachable
+
+; CHECK-LABEL: f:
+; WIN32: nop
+; WIN64: ud2
+; LINUX-NOT: nop
+; LINUX-NOT: ud2
+
+}