Fully fix Bug #22115.

[jhibbits]

Summary

In the previous commit, the register was saved, but space was not allocated.
This resulted in the parameter save area potentially clobbering r30, leading to
nasty results.

[hfinkel]

lib/Target/PowerPC/PPCFrameLowering.cpp

1163 ↗

(On Diff #17951)

This variable is only used inside the if statement. Why are you creating it out here?

lib/Target/PowerPC/PPCMachineFunctionInfo.h

123 ↗

(On Diff #17951)

We don't initialize all of the member variables here, but I'd rather we did. Can you please initialize PICBasePointerSaveIndex here.

[jhibbits]

lib/Target/PowerPC/PPCFrameLowering.cpp

1163 ↗

(On Diff #17951)

It's how the others do it (BPSI is identical, which is where I copied it from). I'll fix this one though.

1163 ↗

(On Diff #17951)

D'oh, bad reading on my part. Ignore this. BPSI is used in the condition.

lib/Target/PowerPC/PPCMachineFunctionInfo.h

123 ↗

(On Diff #17951)

Sure thing.

[hfinkel]

Thanks. With those changes, LGTM.

-Hal

• Original Message -----

From: "Justin Hibbits" <[e-mail]>
To: [e-mail], [e-mail]
Cc: [e-mail]
Sent: Friday, January 9, 2015 5:26:23 PM
Subject: Re: [PATCH] Fully fix Bug #22115.

Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:1163
@@ +1162,3 @@
+ // Only used in SVR4 32-bit.
+ int PBPSI = FI->getPICBasePointerSaveIndex();

+ if (FI->usesPICBase()) {

hfinkel wrote:

This variable is only used inside the if statement. Why are you
creating it out here?

It's how the others do it (BPSI is identical, which is where I copied
it from). I'll fix this one though.

Comment at: lib/Target/PowerPC/PPCMachineFunctionInfo.h:123
@@ -119,3 +122,3 @@

MF(MF),
UsesPICBase(0) {}

---

hfinkel wrote:

We don't initialize all of the member variables here, but I'd
rather we did. Can you please initialize PICBasePointerSaveIndex
here.

Sure thing.

http://reviews.llvm.org/D6906

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[jhibbits]

Address hfinkel's comments

[hfinkel]

LGTM

llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp

if (HasBP) {
     } else {
       BPOffset =
         PPCFrameLowering::getBasePointerSaveOffset(isPPC64,
                                                    isDarwinABI,
                                                    isPIC);
     }
   }
 
+  int PBPOffset = 0;
+  if (FI->usesPICBase()) {
+    MachineFrameInfo *FFI = MF.getFrameInfo();
+    int PBPIndex = FI->getPICBasePointerSaveIndex();
+    assert(PBPIndex && "No PIC Base Pointer Save Slot!");
+    PBPOffset = FFI->getObjectOffset(PBPIndex);
+  }
+
   // Get stack alignments.
   unsigned MaxAlign = MFI->getMaxAlignment();
   if (HasBP && MaxAlign > 1)
     assert(isPowerOf2_32(MaxAlign) && isInt<16>(MaxAlign) &&
            "Invalid alignment!");
 
   // Frames of 32KB & larger require special handling because they cannot be
   // indexed into with a simple STDU/STWU/STD/STW immediate offset operand.
void PPCFrameLowering::emitPrologue(MachineFunction &MF) const {
 
   if (HasFP)
     // FIXME: On PPC32 SVR4, we must not spill before claiming the stackframe.
     BuildMI(MBB, MBBI, dl, StoreInst)
       .addReg(FPReg)
       .addImm(FPOffset)
       .addReg(SPReg);
 
-  if (isPIC && !isDarwinABI && !isPPC64 &&
-      MF.getInfo<PPCFunctionInfo>()->usesPICBase())
+  if (FI->usesPICBase())
     // FIXME: On PPC32 SVR4, we must not spill before claiming the stackframe.
     BuildMI(MBB, MBBI, dl, StoreInst)
       .addReg(PPC::R30)
-      .addImm(-8U)
+      .addImm(PBPOffset)
       .addReg(SPReg);
 
   if (HasBP)
     // FIXME: On PPC32 SVR4, we must not spill before claiming the stackframe.
     BuildMI(MBB, MBBI, dl, StoreInst)
       .addReg(BPReg)
       .addImm(BPOffset)
       .addReg(SPReg);
if (HasFP) {
       // Describe where FP was saved, at a fixed offset from CFA.
       unsigned Reg = MRI->getDwarfRegNum(FPReg, true);
       CFIIndex = MMI.addFrameInst(
           MCCFIInstruction::createOffset(nullptr, Reg, FPOffset));
       BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
           .addCFIIndex(CFIIndex);
     }
 
+    if (FI->usesPICBase()) {
+      // Describe where FP was saved, at a fixed offset from CFA.
+      unsigned Reg = MRI->getDwarfRegNum(PPC::R30, true);
+      CFIIndex = MMI.addFrameInst(
+          MCCFIInstruction::createOffset(nullptr, Reg, PBPOffset));
+      BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
+          .addCFIIndex(CFIIndex);
+    }
+
     if (HasBP) {
       // Describe where BP was saved, at a fixed offset from CFA.
       unsigned Reg = MRI->getDwarfRegNum(BPReg, true);
       CFIIndex = MMI.addFrameInst(
           MCCFIInstruction::createOffset(nullptr, Reg, BPOffset));
       BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
           .addCFIIndex(CFIIndex);
     }
if (HasBP) {
     } else {
       BPOffset =
         PPCFrameLowering::getBasePointerSaveOffset(isPPC64,
                                                    isDarwinABI,
                                                    isPIC);
     }
   }
 
+  int PBPOffset = 0;
+  if (FI->usesPICBase()) {
+    MachineFrameInfo *FFI = MF.getFrameInfo();
+    int PBPIndex = FI->getPICBasePointerSaveIndex();
+    assert(PBPIndex && "No PIC Base Pointer Save Slot!");
+    PBPOffset = FFI->getObjectOffset(PBPIndex);
+  }
+
   bool UsesTCRet =  RetOpcode == PPC::TCRETURNri ||
     RetOpcode == PPC::TCRETURNdi ||
     RetOpcode == PPC::TCRETURNai ||
     RetOpcode == PPC::TCRETURNri8 ||
     RetOpcode == PPC::TCRETURNdi8 ||
     RetOpcode == PPC::TCRETURNai8;
 
   if (UsesTCRet) {
BuildMI(MBB, MBBI, dl, TII.get(PPC::LWZ8), TempReg)
       .addImm(8)
       .addReg(SPReg);
 
   if (HasFP)
     BuildMI(MBB, MBBI, dl, LoadInst, FPReg)
       .addImm(FPOffset)
       .addReg(SPReg);
 
-  if (isPIC && !isDarwinABI && !isPPC64 &&
-      MF.getInfo<PPCFunctionInfo>()->usesPICBase())
+  if (FI->usesPICBase())
     // FIXME: On PPC32 SVR4, we must not spill before claiming the stackframe.
     BuildMI(MBB, MBBI, dl, LoadInst)
       .addReg(PPC::R30)
-      .addImm(-8U)
+      .addImm(PBPOffset)
       .addReg(SPReg);
 
   if (HasBP)
     BuildMI(MBB, MBBI, dl, LoadInst, BPReg)
       .addImm(BPOffset)
       .addReg(SPReg);
 
   if (!MustSaveCRs.empty()) // will only occur for PPC64
PPCFrameLowering::processFunctionBeforeCalleeSavedScan(MachineFunction &MF,
   if (!BPSI && RegInfo->hasBasePointer(MF)) {
     int BPOffset = getBasePointerSaveOffset(isPPC64, isDarwinABI, isPIC);
     // Allocate the frame index for the base pointer save area.
     BPSI = MFI->CreateFixedObject(isPPC64? 8 : 4, BPOffset, true);
     // Save the result.
     FI->setBasePointerSaveIndex(BPSI);
   }
 
+  // Reserve stack space for the PIC Base register (R30).
+  // Only used in SVR4 32-bit.
+  if (FI->usesPICBase()) {
+    int PBPSI = FI->getPICBasePointerSaveIndex();
+    PBPSI = MFI->CreateFixedObject(4, -8, true);
+    FI->setPICBasePointerSaveIndex(PBPSI);
+  }
+
   // Reserve stack space to move the linkage area to in case of a tail call.
   int TCSPDelta = 0;
   if (MF.getTarget().Options.GuaranteedTailCallOpt &&
       (TCSPDelta = FI->getTailCallSPDelta()) < 0) {
     MFI->CreateFixedObject(-1 * TCSPDelta, TCSPDelta, true);
   }
 
   // For 32-bit SVR4, allocate the nonvolatile CR spill slot iff the
if (needsFP(MF)) {
     HasGPSaveArea = true;
 
     int FI = PFI->getFramePointerSaveIndex();
     assert(FI && "No Frame Pointer Save Slot!");
 
     FFI->setObjectOffset(FI, LowerBound + FFI->getObjectOffset(FI));
   }
 
+  if (PFI->usesPICBase()) {
+    HasGPSaveArea = true;
+
+    int FI = PFI->getPICBasePointerSaveIndex();
+    assert(FI && "No PIC Base Pointer Save Slot!");
+
+    FFI->setObjectOffset(FI, LowerBound + FFI->getObjectOffset(FI));
+  }
+
   const PPCRegisterInfo *RegInfo =
       static_cast<const PPCRegisterInfo *>(MF.getSubtarget().getRegisterInfo());
   if (RegInfo->hasBasePointer(MF)) {
     HasGPSaveArea = true;
 
     int FI = PFI->getBasePointerSaveIndex();
     assert(FI && "No Base Pointer Save Slot!");
 

llvm/trunk/lib/Target/PowerPC/PPCMachineFunctionInfo.h

class PPCFunctionInfo : public MachineFunctionInfo {
   
   /// ReturnAddrSaveIndex - Frame index of where the return address is stored.
   ///
   int ReturnAddrSaveIndex;
 
   /// Frame index where the old base pointer is stored.
   int BasePointerSaveIndex;
 
+  /// Frame index where the old PIC base pointer is stored.
+  int PICBasePointerSaveIndex;
+
   /// MustSaveLR - Indicates whether LR is defined (or clobbered) in the current
   /// function.  This is only valid after the initial scan of the function by
   /// PEI.
   bool MustSaveLR;
 
   /// Does this function have any stack spills.
   bool HasSpills;
 
class PPCFunctionInfo : public MachineFunctionInfo {
   /// Whether this uses the PIC Base register or not.
   bool UsesPICBase;
 
 public:
   explicit PPCFunctionInfo(MachineFunction &MF) 
     : FramePointerSaveIndex(0),
       ReturnAddrSaveIndex(0),
       BasePointerSaveIndex(0),
+      PICBasePointerSaveIndex(0),
       HasSpills(false),
       HasNonRISpills(false),
       SpillsCR(false),
       SpillsVRSAVE(false),
       LRStoreRequired(false),
       MinReservedArea(0),
       TailCallSPDelta(0),
       HasFastCall(false),
public:
   void setFramePointerSaveIndex(int Idx) { FramePointerSaveIndex = Idx; }
   
   int getReturnAddrSaveIndex() const { return ReturnAddrSaveIndex; }
   void setReturnAddrSaveIndex(int idx) { ReturnAddrSaveIndex = idx; }
 
   int getBasePointerSaveIndex() const { return BasePointerSaveIndex; }
   void setBasePointerSaveIndex(int Idx) { BasePointerSaveIndex = Idx; }
 
+  int getPICBasePointerSaveIndex() const { return PICBasePointerSaveIndex; }
+  void setPICBasePointerSaveIndex(int Idx) { PICBasePointerSaveIndex = Idx; }
+
   unsigned getMinReservedArea() const { return MinReservedArea; }
   void setMinReservedArea(unsigned size) { MinReservedArea = size; }
 
   int getTailCallSPDelta() const { return TailCallSPDelta; }
   void setTailCallSPDelta(int size) { TailCallSPDelta = size; }
 
   /// MustSaveLR - This is set when the prolog/epilog inserter does its initial
   /// scan of the function. It is true if the LR/LR8 register is ever explicitly

llvm/trunk/test/CodeGen/PowerPC/ppc32-pic-large.ll

 ; RUN: llc < %s -mtriple=powerpc-unknown-linux-gnu -relocation-model=pic | FileCheck -check-prefix=LARGE-BSS %s
 @bar = common global i32 0, align 4
 
+declare i32 @call_foo(i32, ...)
+
 define i32 @foo() {
 entry:
   %0 = load i32* @bar, align 4
+  %call = call i32 (i32, ...)* @call_foo(i32 %0, i32 0, i32 1, i32 2, i32 4, i32 8, i32 16, i32 32, i32 64)
   ret i32 %0
 }
 
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"PIC Level", i32 2}
 ; LARGE-BSS:       [[POFF:\.L[0-9]+\$poff]]:
 ; LARGE-BSS-NEXT:    .long .LTOC-[[PB:\.L[0-9]+\$pb]]
 ; LARGE-BSS-NEXT:  foo:
 ; LARGE-BSS:         stw 30, -8(1)
 ; LARGE-BSS:         bl [[PB]]
 ; LARGE-BSS-NEXT:  [[PB]]:
 ; LARGE-BSS:         mflr 30
 ; LARGE-BSS:         lwz [[REG:[0-9]+]], [[POFF]]-[[PB]](30)
 ; LARGE-BSS-NEXT:    add 30, [[REG]], 30
-; LARGE-BSS:         lwz [[VREG:[0-9]+]], [[VREF:\.LC[0-9]+]]-.LTOC(30)
+; LARGE-BSS-DAG:     lwz [[VREG:[0-9]+]], [[VREF:\.LC[0-9]+]]-.LTOC(30)
 ; LARGE-BSS-DAG:     lwz {{[0-9]+}}, 0([[VREG]])
-; LARGE-BSS-DAG:     lwz 30, -8(1)
+; LARGE-BSS-DAG:     stw {{[0-9]+}}, 8(1)
+; LARGE-BSS:         lwz 30, -8(1)
 ; LARGE-BSS:       [[VREF]]:
 ; LARGE-BSS-NEXT:    .long bar

llvm/trunk/test/CodeGen/PowerPC/ppc32-pic.ll

 ; RUN: llc < %s -mtriple=powerpc-unknown-linux-gnu -relocation-model=pic | FileCheck -check-prefix=SMALL-BSS %s
 @bar = common global i32 0, align 4
 
+declare i32 @call_foo(i32, ...)
+
 define i32 @foo() {
 entry:
   %0 = load i32* @bar, align 4
-  ret i32 %0
+  %call = call i32 (i32, ...)* @call_foo(i32 %0, i32 0, i32 1, i32 2, i32 4, i32 8, i32 16, i32 32, i32 64)
+  ret i32 0
 }
 
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"PIC Level", i32 1}
 ; SMALL-BSS-LABEL:foo:
 ; SMALL-BSS:         stw 30, -8(1)
+; SMALL-BSS:         stwu 1, -32(1)
 ; SMALL-BSS:         bl _GLOBAL_OFFSET_TABLE_@local-4
 ; SMALL-BSS:         mflr 30
-; SMALL-BSS:         lwz [[VREG:[0-9]+]], bar@GOT(30)
+; SMALL-BSS-DAG:     stw {{[0-9]+}}, 8(1)
+; SMALL-BSS-DAG:     lwz [[VREG:[0-9]+]], bar@GOT(30)
 ; SMALL-BSS-DAG:     lwz {{[0-9]+}}, 0([[VREG]])
-; SMALL-BSS-DAG:     lwz 30, -8(1)
+; SMALL-BSS:         bl call_foo@PLT
+; SMALL-BSS:         lwz 30, -8(1)