[PATCH] D32330: X86: Don't emit zero-byte functions on Windows

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1053-1054
+  // https://developercommunity.visualstudio.com/content/problem/45366/vc-linker-creates-invalid-dll-with-clang-cl.html
+  if (!HasAnyRealCode &&
+      (MAI->hasSubsectionsViaSymbols() || TM.getTargetTriple().isOSWindows())) {
     MCInst Noop;
----------------
MatzeB wrote:
> 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.
I'll add a FIXME for now.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1056
     MCInst Noop;
+    // FIXME: getNoopForMachoTarget() is actually not just for Mach-O.
     MF->getSubtarget().getInstrInfo()->getNoopForMachoTarget(Noop);
----------------
MatzeB wrote:
> hans wrote:
> > 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.
> You can just rename as part of this commit instead of adding a FIXME. `getNoopForEmptyFunction()` maybe?
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.


https://reviews.llvm.org/D32330