This is an archive of the discontinued LLVM Phabricator instance.

[mips] Adds support for R_MIPS_26, HIGHER, HIGHEST relocations in RuntimeDyld
ClosedPublic

Authored by slthakur on Apr 4 2017, 12:14 AM.

Details

Reviewers
sdardis
Summary

After the N64 static relocation model support was added to llvm it is required to add its support in RuntimeDyld also because lldb uses ExecutionEngine for evaluating expressions.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur created this revision.Apr 4 2017, 12:14 AM
sdardis added inline comments.Apr 10 2017, 4:28 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
874

Shouldn't the N32ABI be reusing the O32 implementation, given it also has a 32 bit address space?

875–878

This comment is out of sync with the code below.

883

Shouldn't this be EF_MIPS_ARCH_64R6 ?

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1319
// Creating Highest,  Higher, Hi and Lo relocations for the filled stub instructions.
1337–1338

Formatting. This should be "} else {".

slthakur updated this revision to Diff 95105.Apr 13 2017, 4:44 AM
slthakur marked 5 inline comments as done.

Addressed review comments

sdardis edited edge metadata.Apr 28 2017, 7:17 AM

Sorry for the delay in getting back to this. I've spotted something that I think is a problem. Highest & higher are processed as a pair, followed by processing Hi & Lo as a pair. I don't think this is correct, See my comment inline.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1316–1317

clang-format this, the indentation for the argument is incorrect.

1336–1340

Indentation of the arguments and line length are wrong here.

1361

Spurious whitespace after the ';'.

1395–1415

I don't think this is correct for HIGHER/HIGHEST/HI/LO. You're creating two relocation entries for a symbol/section. One containing the HIGHEST+HIGHER part of the addend and the other the HI+HO. Shouldn't R_MIPS_HIGHER get pushed back as well and add the RelocationEntry after computing the addend from all four relocations?

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.cpp
152

Spurious blank line.

sdardis requested changes to this revision.May 5 2017, 3:03 AM

Marking this as changes required given my comments.

This revision now requires changes to proceed.May 5 2017, 3:03 AM
slthakur updated this revision to Diff 98259.May 9 2017, 2:19 AM
slthakur edited edge metadata.
slthakur marked 5 inline comments as done.

Addressed review comments.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1395–1415

Actually these relocations are filled in the .rela section for n32/n64 which has a separate addend field. Do we need to do this pair matching for the calculation of the addend?

slthakur updated this revision to Diff 102520.EditedJun 14 2017, 3:47 AM

Removed unnecessary addend calculation.

slthakur planned changes to this revision.Jun 14 2017, 5:04 AM
slthakur added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1361–1366

Just realized I can use processSimpleRelocation here. I will update the diff with current TOT.

slthakur updated this revision to Diff 102788.Jun 15 2017, 11:35 PM

Updated diff with latest TOT.

sdardis accepted this revision.Jun 21 2017, 2:21 AM

LGTM.

This revision is now accepted and ready to land.Jun 21 2017, 2:21 AM
slthakur closed this revision.Jun 22 2017, 4:50 AM

Committed in revision 305997.