This is an archive of the discontinued LLVM Phabricator instance.

[mips][rtdyld] Move MIPS relocation resolution to a subclass and implement N32 relocations
ClosedPublic

Authored by sdardis on Dec 6 2016, 9:27 AM.

Details

Summary

N32 relocations are only correct for individual relocations at the moment.
Support for relocation composition will follow in a later patch.

Patch By: Daniel Sanders

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 80438.Dec 6 2016, 9:27 AM
sdardis retitled this revision from to [mips][rtdyld] Move MIPS relocation resolution to a subclass and implement N32 relocations.
sdardis updated this object.
sdardis added a reviewer: atanasyan.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
atanasyan added inline comments.Dec 6 2016, 2:18 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
15 ↗(On Diff #80438)

Unsorted include directives.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h
88 ↗(On Diff #80438)

Are this and similar changes below mandatory for this patch? Maybe move them into separate commit?

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.cpp
1 ↗(On Diff #80438)

s/h/cpp/

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.h
20 ↗(On Diff #80438)

Is it really necessary to move MIPS specific methods into the separate class? What is intention?

sdardis updated this revision to Diff 81087.Dec 12 2016, 8:01 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.
sdardis marked 2 inline comments as done.

Addressed review comments.

sdardis added inline comments.Dec 12 2016, 8:01 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h
88 ↗(On Diff #80438)

This and the changes below expose the minimum number of class members for use in a subclass of RuntimeDyldELF which this patch implements.

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.h
20 ↗(On Diff #80438)

The intention here in creating a separate class to handle MIPS relocations is to simplify resolving N32 relocations in a later patch. Factoring out some of the MIPS specific methods is similar to how other architecture+exe format combinations handle differences.

atanasyan accepted this revision.Dec 12 2016, 12:52 PM
atanasyan edited edge metadata.

LGTM

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.h
20 ↗(On Diff #80438)

I'm concerned a bit that the only target requires RuntimeDyldELF successor is MIPS. But I do not have any formal objections on this change.

This revision is now accepted and ready to land.Dec 12 2016, 12:52 PM
This revision was automatically updated to reflect the committed changes.