[SPARC] Improve PIC vs non-PIC handling

[joerg]

Summary

SPARC has a somewhat strange handling of %lo and %hi modifiers. The existing implementation doesn't agree with the behavior observed with GNU as and we've been missing the corresponding logic for .set. With the patch, I get exactly the same relocations as with GNU as and most importantly, no longer .text relocations in NetBSD's libc.

[jyknight]

lib/Target/Sparc/AsmParser/SparcAsmParser.cpp

470 ↗

(On Diff #35752)

"set _GLOBAL_OFFSET_TABLE_" should be emitted with a PC22/PC10 reloc to match, too. Can you extract the kind conversion logic to a separate function and just call it?

1088 ↗

(On Diff #35752)

"work slightly differently" seems rather understating the extent of the insanity. :)

test/MC/Sparc/sparc-pic.s

2 ↗

(On Diff #35752)

The nonpic cases are covered by sparc-relocations.s already I think? Guess it's fine to restate them here tho.

49 ↗

(On Diff #35752)

Might be nice to keep both cases since they're supposed to end up the same.

llvm/trunk/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp

#include "SparcGenAsmMatcher.inc"
   OperandMatchResultTy parseOperand(OperandVector &Operands, StringRef Name);
 
   OperandMatchResultTy
   parseSparcAsmOperand(std::unique_ptr<SparcOperand> &Operand,
                        bool isCall = false);
 
   OperandMatchResultTy parseBranchModifiers(OperandVector &Operands);
 
+  // Helper function for dealing with %lo / %hi in PIC mode.
+  const SparcMCExpr *adjustPICRelocation(SparcMCExpr::VariantKind VK,
+                                         const MCExpr *subExpr);
+
   // returns true if Tok is matched to a register and returns register in RegNo.
   bool matchRegisterName(const AsmToken &Tok, unsigned &RegNo,
                          unsigned &RegKind);
 
   bool matchSparcAsmModifiers(const MCExpr *&EVal, SMLoc &EndLoc);
   bool parseDirectiveWord(unsigned Size, SMLoc L);
 
   bool is64Bit() const {
void SparcAsmParser::expandSET(MCInst &Inst, SMLoc IDLoc,
 
   MCOperand PrevReg = MCOperand::createReg(Sparc::G0);
 
   // If not just a signed imm13 value, then either we use a 'sethi' with a
   // following 'or', or a 'sethi' by itself if there are no more 1 bits.
   // In either case, start with the 'sethi'.
   if (!IsEffectivelyImm13) {
     MCInst TmpInst;
-    const MCExpr *Expr =
-        SparcMCExpr::create(SparcMCExpr::VK_Sparc_HI, ValExpr, getContext());
+    const MCExpr *Expr = adjustPICRelocation(SparcMCExpr::VK_Sparc_HI, ValExpr);
     TmpInst.setLoc(IDLoc);
     TmpInst.setOpcode(SP::SETHIi);
     TmpInst.addOperand(MCRegOp);
     TmpInst.addOperand(MCOperand::createExpr(Expr));
     Instructions.push_back(TmpInst);
     PrevReg = MCRegOp;
   }
 
   // The low bits require touching in 3 cases:
   // * A non-immediate value will always require both instructions.
   // * An effectively imm13 value needs only an 'or' instruction.
   // * Otherwise, an immediate that is not effectively imm13 requires the
   //   'or' only if bits remain after clearing the 22 bits that 'sethi' set.
   // If the low bits are known zeros, there's nothing to do.
   // In the second case, and only in that case, must we NOT clear
   // bits of the immediate value via the %lo() assembler function.
   // Note also, the 'or' instruction doesn't mind a large value in the case
   // where the operand to 'set' was 0xFFFFFzzz - it does exactly what you mean.
   if (!IsImm || IsEffectivelyImm13 || (ImmValue & 0x3ff)) {
     MCInst TmpInst;
     const MCExpr *Expr;
     if (IsEffectivelyImm13)
       Expr = ValExpr;
     else
-      Expr =
-          SparcMCExpr::create(SparcMCExpr::VK_Sparc_LO, ValExpr, getContext());
+      Expr = adjustPICRelocation(SparcMCExpr::VK_Sparc_LO, ValExpr);
     TmpInst.setLoc(IDLoc);
     TmpInst.setOpcode(SP::ORri);
     TmpInst.addOperand(MCRegOp);
     TmpInst.addOperand(PrevReg);
     TmpInst.addOperand(MCOperand::createExpr(Expr));
     Instructions.push_back(TmpInst);
   }
 }
static bool hasGOTReference(const MCExpr *Expr) {
   }
 
   case MCExpr::Unary:
     return hasGOTReference(cast<MCUnaryExpr>(Expr)->getSubExpr());
   }
   return false;
 }
 
+const SparcMCExpr *
+SparcAsmParser::adjustPICRelocation(SparcMCExpr::VariantKind VK,
+                                    const MCExpr *subExpr)
+{
+  // When in PIC mode, "%lo(...)" and "%hi(...)" behave differently.
+  // If the expression refers contains _GLOBAL_OFFSETE_TABLE, it is
+  // actually a %pc10 or %pc22 relocation. Otherwise, they are interpreted
+  // as %got10 or %got22 relocation.
+
+  if (getContext().getObjectFileInfo()->getRelocM() == Reloc::PIC_) {
+    switch(VK) {
+    default: break;
+    case SparcMCExpr::VK_Sparc_LO:
+      VK = (hasGOTReference(subExpr) ? SparcMCExpr::VK_Sparc_PC10
+                                     : SparcMCExpr::VK_Sparc_GOT10);
+      break;
+    case SparcMCExpr::VK_Sparc_HI:
+      VK = (hasGOTReference(subExpr) ? SparcMCExpr::VK_Sparc_PC22
+                                     : SparcMCExpr::VK_Sparc_GOT22);
+      break;
+    }
+  }
+
+  return SparcMCExpr::create(VK, subExpr, getContext());
+}
+
 bool SparcAsmParser::matchSparcAsmModifiers(const MCExpr *&EVal,
                                             SMLoc &EndLoc)
 {
   AsmToken Tok = Parser.getTok();
   if (!Tok.is(AsmToken::Identifier))
     return false;
 
   StringRef name = Tok.getString();
 
   SparcMCExpr::VariantKind VK = SparcMCExpr::parseVariantKind(name);
 
   if (VK == SparcMCExpr::VK_Sparc_None)
     return false;
 
   Parser.Lex(); // Eat the identifier.
   if (Parser.getTok().getKind() != AsmToken::LParen)
     return false;
 
   Parser.Lex(); // Eat the LParen token.
   const MCExpr *subExpr;
   if (Parser.parseParenExpression(subExpr, EndLoc))
     return false;
 
-  bool isPIC = getContext().getObjectFileInfo()->getRelocM() == Reloc::PIC_;
-
-  // Ugly: if a sparc assembly expression says "%hi(...)" but the
-  // expression within contains _GLOBAL_OFFSET_TABLE_, it REALLY means
-  // %pc22. Same with %lo -> %pc10. Worse, if it doesn't contain that,
-  // the meaning depends on whether the assembler was invoked with
-  // -KPIC or not: if so, it really means %got22/%got10; if not, it
-  // actually means what it said! Sigh, historical mistakes...
-
-  switch(VK) {
-  default: break;
-  case SparcMCExpr::VK_Sparc_LO:
-    VK =  (hasGOTReference(subExpr)
-           ? SparcMCExpr::VK_Sparc_PC10
-           : (isPIC ? SparcMCExpr::VK_Sparc_GOT10 : VK));
-    break;
-  case SparcMCExpr::VK_Sparc_HI:
-    VK =  (hasGOTReference(subExpr)
-           ? SparcMCExpr::VK_Sparc_PC22
-           : (isPIC ? SparcMCExpr::VK_Sparc_GOT22 : VK));
-    break;
-  }
-
-  EVal = SparcMCExpr::create(VK, subExpr, getContext());
+  EVal = adjustPICRelocation(VK, subExpr);
   return true;
 }
 
 extern "C" void LLVMInitializeSparcAsmParser() {
   RegisterMCAsmParser<SparcAsmParser> A(TheSparcTarget);
   RegisterMCAsmParser<SparcAsmParser> B(TheSparcV9Target);
   RegisterMCAsmParser<SparcAsmParser> C(TheSparcelTarget);
 }

llvm/trunk/test/MC/Sparc/sparc-pic.s

-! RUN: llvm-mc %s -arch=sparcv9 --relocation-model=pic -filetype=obj | llvm-readobj -r | FileCheck %s
+! RUN: llvm-mc %s -arch=sparcv9 --relocation-model=pic -filetype=obj | llvm-readobj -r | FileCheck --check-prefix=PIC %s
+! RUN: llvm-mc %s -arch=sparcv9 --relocation-model=static -filetype=obj | llvm-readobj -r | FileCheck --check-prefix=NOPIC %s
 
 
-! CHECK:      Relocations [
-! CHECK-NOT:    0x{{[0-9,A-F]+}} R_SPARC_WPLT30 .text 0xC
-! CHECK:        0x{{[0-9,A-F]+}} R_SPARC_PC22 _GLOBAL_OFFSET_TABLE_ 0x4
-! CHECK-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_PC10 _GLOBAL_OFFSET_TABLE_ 0x8
-! CHECK-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT22 AGlobalVar 0x0
-! CHECK-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT10 AGlobalVar 0x0
-! CHECK-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT22 .LC0 0x0
-! CHECK-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT10 .LC0 0x0
-! CHECK-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_WPLT30 bar 0x0
-! CHECK:      ]
+! PIC:      Relocations [
+! PIC-NOT:    0x{{[0-9,A-F]+}} R_SPARC_WPLT30 .text 0xC
+! PIC:        0x{{[0-9,A-F]+}} R_SPARC_PC22 _GLOBAL_OFFSET_TABLE_ 0x4
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_PC10 _GLOBAL_OFFSET_TABLE_ 0x8
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_PC22 _GLOBAL_OFFSET_TABLE_ 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_PC10 _GLOBAL_OFFSET_TABLE_ 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT22 AGlobalVar 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT10 AGlobalVar 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT22 AGlobalVar 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT10 AGlobalVar 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT22 .LC0 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_GOT10 .LC0 0x0
+! PIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_WPLT30 bar 0x0
+! PIC:      ]
+
+! NOPIC:      Relocations [
+! NOPIC-NOT:    0x{{[0-9,A-F]+}} R_SPARC_WPLT30 .text 0xC
+! NOPIC:        0x{{[0-9,A-F]+}} R_SPARC_HI22 _GLOBAL_OFFSET_TABLE_ 0x4
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_LO10 _GLOBAL_OFFSET_TABLE_ 0x8
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_HI22 _GLOBAL_OFFSET_TABLE_ 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_LO10 _GLOBAL_OFFSET_TABLE_ 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_HI22 AGlobalVar 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_LO10 AGlobalVar 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_HI22 AGlobalVar 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_LO10 AGlobalVar 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_HI22 .rodata 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_LO10 .rodata 0x0
+! NOPIC-NEXT:   0x{{[0-9,A-F]+}} R_SPARC_WDISP30 bar 0x0
+! NOPIC:      ]
 
         .section        ".rodata"
         .align 8
 .LC0:
         .asciz   "string"
         .section ".text"
         .text
         .globl  foo
         .align  4
         .type   foo,@function
 foo:
         .cfi_startproc
         save %sp, -176, %sp
         .cfi_def_cfa_register %fp
         .cfi_window_save
         .cfi_register 15, 31
 .Ltmp4:
         call .Ltmp5
 .Ltmp6:
         sethi %hi(_GLOBAL_OFFSET_TABLE_+(.Ltmp6-.Ltmp4)), %i1
 .Ltmp5:
         or %i1, %lo(_GLOBAL_OFFSET_TABLE_+(.Ltmp5-.Ltmp4)), %i1
+        set _GLOBAL_OFFSET_TABLE_, %i1
         add %i1, %o7, %i1
         sethi %hi(AGlobalVar), %i2
         add %i2, %lo(AGlobalVar), %i2
+        set AGlobalVar, %i2
         ldx [%i1+%i2], %i3
         ldx [%i3], %i3
         sethi %hi(.LC0), %i2
         add %i2, %lo(.LC0), %i2
         ldx [%i1+%i2], %i4
         call bar
         add %i0, %i1, %o0
         ret