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.

lib/Target/X86/X86TargetMachine.cpp

 static cl::opt<bool> EnableMachineCombinerPass("x86-machine-combiner",
                                cl::desc("Enable the machine combiner pass"),
                                cl::init(true), cl::Hidden);
 
 namespace llvm {
 
 void initializeWinEHStatePassPass(PassRegistry &);
 void initializeX86ExecutionDepsFixPass(PassRegistry &);
+void initializeX86EmptyFunctionFixPass(PassRegistry &);
 
 } // end namespace llvm
 
 extern "C" void LLVMInitializeX86Target() {
   // Register the target.
   RegisterTargetMachine<X86TargetMachine> X(getTheX86_32Target());
   RegisterTargetMachine<X86TargetMachine> Y(getTheX86_64Target());
 
   PassRegistry &PR = *PassRegistry::getPassRegistry();
   initializeGlobalISel(PR);
   initializeWinEHStatePassPass(PR);
   initializeFixupBWInstPassPass(PR);
   initializeEvexToVexInstPassPass(PR);
   initializeX86ExecutionDepsFixPass(PR);
+  initializeX86EmptyFunctionFixPass(PR);
 }
 
 static std::unique_ptr<TargetLoweringObjectFile> createTLOF(const Triple &TT) {
   if (TT.isOSBinFormatMachO()) {
     if (TT.getArch() == Triple::x86_64)
       return llvm::make_unique<X86_64MachoTargetObjectFile>();
     return llvm::make_unique<TargetLoweringObjectFileMachO>();
   }
public:
   static char ID;
   X86ExecutionDepsFix() : ExecutionDepsFix(ID, X86::VR128XRegClass) {}
   StringRef getPassName() const override {
     return "X86 Execution Dependency Fix";
   }
 };
 char X86ExecutionDepsFix::ID;
 
+class X86EmptyFunctionFix : public MachineFunctionPass {
+public:
+  static char ID;
+  X86EmptyFunctionFix() : MachineFunctionPass(ID) {}
+  StringRef getPassName() const override { return "X86 Empty Function Fix"; }
+  bool runOnMachineFunction(MachineFunction &MF) override {
+    assert(!MF.empty() && "There's always an entry block.");
+    MachineBasicBlock &Entry = *MF.begin();
+    if (++MF.begin() != MF.end() || !Entry.empty())
+      return false;
+
+    // The function has a single basic block, and that is empty. Add a trap
+    // instruction to avoid emitting a zero-byte function.
+    BuildMI(Entry, Entry.begin(), DebugLoc(),
+            MF.getSubtarget().getInstrInfo()->get(X86::TRAP));
+    return true;
+  }
+};
+char X86EmptyFunctionFix::ID;
+
 } // end anonymous namespace
 
 INITIALIZE_PASS(X86ExecutionDepsFix, "x86-execution-deps-fix",
                 "X86 Execution Dependency Fix", false, false)
 
+INITIALIZE_PASS(X86EmptyFunctionFix, "x86-empty-function-fix",
+                "X86 Empty Function Fix", false, false)
+
 TargetPassConfig *X86TargetMachine::createPassConfig(PassManagerBase &PM) {
   return new X86PassConfig(this, PM);
 }
 
 void X86PassConfig::addIRPasses() {
   addPass(createAtomicExpandPass(&getX86TargetMachine()));
 
   TargetPassConfig::addIRPasses();
if (UseVZeroUpper)
     addPass(createX86IssueVZeroUpperPass());
 
   if (getOptLevel() != CodeGenOpt::None) {
     addPass(createX86FixupBWInsts());
     addPass(createX86PadShortFunctions());
     addPass(createX86FixupLEAs());
     addPass(createX86EvexToVexInsts());
   }
+
+  // Make sure not to empty zero-byte functions for Windows. Empty functions can
+  // lead to duplicate entries (different functions with same RVAs) in the Guard
+  // CF Function Table, which the kernel will then refuse to load. See
+  // https://developercommunity.visualstudio.com/content/problem/45366/vc-linker-creates-invalid-dll-with-clang-cl.html
+  if (TM->getTargetTriple().isOSWindows())
+    addPass(new X86EmptyFunctionFix());
 }

test/CodeGen/X86/empty-function.ll

+; RUN: llc < %s -mtriple=i686-pc-win32   | FileCheck -check-prefix=CHECK -check-prefix=WIN -check-prefix=WIN32 %s
+; RUN: llc < %s -mtriple=x86_64-pc-win32 | FileCheck -check-prefix=CHECK -check-prefix=WIN %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:
+; WIN: ud2
+; LINUX-NOT: ud2
+
+}
+
+
+; unreachable still shouldn't trap in general on 32-bit Windows. As long as the
+; function doesn't end up being empty, it's fine.
+
+define void @g() {
+entry:
+  tail call void @foo()
+  unreachable
+
+; WIN32-LABEL: _g:
+; WIN32: calll _foo
+; WIN32-NOT: ud2
+
+}
+
+
+declare void @foo()