[mips] Make TTypeEncoding indirect to allow .eh_frame to be read-only.

[dsanders]

Summary

Following on from r209907 which made personality encodings indirect, do the
same for TType encodings. This fixes the case where a try/catch block needs
to generate references to, for example, std::exception in the
.gcc_except_table.

Depends on D9668

[joerg]

lib/MC/MCObjectFileInfo.cpp

327

Is this within 80columns? It looked very long.

lib/Target/Mips/MipsAsmPrinter.cpp

1073 ↗

(On Diff #25479)

Why is this chunk needed? TargetLoweringObjectFileELF::getTTypeGlobalReference is not handling this already?

[dsanders]

lib/MC/MCObjectFileInfo.cpp

327

It's 87. I've fixed it in my working copy.

I normally have clang-format as part of a pre-commit hook but it looks like I've forgotten to install it on a new git-clone again. Thanks for noticing.

lib/Target/Mips/MipsAsmPrinter.cpp

1073 ↗

(On Diff #25479)

It was needed for the LLVM that's in AOSP but it looks like that might not be the case on the trunk. It seems Rafael refactored the other instances of this about a month ago. I'll look into it.

[dsanders]

Fix 80 col violation.

Removed the stub emission chunk. Trunk works perfectly without it. It will be
necessary for the 3.6 branch when we merge it to 3.6.2 though.

[petarj]

lib/MC/MCObjectFileInfo.cpp

334

I see your comment for N64 ABI and DW_EH_PE_sdata8, but if it is complex to fix it in this change, and considering incompleteness of N32 and common use(rs) of LLVM today, would not we be closer to correct solution if this case is divided in two switch/cases for 32-bit and 64-bit triples, with dwarf::DW_EH_PE_sdata4 and DW_EH_PE_sdata8 flags set in TTypeEncoding respectively?

[dsanders]

lib/MC/MCObjectFileInfo.cpp

334

It's not correct to use the triple to distinguish between ABI's. For example, it's valid to generate O32 code on a mips64-linux-gnu target. Admittedly, this currently doesn't work due to long standing misuse of triples in our backend which I'm slowly fixing.

Given that the TType is pc relative, we'd be fairly unlucky to need sdata8 in the near future.

[petarj]

lib/MC/MCObjectFileInfo.cpp

334

It's not correct to use the triple to distinguish between ABI's.

I was not saying that. I was implicating that common users of LLVM (such as Android) use common options such as mips64 with N64 ABI and mips32 with O32 ABI.

For example, it's valid to generate O32 code on a mips64-linux-gnu target. Admittedly, this currently doesn't work due to long standing misuse of triples in our backend which I'm slowly fixing.

In LLVM as-is today, this will even trigger "Invalid Arch & ABI pair." error. Check MipsSubtarget.cpp:92-94

Given that the TType is pc relative, we'd be fairly unlucky to need sdata8 in the near future.

I am leaving this to you. The rest of the change looks fine, as far as I can see.

[dsanders]

lib/MC/MCObjectFileInfo.cpp

334

It's not correct to use the triple to distinguish between ABI's.

I was not saying that. I was implicating that common users of LLVM (such as Android) use common options such as mips64 with N64 ABI and mips32 with O32 ABI.

I'm not sure I understand the distinction you're making here.

For example, it's valid to generate O32 code on a mips64-linux-gnu target. Admittedly, this currently doesn't work due to long standing misuse of triples in our backend which I'm slowly fixing.

In LLVM as-is today, this will even trigger "Invalid Arch & ABI pair." error. Check MipsSubtarget.cpp:92-94

Yes, that assert exists because of the historic misuse of target triples in our backend. I haven't removed this one from upstream yet because removing it can cause very confusing failures.

Given that the TType is pc relative, we'd be fairly unlucky to need sdata8 in the near future.

I am leaving this to you. The rest of the change looks fine, as far as I can see.

Ok. I'll follow up on this patch to get the ABI information into this function. If we can get at the TargetMachine then we can get at the ABI.

[joerg]

Can/Should MIPS adjust the code model to match the ABI? E.g. use large for N64 and small or medium for N32/O32?

[dsanders]

Can/Should MIPS adjust the code model to match the ABI? E.g. use large for N64 and small or medium for N32/O32?

I'm not sure. Mips doesn't use CodeModel at the moment so we could potentially use it to mean N64. However, GCC for Mips doesn't have the -mcode-model option that's associated with CodeModel so I'm not sure we should.

I think I've found a way to get at the ABI though. It turns out that the Mips subclasses have access to the TargetMachine which can tell us the ABI. Failing that, I ought to be able to distinguish ELF32 (O32 and N32 ABI's) from ELF64 (N64 ABI) by splitting MipsTargetObject into two and selecting between them.

[dsanders]

Handle N32/N64 differences properly by allowing MipsTargetObjectFile to
change the TType encoding. This allows us to have a reasonable default
when there's no TargetMachine available and do the right thing when it
is available. The exception tables are self-describing so there's no
risk of issues with mixing different encodings in the same binary.

In the long term, MCObjectFileInfo should probably have visibility of the
TargetMachine. Unfortunately there are a few issues with this:

• Not all callers that construct MCObjectFileInfo subclasses have a TargetMachine.
• Of those that have a TargetMachine, not all fully initialize it. In particular TargetOptions/MCTargetOptions are sometimes uninitialized.
• Of those that don't have a TargetMachine, not all have an ABI name stored elsewhere.

[petarj]

lgtm.

[dsanders]

lib/MC/MCObjectFileInfo.cpp

334

Given that the TType is pc relative, we'd be fairly unlucky to need sdata8 in
the near future.

I am leaving this to you. The rest of the change looks fine, as far as I can see.

It turns out we can't use sdata8 for N64. GAS rejects PC-relative references to other sections since there is no 64-bit version of R_MIPS_PC32 (which would presumably be called R_MIPS_PC64). IAS accepts it at the moment and this is probably a bug.

Is it ok to commit the previous version of this patch? (http://reviews.llvm.org/differential/diff/25509/)

[petarj]

lib/MC/MCObjectFileInfo.cpp

334

Yes, if the buildbots are happy, that's the way to go for now. Thanks.

include/llvm/MC/MCObjectFileInfo.h

private:
   void InitMachOMCObjectFileInfo(Triple T);
   void InitELFMCObjectFileInfo(Triple T);
   void InitCOFFMCObjectFileInfo(Triple T);
 
   /// InitEHFrameSection - Initialize EHFrameSection on demand.
   ///
   void InitEHFrameSection();
 
+  virtual unsigned SelectMipsTTypeEncoding() const;
+
 public:
   const Triple &getTargetTriple() const { return TT; }
 };
 
 } // end namespace llvm
 
 #endif

