Page MenuHomePhabricator

[COFF] Hoist constant pool handling from X86AsmPrinter into AsmPrinter

Authored by mstorsjo on Jul 21 2018, 10:06 PM.



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

Diff Detail


Event Timeline

mstorsjo created this revision.Jul 21 2018, 10:06 PM
This revision is now accepted and ready to land.Jul 22 2018, 12:24 PM
mstorsjo added inline comments.Jul 23 2018, 11:57 AM
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 updated this revision to Diff 156855.Jul 23 2018, 12:10 PM

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

majnemer added inline comments.Jul 23 2018, 12:29 PM
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 added inline comments.Jul 23 2018, 12:33 PM
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 added inline comments.Jul 23 2018, 12:44 PM
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 added inline comments.Jul 24 2018, 2:10 PM
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 added inline comments.Jul 24 2018, 2:33 PM
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 added inline comments.Jul 24 2018, 3:55 PM
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 added inline comments.Jul 25 2018, 7:18 AM
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 updated this revision to Diff 157260.Jul 25 2018, 7:31 AM

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

efriedma accepted this revision.Jul 25 2018, 10:46 AM

Looks fine.

240 ↗(On Diff #157260)


This revision was automatically updated to reflect the committed changes.