This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dsanders on May 11 2015, 9:20 AM.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 25479.May 11 2015, 9:20 AM
dsanders retitled this revision from to [mips] Make TTypeEncoding indirect to allow .eh_frame to be read-only..
dsanders updated this object.
dsanders edited the test plan for this revision. (Show Details)
dsanders added a reviewer: petarj.
dsanders added a subscriber: Unknown Object (MLST).
joerg added a subscriber: joerg.May 11 2015, 10:17 AM
joerg added inline comments.
lib/MC/MCObjectFileInfo.cpp
328

Is this within 80columns? It looked very long.

lib/Target/Mips/MipsAsmPrinter.cpp
1073

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

dsanders added inline comments.May 11 2015, 2:08 PM
lib/MC/MCObjectFileInfo.cpp
328

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

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 updated this revision to Diff 25509.May 11 2015, 2:14 PM

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 added inline comments.May 19 2015, 6:33 AM
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 added inline comments.May 19 2015, 8:28 AM
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 added inline comments.May 19 2015, 10:26 AM
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 added inline comments.May 20 2015, 6:16 AM
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 added a comment.May 20 2015, 3:25 PM

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

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 updated this revision to Diff 26315.May 22 2015, 3:44 AM

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 accepted this revision.May 25 2015, 10:44 AM
petarj edited edge metadata.

lgtm.

This revision is now accepted and ready to land.May 25 2015, 10:45 AM
dsanders closed this revision.May 26 2015, 3:23 AM
dsanders added inline comments.May 28 2015, 6:23 AM
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 added inline comments.May 28 2015, 7:46 AM
lib/MC/MCObjectFileInfo.cpp
334

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