lib/MC/MCObjectFileInfo.cpp

if (RelocM == Reloc::PIC_) {
       LSDAEncoding = dwarf::DW_EH_PE_absptr;
       TTypeEncoding = dwarf::DW_EH_PE_absptr;
     }
     break;
   case Triple::mips:
   case Triple::mipsel:
   case Triple::mips64:
   case Triple::mips64el:
-    // MIPS uses indirect pointer to refer personality functions, so that the
-    // eh_frame section can be read-only.  DW.ref.personality will be generated
-    // for relocation.
+    // MIPS uses indirect pointer to refer personality functions and types, so
+    // that the eh_frame section can be read-only. DW.ref.personality will be
+    // generated for relocation.
     PersonalityEncoding = dwarf::DW_EH_PE_indirect;
+    TTypeEncoding = SelectMipsTTypeEncoding();
+    // We don't support PC-relative LSDA references in GAS so we use the default
+    // DW_EH_PE_absptr for those.
     break;
   case Triple::ppc64:
   case Triple::ppc64le:
     PersonalityEncoding = dwarf::DW_EH_PE_indirect | dwarf::DW_EH_PE_pcrel |
       dwarf::DW_EH_PE_udata8;
     LSDAEncoding = dwarf::DW_EH_PE_pcrel | dwarf::DW_EH_PE_udata8;
     TTypeEncoding = dwarf::DW_EH_PE_indirect | dwarf::DW_EH_PE_pcrel |
       dwarf::DW_EH_PE_udata8;
     break;
