Page MenuHomePhabricator

Initial ELF/AArch64 Support
ClosedPublic

Authored by stewartd on Aug 4 2014, 7:33 AM.

Details

Summary

This adds the initial ELF/AArch64 support to lld. Only basic a "Hello World" app has been successfully tested for both dynamic and static compiling.

Diff Detail

Event Timeline

stewartd updated this revision to Diff 12160.Aug 4 2014, 7:33 AM
stewartd retitled this revision from to Initial ELF/AArch64 Support.
stewartd updated this object.
stewartd edited the test plan for this revision. (Show Details)
stewartd added subscribers: Unknown Object (MLST), lld.
shankarke accepted this revision.Aug 4 2014, 11:20 AM
shankarke added a reviewer: shankarke.
shankarke added a subscriber: shankarke.

LGTM with tests.

This revision is now accepted and ready to land.Aug 4 2014, 11:20 AM
garious edited edge metadata.Aug 4 2014, 5:33 PM

Thanks for doing this! And I appreciate the detailed list of unimplemented features. :)

LGTM with tests.

atanasyan accepted this revision.Aug 4 2014, 10:39 PM
atanasyan added a reviewer: atanasyan.
atanasyan added a subscriber: atanasyan.

LGTM with tests and 80-column fixups.

rengolin edited edge metadata.Aug 5 2014, 1:20 AM

Hi Daniel,

I'm not an expert on linkers neither AARch64, but I agree that this is a great starting point. Thanks for working on this!

As Shankar said, tests would be good, following what's already in lld for other archs. Even if you can't link more than hello world, you do have some features in, and linking / disassembling / FileCheck'ing wouldn't be that hard to implement for a few basic cases. It would be good, however, to stress each part of the code more thoroughly with time, and since this is brand new code, it shouldn't take too long to go in, or we'll forget, and then ignore.

With tests and the comments addressed by others and me, I think this looks good to me, too.

cheers,
--renato

lib/ReaderWriter/ELF/AArch64/AArch64RelocationHandler.cpp
33

Forgot debug for this one, and some others. Is that intentional?

353

If I got it right, the ones that have debug messages here are not the ones that have it up there. Since we're mostly just calling them in the switch, why not move them all into one place, ie. the functions up there?

rengolin set the repository for this revision to rL LLVM.
rengolin added a project: lld.

Thanks for the comments. I'll try to add some tests and resubmit a modified patch.

lib/ReaderWriter/ELF/AArch64/AArch64RelocationHandler.cpp
353

You're right, the debug messages should all be moved up into the actual relocation functions. I'll move them all up there and ensure all relocation functions have a debug message.

ruiu accepted this revision.Aug 5 2014, 3:07 PM
ruiu added a reviewer: ruiu.
ruiu added a subscriber: ruiu.

I'm not an expert of ARM64 so maybe I'm just scratching the surface, but here are my comments.

lib/ReaderWriter/ELF/AArch64/AArch64ExecutableWriter.h
23

Indentation error? Please run clang-format on the files.

27

I think this function overrides ExecutableWriter::createImplicitFiles. If a member function overrides, don't write "virtual" but add "override". That applies to other functions too.

lib/ReaderWriter/ELF/AArch64/AArch64RelocationHandler.cpp
67

Debug leftover?

stewartd updated this revision to Diff 12326.Aug 9 2014, 2:08 PM
stewartd edited edge metadata.

I added test cases (using Hexagon and X86_64 as guides) and corrected formatting issues (clang-format + 80 column). I also addressed the issue of adding override where needed.

I'll be upstreaming this shortly.

stewartd updated this revision to Diff 12409.Aug 12 2014, 11:33 AM

Added the AArch64ELFTarget.a library to the unittests/DriverTests/Makefile. Thanks to Chad for alerting me that it didn't build under make, only ninja.

mcrosier closed this revision.Aug 13 2014, 6:27 AM

Committed as r215544.