[PATCH] D49644: [COFF] Hoist constant pool handling from X86AsmPrinter into AsmPrinter

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2669
+    const MachineConstantPoolEntry &CPE =
+        MF->getConstantPool()->getConstants()[CPID];
+    if (!CPE.isMachineConstantPoolEntry() && CPE.getType() &&
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > efriedma wrote:
> > > mstorsjo wrote:
> > > > majnemer wrote:
> > > > > mstorsjo wrote:
> > > > > > 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.
> > > > > 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?
> > > > 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.
> > > 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).
> > 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)?
> 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?
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.


https://reviews.llvm.org/D49644