[COFF] Hoist constant pool handling from X86AsmPrinter into AsmPrinter

[mstorsjo]

Summary

In SVN r334523, the first half of comdat constant pool handling was hoisted from X86WindowsTargetObjectFile (which despite the name only was used for msvc targets) into the arch independent TargetLoweringObjectFileCOFF, but the other half of the handling was left behind in X86AsmPrinter::GetCPISymbol.

With only half of the handling in place, inconsistent comdat sections/symbols are created, causing issues with both GNU binutils (avoided for X86 in SVN r335918) and with the MS linker, which would complain like this:

fatal error LNK1143: invalid or corrupt file: no symbol for COMDAT section 0x4

[mstorsjo]

The test goes on top of one from D49637.

[majnemer]

LGTM

[ssijaric]

LGTM

[mstorsjo]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

FWIW, in order to make test/CodeGen/ARM/setjmp_longjmp.ll pass reliably without out of bounds accesses, I have to do this extra modification:

@@ -2664,11 +2664,11 @@ MCSymbol *AsmPrinter::GetBlockAddressSymbol(const BasicBlock *BB) const {
 
 /// GetCPISymbol - Return the symbol for the specified constant pool entry.
 MCSymbol *AsmPrinter::GetCPISymbol(unsigned CPID) const {
-  if (getSubtargetInfo().getTargetTriple().isOSWindows()) {
-    const MachineConstantPoolEntry &CPE =
-        MF->getConstantPool()->getConstants()[CPID];
+  const std::vector<MachineConstantPoolEntry> &CP =
+      MF->getConstantPool()->getConstants();
+  if (getSubtargetInfo().getTargetTriple().isOSWindows() && CPID < CP.size()) {
+    const MachineConstantPoolEntry &CPE = CP[CPID];
    if (!CPE.isMachineConstantPoolEntry() && CPE.getType() &&
        CPE.getType()->isSized()) {
       const DataLayout &DL = MF->getDataLayout();
       SectionKind Kind = CPE.getSectionKind(&DL);

I'll update the diff accordingly.

[mstorsjo]

Added a CPID < CP.size() check to make test/CodeGen/ARM/setjmp_longjmp.ll pass reliably.

[majnemer]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

That's a little weird. Sounds like somebody is predicting the ID of something in the constant pool. Can you find out which path is doing this?

[mstorsjo]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

I can try, but if someone else who is more familiar wants to have a look I'd appreciate it - since CPID doesn't seem to come from somewhere directly further up in the call stack, I'd have to sit down and try trace it through the machinery to see where it comes from.

[efriedma]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

ARMConstantIslands clones constant pool entries in some cases, and assigns them new IDs, but doesn't bother to update the MachineConstantPool because nothing cares (or at least, nothing cared before this patch).

[mstorsjo]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

Thanks Eli for the insight on this, that's very helpful!

What would you suggest on how to move forward with this; updating ARMConstantIslands to update MachineConstantPool accordingly, or remapping indexes back to what MachineConstantPool knows at some point?

As @ssijaric had a need for this patch as well, do you think you could have a look at this issue, I'm not sure how quickly I can come up with a good fix other than what I have here (just avoiding the crash here)?

[mstorsjo]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

Sorry that last message didn't read the way I meant to; could @ssijaric help out with looking into this issue on ARM, in order to be able to land this?

[efriedma]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

Could you move this code to AsmPrinter::EmitConstantPool, where we actually emit the constant pool entries? ARM overrides EmitConstantPool to do its own constant-pool lowering, but that's fine because we don't need to run this code on ARM anyway.

[mstorsjo]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2669 ↗

(On Diff #156700)

Oh, that's a good idea!

Although, the code feels a bit out of place there, but I can overrride GetCPISymbol in ARMAsmPrinter instead.

[mstorsjo]

Overriding GetCPISymbol in ARMAsmPrinter, to avoid using indexes in MachineConstantPool which isn't kept in sync.

[efriedma]

Looks fine.

lib/Target/ARM/ARMAsmPrinter.cpp

240 ↗

(On Diff #157260)

*MachineConstantPool.

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

 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCCodePadder.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCSection.h"
+#include "llvm/MC/MCSectionCOFF.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCSymbolELF.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/MCValue.h"

 }
 
 MCSymbol *AsmPrinter::GetBlockAddressSymbol(const BasicBlock *BB) const {
   return MMI->getAddrLabelSymbol(BB);
 }
 
 /// GetCPISymbol - Return the symbol for the specified constant pool entry.
 MCSymbol *AsmPrinter::GetCPISymbol(unsigned CPID) const {
+  if (getSubtargetInfo().getTargetTriple().isKnownWindowsMSVCEnvironment()) {
+    const MachineConstantPoolEntry &CPE =
+        MF->getConstantPool()->getConstants()[CPID];
+    if (!CPE.isMachineConstantPoolEntry()) {
+      const DataLayout &DL = MF->getDataLayout();
+      SectionKind Kind = CPE.getSectionKind(&DL);
+      const Constant *C = CPE.Val.ConstVal;
+      unsigned Align = CPE.Alignment;
+      if (const MCSectionCOFF *S = dyn_cast<MCSectionCOFF>(
+              getObjFileLowering().getSectionForConstant(DL, Kind, C, Align))) {
+        if (MCSymbol *Sym = S->getCOMDATSymbol()) {
+          if (Sym->isUndefined())
+            OutStreamer->EmitSymbolAttribute(Sym, MCSA_Global);
+          return Sym;
+        }
+      }
+    }
+  }
+
   const DataLayout &DL = getDataLayout();
   return OutContext.getOrCreateSymbol(Twine(DL.getPrivateGlobalPrefix()) +
                                       "CPI" + Twine(getFunctionNumber()) + "_" +
                                       Twine(CPID));
 }
 
 /// GetJTISymbol - Return the symbol for the specified jump table entry.
 MCSymbol *AsmPrinter::GetJTISymbol(unsigned JTID, bool isLinkerPrivate) const {

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

MCSymbol *AArch64AsmPrinter::GetCPISymbol(unsigned CPID) const {
   // Darwin uses a linker-private symbol name for constant-pools (to
   // avoid addends on the relocation?), ELF has no such concept and
   // uses a normal private symbol.
   if (!getDataLayout().getLinkerPrivateGlobalPrefix().empty())
     return OutContext.getOrCreateSymbol(
         Twine(getDataLayout().getLinkerPrivateGlobalPrefix()) + "CPI" +
         Twine(getFunctionNumber()) + "_" + Twine(CPID));
 
-  return OutContext.getOrCreateSymbol(
-      Twine(getDataLayout().getPrivateGlobalPrefix()) + "CPI" +
-      Twine(getFunctionNumber()) + "_" + Twine(CPID));
+  return AsmPrinter::GetCPISymbol(CPID);
 }
 
 void AArch64AsmPrinter::printOperand(const MachineInstr *MI, unsigned OpNum,
                                      raw_ostream &O) {
   const MachineOperand &MO = MI->getOperand(OpNum);
   switch (MO.getType()) {
   default:
     llvm_unreachable("<unknown operand type>");

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

void EmitConstantPool() override {
     // we emit constant pools customly!
   }
   void EmitFunctionBodyEnd() override;
   void EmitFunctionEntryLabel() override;
   void EmitStartOfAsmFile(Module &M) override;
   void EmitEndOfAsmFile(Module &M) override;
   void EmitXXStructor(const DataLayout &DL, const Constant *CV) override;
   void EmitGlobalVariable(const GlobalVariable *GV) override;
-  
+  MCSymbol *GetCPISymbol(unsigned CPID) const;
+
   // lowerOperand - Convert a MachineOperand into the equivalent MCOperand.
   bool lowerOperand(const MachineOperand &MO, MCOperand &MCOp);
 
   //===------------------------------------------------------------------===//
   // XRay implementation
   //===------------------------------------------------------------------===//
 public:
   // XRay-specific lowering for ARM.

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

void ARMAsmPrinter::printOperand(const MachineInstr *MI, int OpNum,
   case MachineOperand::MO_ConstantPoolIndex:
     if (Subtarget->genExecuteOnly())
       llvm_unreachable("execute-only should not generate constant pools");
     GetCPISymbol(MO.getIndex())->print(O, MAI);
     break;
   }
 }
 
+MCSymbol *ARMAsmPrinter::GetCPISymbol(unsigned CPID) const {
+  // The AsmPrinter::GetCPISymbol superclass method tries to use CPID as
+  // indexes in MachineConstantPool, which isn't in sync with indexes used here.
+  const DataLayout &DL = getDataLayout();
+  return OutContext.getOrCreateSymbol(Twine(DL.getPrivateGlobalPrefix()) +
+                                      "CPI" + Twine(getFunctionNumber()) + "_" +
+                                      Twine(CPID));
+}
+
 //===--------------------------------------------------------------------===//
 
 MCSymbol *ARMAsmPrinter::
 GetARMJTIPICJumpTableLabel(unsigned uid) const {
   const DataLayout &DL = getDataLayout();
   SmallString<60> Name;
   raw_svector_ostream(Name) << DL.getPrivateGlobalPrefix() << "JTI"
                             << getFunctionNumber() << '_' << uid;

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

public:
 
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        unsigned AsmVariant, const char *ExtraCode,
                        raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              unsigned AsmVariant, const char *ExtraCode,
                              raw_ostream &OS) override;
 
-  /// Return the symbol for the specified constant pool entry.
-  MCSymbol *GetCPISymbol(unsigned CPID) const override;
-
   bool doInitialization(Module &M) override {
     SMShadowTracker.reset(0);
     SM.reset();
     FM.reset();
     return AsmPrinter::doInitialization(M);
   }
 
   bool runOnMachineFunction(MachineFunction &F) override;
   void EmitFunctionBodyStart() override;
   void EmitFunctionBodyEnd() override;
 };
 
 } // end namespace llvm
 
 #endif

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

void X86AsmPrinter::EmitStartOfAsmFile(Module &M) {
 
   // If this is not inline asm and we're in 16-bit
   // mode prefix assembly with .code16.
   bool is16 = TT.getEnvironment() == Triple::CODE16;
   if (M.getModuleInlineAsm().empty() && is16)
     OutStreamer->EmitAssemblerFlag(MCAF_Code16);
 }
 
-MCSymbol *X86AsmPrinter::GetCPISymbol(unsigned CPID) const {
-  if (Subtarget->isTargetKnownWindowsMSVC()) {
-    const MachineConstantPoolEntry &CPE =
-        MF->getConstantPool()->getConstants()[CPID];
-    if (!CPE.isMachineConstantPoolEntry()) {
-      const DataLayout &DL = MF->getDataLayout();
-      SectionKind Kind = CPE.getSectionKind(&DL);
-      const Constant *C = CPE.Val.ConstVal;
-      unsigned Align = CPE.Alignment;
-      if (const MCSectionCOFF *S = dyn_cast<MCSectionCOFF>(
-              getObjFileLowering().getSectionForConstant(DL, Kind, C, Align))) {
-        if (MCSymbol *Sym = S->getCOMDATSymbol()) {
-          if (Sym->isUndefined())
-            OutStreamer->EmitSymbolAttribute(Sym, MCSA_Global);
-          return Sym;
-        }
-      }
-    }
-  }
-
-  return AsmPrinter::GetCPISymbol(CPID);
-}
-
 static void
 emitNonLazySymbolPointer(MCStreamer &OutStreamer, MCSymbol *StubLabel,
                          MachineModuleInfoImpl::StubValueTy &MCSym) {
   // L_foo$stub:
   OutStreamer.EmitLabel(StubLabel);
   //   .indirect_symbol _foo
   OutStreamer.EmitSymbolAttribute(MCSym.getPointer(), MCSA_IndirectSymbol);
 

llvm/trunk/test/CodeGen/AArch64/win_cst_pool.ll

+; RUN: llc < %s -mtriple=aarch64-win32-msvc | FileCheck %s
 ; RUN: llc < %s -mtriple=aarch64-win32-gnu | FileCheck -check-prefix=MINGW %s
 
 define double @double() {
   ret double 0x0000000000800000
 }
+; CHECK:              .globl  __real@0000000000800000
+; CHECK-NEXT:         .section        .rdata,"dr",discard,__real@0000000000800000
+; CHECK-NEXT:         .p2align  3
+; CHECK-NEXT: __real@0000000000800000:
+; CHECK-NEXT:         .xword   8388608
+; CHECK:      double:
+; CHECK:               adrp    x8, __real@0000000000800000
+; CHECK-NEXT:          ldr     d0, [x8, __real@0000000000800000]
+; CHECK-NEXT:          ret
+
 ; MINGW:              .section        .rdata,"dr"
 ; MINGW-NEXT:         .p2align  3
 ; MINGW-NEXT: [[LABEL:\.LC.*]]:
 ; MINGW-NEXT:         .xword   8388608
 ; MINGW:      double:
 ; MINGW:               adrp    x8, [[LABEL]]
 ; MINGW-NEXT:          ldr     d0, [x8, [[LABEL]]]
 ; MINGW-NEXT:          ret