void MCObjectFileInfo::InitEHFrameSection() {
   else
     EHFrameSection =
       Ctx->getCOFFSection(".eh_frame",
                           COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
                           COFF::IMAGE_SCN_MEM_READ |
                           COFF::IMAGE_SCN_MEM_WRITE,
                           SectionKind::getDataRel());
 }
+
+unsigned MCObjectFileInfo::SelectMipsTTypeEncoding() const {
+  // FIXME: The N64 ABI ought to use DW_EH_PE_sdata8 but we can't
+  //        identify N64 from just a triple. We currently fix this by
+  //        overriding this function in MipsTargetObjectFile.
+  return dwarf::DW_EH_PE_indirect | dwarf::DW_EH_PE_pcrel |
+         dwarf::DW_EH_PE_sdata4;
+}

lib/Target/Mips/MipsTargetObjectFile.h

const MCSection *SelectSectionForGlobal(const GlobalValue *GV,
                                         const TargetMachine &TM) const override;
 
     /// Return true if this constant should be placed into small data section.
     bool IsConstantInSmallSection(const Constant *CN,
                                   const TargetMachine &TM) const;
 
     const MCSection *getSectionForConstant(SectionKind Kind,
                                            const Constant *C) const override;
+
+    unsigned SelectMipsTTypeEncoding() const override;
   };
 } // end namespace llvm
 
 #endif

