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

[Petar Jovanovic]

================
Comment at: lib/MC/MCObjectFileInfo.cpp:334
@@ +333,3 @@
+    TTypeEncoding = dwarf::DW_EH_PE_indirect | dwarf::DW_EH_PE_pcrel |
+                    dwarf::DW_EH_PE_sdata4;
+    // We don't support PC-relative LSDA references in GAS so we use the default
----------------
dsanders wrote:
> dsanders wrote:
> > petarj wrote:
> > > dsanders wrote:
> > > > petarj wrote:
> > > > > 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?
> > > > 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.
> > > > 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.
> > > > 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.
> > > 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/)
Yes, if the buildbots are happy, that's the way to go for now. Thanks.

http://reviews.llvm.org/D9669

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