This is an archive of the discontinued LLVM Phabricator instance.

Fix Android MIPS exception personality encoding.
ClosedPublic

Authored by logan on May 19 2014, 9:57 AM.

Details

Reviewers
dsanders
logan
Summary

Android MIPS executable loader prohibits the relocation
in the read-only section. Thus, we have to use
DW_EH_PE_indirect instead.

Diff Detail

Event Timeline

logan updated this revision to Diff 9580.May 19 2014, 9:57 AM
logan retitled this revision from to Fix Android MIPS exception personality encoding..
logan updated this object.
logan edited the test plan for this revision. (Show Details)
logan added a reviewer: ahatanak.
logan added a subscriber: Unknown Object (MLST).
logan edited reviewers, added: dsanders; removed: ahatanak.May 19 2014, 10:14 AM
dsanders edited edge metadata.May 22 2014, 3:22 AM

My background is in C and Embedded-C compilers so my knowledge of things like CFI is rather limited at the moment and I don't think I'm suitable to review this patch. Hopefully someone who knows this area better will also take a look.

Bearing that in mind, a couple things look a bit odd to me. I've put the comments inline.

lib/MC/MCObjectFileInfo.cpp
328

I'm fairly sure that the if-statement should be testing for RelocM == Reloc::PIC_ rather than Android.

I'm not so sure what the correct PersonalityEncoding is but DW_EH_PE_indirect by itself seems odd. I see that all the other targets are combining DW_EH_PE_indirect with DW_EH_PE_pcrel and one of the data size values. If this is the right thing to do for MIPS then it removes the need for the change to getCFIPersonalitySymbol(). However, it's possible that it should be combining DW_EH_PE_indirect with DW_EH_PE_absptr.

joerg added a subscriber: joerg.May 22 2014, 3:29 AM

The problem here is that MIPS doesn't allow PC-relative relocations against a symbol in a different section. That said, I believe indirect encoding should be used for MIPS in the PIC case in general. I'm not sure about the correctness of the rest of the patch yet.

logan updated this revision to Diff 9785.May 24 2014, 6:05 AM
logan edited edge metadata.

The indirect pointer change should not limit to Android.

After checking the output of GCC on Debian mipsel port,
it seems that this change should be applied regardless the -fPIC
flags.

Besides, Joerg's comment is correct. It seems that
MIPS doesn't allow PC-relative relocations between sections, thus
we should use DW_EH_PE_absptr instead of PW_EH_PE_pcrel.

p.s. DW_EH_PE_absptr = 0

joerg added inline comments.May 24 2014, 10:29 AM
lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
63

Don't we just want to check DW_EH_PE_indirect here? Non-indirect references would have already been in place.

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
52

Same here.

lib/MC/MCObjectFileInfo.cpp
327

Just plain DW_EH_PE_indirect, I think. The absptr is implicit.

logan updated this revision to Diff 9789.May 25 2014, 2:17 PM

Address Joerg's comments.

logan accepted this revision.May 30 2014, 10:11 AM
logan added a reviewer: logan.
This revision is now accepted and ready to land.May 30 2014, 10:11 AM
logan closed this revision.May 30 2014, 10:12 AM

Committed as rL209907.