lib/Target/Mips/MipsTargetObjectFile.cpp

 
 static cl::opt<bool>
 ExternSData("mextern-sdata", cl::Hidden,
             cl::desc("MIPS: Use gp_rel for data that is not defined by the "
                      "current object."),
             cl::init(true));
 
 void MipsTargetObjectFile::Initialize(MCContext &Ctx, const TargetMachine &TM){
+  this->TM = &static_cast<const MipsTargetMachine &>(TM);
   TargetLoweringObjectFileELF::Initialize(Ctx, TM);
   InitializeELF(TM.Options.UseInitArray);
 
   SmallDataSection = getContext().getELFSection(
       ".sdata", ELF::SHT_PROGBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
 
   SmallBSSSection = getContext().getELFSection(".sbss", ELF::SHT_NOBITS,
                                                ELF::SHF_WRITE | ELF::SHF_ALLOC);
-  this->TM = &static_cast<const MipsTargetMachine &>(TM);
 }
 
 // A address must be loaded from a small section if its size is less than the
 // small section size threshold. Data in this section must be addressed using
 // gp_rel operator.
 static bool IsInSmallSection(uint64_t Size) {
   // gcc has traditionally not treated zero-sized objects as small data, so this
   // is effectively part of the ABI.

 const MCSection *MipsTargetObjectFile::
 getSectionForConstant(SectionKind Kind, const Constant *C) const {
   if (IsConstantInSmallSection(C, *TM))
     return SmallDataSection;
 
   // Otherwise, we work the same as ELF.
   return TargetLoweringObjectFileELF::getSectionForConstant(Kind, C);
 }
+
+unsigned MipsTargetObjectFile::SelectMipsTTypeEncoding() const {
+  return dwarf::DW_EH_PE_indirect | dwarf::DW_EH_PE_pcrel |
+         (TM->getABI().ArePtrs64bit() ? dwarf::DW_EH_PE_sdata8
+                                      : dwarf::DW_EH_PE_sdata4);
+}

test/CodeGen/Mips/ehframe-indirect.ll

-; RUN: llc -mtriple=mipsel-linux-gnu < %s | FileCheck -check-prefix=ALL -check-prefix=O32 %s
-; RUN: llc -mtriple=mipsel-linux-android < %s | FileCheck -check-prefix=ALL -check-prefix=O32 %s
-; RUN: llc -mtriple=mips64el-linux-gnu -target-abi=n32 < %s | FileCheck -check-prefix=ALL -check-prefix=N32 %s
-; RUN: llc -mtriple=mips64el-linux-android -target-abi=n32 < %s | FileCheck -check-prefix=ALL -check-prefix=N32 %s
-; RUN: llc -mtriple=mips64el-linux-gnu < %s | FileCheck -check-prefix=ALL -check-prefix=N64 %s
-; RUN: llc -mtriple=mips64el-linux-android < %s | FileCheck -check-prefix=ALL -check-prefix=N64 %s
+; RUN: llc -mtriple=mipsel-linux-gnu < %s -asm-verbose | FileCheck -check-prefix=ALL -check-prefix=O32 %s
+; RUN: llc -mtriple=mipsel-linux-android < %s -asm-verbose | FileCheck -check-prefix=ALL -check-prefix=O32 %s
+; RUN: llc -mtriple=mips64el-linux-gnu -target-abi=n32 < %s -asm-verbose | FileCheck -check-prefix=ALL -check-prefix=N32 %s
+; RUN: llc -mtriple=mips64el-linux-android -target-abi=n32 < %s -asm-verbose | FileCheck -check-prefix=ALL -check-prefix=N32 %s
+; RUN: llc -mtriple=mips64el-linux-gnu < %s -asm-verbose | FileCheck -check-prefix=ALL -check-prefix=N64 %s
+; RUN: llc -mtriple=mips64el-linux-android < %s -asm-verbose | FileCheck -check-prefix=ALL -check-prefix=N64 %s
+
+@_ZTISt9exception = external constant i8*
 
 define i32 @main() {
 ; ALL: .cfi_startproc
 ; ALL: .cfi_personality 128, DW.ref.__gxx_personality_v0
 
 entry:
   invoke void @foo() to label %cont unwind label %lpad
 ; ALL: foo
 ; ALL: jalr
 
 lpad:
   %0 = landingpad { i8*, i32 } personality i8*
-    bitcast (i32 (...)* @__gxx_personality_v0 to i8*) catch i8* null
+    bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
+    catch i8* null
+    catch i8* bitcast (i8** @_ZTISt9exception to i8*)
   ret i32 0
 
 cont:
   ret i32 0
 }
 ; ALL: .cfi_endproc
 
 declare i32 @__gxx_personality_v0(...)
 
 declare void @foo()
 
+; ALL: GCC_except_table{{[0-9]+}}:
+; O32: .byte 155 # @TType Encoding = indirect pcrel sdata4
+; N32: .byte 155 # @TType Encoding = indirect pcrel sdata4
+; N64: .byte 156 # @TType Encoding = indirect pcrel sdata8
+; ALL: $[[PC_LABEL:tmp[0-9]+]]:
+; O32: .4byte ($_ZTISt9exception.DW.stub)-($[[PC_LABEL]])
+; N32: .4byte ($_ZTISt9exception.DW.stub)-($[[PC_LABEL]])
+; N64: .8byte ($_ZTISt9exception.DW.stub)-($[[PC_LABEL]])
+; ALL: $_ZTISt9exception.DW.stub:
+; O32: .4byte _ZTISt9exception
+; N32: .4byte _ZTISt9exception
+; N64: .8byte _ZTISt9exception
 ; ALL: .hidden DW.ref.__gxx_personality_v0
 ; ALL: .weak DW.ref.__gxx_personality_v0
 ; ALL: .section .data.DW.ref.__gxx_personality_v0,"aGw",@progbits,DW.ref.__gxx_personality_v0,comdat
 ; O32: .align 2
 ; N32: .align 2
 ; N64: .align 3
 ; ALL: .type DW.ref.__gxx_personality_v0,@object
 ; O32: .size DW.ref.__gxx_personality_v0, 4
 ; N32: .size DW.ref.__gxx_personality_v0, 4
 ; N64: .size DW.ref.__gxx_personality_v0, 8
 ; ALL: DW.ref.__gxx_personality_v0:
 ; O32: .4byte __gxx_personality_v0
 ; N32: .4byte __gxx_personality_v0
 ; N64: .8byte __gxx_personality_v0