Page MenuHomePhabricator

[LLD] New ELF implementation
ClosedPublic

Authored by Bigcheese on Jul 14 2015, 11:27 AM.

Details

Summary

This is a direct port of the new PE/COFF linker to ELF. It can link simple object files, but doesn't do relocations yet.

The plan for this is to implement gnu-ld semantics and see what code can be shared with the new PE/COFF implementation, if any.

Event Timeline

Bigcheese updated this revision to Diff 29688.Jul 14 2015, 11:27 AM
Bigcheese retitled this revision from to [LLD] New ELF implementation.
Bigcheese updated this object.
Bigcheese added a subscriber: llvm-commits.
atanasyan edited edge metadata.Jul 14 2015, 1:36 PM

Just a few nits.

The patch contains some code copied from COFF especially in the driver. Do you plan to cleanup it before commit?

ELF/Chunks.h
45–47 ↗(On Diff #30433)

Make these methods const

74–75 ↗(On Diff #30433)

Make these methods const

ELF/Driver.cpp
58 ↗(On Diff #30433)

Does the exe extension has a sense for ELF file?

85 ↗(On Diff #30433)

s/obj/o/

98 ↗(On Diff #30433)

s/obj/o/

121 ↗(On Diff #30433)

This code is not applicable for ELF

125 ↗(On Diff #30433)

/nodefaultlib is not applicable for ELF.

136 ↗(On Diff #30433)

The code below are from the COFF linker. Do we really need to commit it to ELF?

ELF/InputFiles.cpp
34 ↗(On Diff #30433)

Is this code equal to llvm::sys::path::filename?

138 ↗(On Diff #30433)

Will this function return something else except "success" code?

ELF/Symbols.h
167 ↗(On Diff #30433)

I guess it is a COFF equivalent for ELF Weak symbol?

ruiu edited edge metadata.Jul 14 2015, 2:14 PM

I want you to strip this patch even more. Especially I'd like you to remove LTO completely from this initial patch. LTO code is fairly large but totally dead and untested. We can restore that when we actually start working on that.

I'd also want you to remove Windows-specific code as I left comments.

Overall, the code looks good -- if I'm eligible to say that. This is really a direct translation of my COFF linker to ELF, and of course I didn't find any difficulty to read and understand the code. The duplication of code with COFF is a bit concerning, but I'd imagine that if we try to merge this with COFF, it would become very messy because so many little things are different in many places. This is what I learned from the old LLD. We'd have eventually had to add tons of hooks and virtual function calls everywhere. So I think this is practical approach to port the COFF linker to ELF. We should keep COFF and ELF in sync as much as possible, and factor out common code to libObject or other LLVM library to remove code from COFF or ELF directories.

Regarding Windows-ness of the COFF linker which has been directly copied to this ELF port. The symbol resolution semantics in particular is not compatible with ELF. As I described in COFF/README.md, the symbol table and SymbolBodies implement Windows symbol resolution semantics. As a result, this ELF linker would behave as if all input files are wrapped with --start-group and --end-group. This would be more efficient than the regular Unix linker because the Unix semantics is too dumb, but it's not fully compatible with other linkers (although it's mostly compatible IMO). So there's a tradeoff between "new, efficient but incompatible way" and "dumb but compatible way".

I'm totally on the side to keep this new semantics in this ELF linker. --start-gropu and --end-group is a hack to deal with Unix linker's dumb semantics, and if we can replace that with better semantics, we should do that using this chance to rewrite a ELF linker. This is much better than before, and it open the possibility of significant optimization. I also think that we can implement backward-compatible mode to this ELF linker without too much trouble. My ambition is that ultimately we would provide "better and mostly compatible" and "slow and fully compatible" options to the user and let users to choose the former. I imagine that that can even be a strong reason to choose LLD over other ELF linkers in the future.

ELF/CMakeLists.txt
18 ↗(On Diff #30433)

Can you remove LTO from this patch? Unless we start working on LTO right now, it's going to be dead code. I'd like you to strip down as much as you can.

ELF/Chunks.cpp
28 ↗(On Diff #30433)

You can remove "elfv2::".

ELF/Chunks.h
21 ↗(On Diff #30433)

I'd name elf2 instead of elfv2 because it's shorter and consistent with lldELF2.

ELF/Driver.cpp
54–59 ↗(On Diff #30433)

Let's remove this function and make -o option mandatory for now. This function is really Windows-specific.

85–104 ↗(On Diff #30433)

Ditto

128 ↗(On Diff #30433)

Same for findFile, doFindLib and findLib.

137–143 ↗(On Diff #30433)

If you remove LTO, you can remove these function calls.

ELF/InputFiles.cpp
124 ↗(On Diff #30433)

Replace "auto" with its real type.

128 ↗(On Diff #30433)

Ditto

143 ↗(On Diff #30433)

Ditto

149 ↗(On Diff #30433)

Ditto

ELF/SymbolTable.cpp
80–89 ↗(On Diff #30433)

Does this also make sense for ELF? If in doubt, I'd remove for now.

ELF/SymbolTable.h
67–68 ↗(On Diff #30433)

I'd remove this for now. This maybe Windows-specific, and we can bring it back if we need it.

ELF/Symbols.h
198–202 ↗(On Diff #30433)

Remove this if you don't use this.

ELF/Writer.cpp
27 ↗(On Diff #30433)

Remove

55–56 ↗(On Diff #30433)

Move this above "static const int PageSize...".

144–149 ↗(On Diff #30433)

I think you don't actually need this function for ELF.

joerg added a subscriber: joerg.Jul 14 2015, 3:07 PM

I don't understand how forcing everything to be grouped can make things more efficient, given that it is strictly required to do more work. It doesn't even matter for plain object files, just for libraries. In the case of libraries, there are subtle error cases involving weak symbols, so please do *not* change the resolution algorithm.

Bigcheese marked 18 inline comments as done.Jul 14 2015, 3:15 PM
Bigcheese added inline comments.
ELF/InputFiles.cpp
138 ↗(On Diff #30433)

It will. The intent is for this to be the one place we scan the entire section table, which can find broken files.

ELF/Symbols.h
167 ↗(On Diff #30433)

This is for archive files.

Bigcheese updated this revision to Diff 29721.Jul 14 2015, 3:15 PM
Bigcheese edited edge metadata.

Address review comments.

ELF/Chunks.cpp
28 ↗(On Diff #30433)

ObjectFile is also a name in llvm::object.

ruiu accepted this revision.Jul 14 2015, 3:42 PM
ruiu edited edge metadata.

LGTM

Please leave this for a while so that other people can review this patch as well.

ELF/Symbols.h
202 ↗(On Diff #30433)

You can now remove this member.

This revision is now accepted and ready to land.Jul 14 2015, 3:42 PM
davide accepted this revision.Jul 14 2015, 4:24 PM
davide edited edge metadata.

This seems good to me (assuming you'll remove all the COFF'isms)

rafael edited edge metadata.Jul 22 2015, 7:29 AM

I am currently rebasing this on trunk.

test/elfv2/basic.test
1 ↗(On Diff #29721)

Rename the directory to elf2

You can test this with llvm-mc. Please don't use yaml2obj when it is not needed.

A patch that builds on trunk is attached.

I have also fixed it to build when using shared libraries.

There is still a *lot* of stuff that should be deleted before this
goes in. Please update phab with this patch (and then change the test
to use llvm-mc).

Bigcheese updated this revision to Diff 30404.Jul 22 2015, 2:43 PM
Bigcheese edited edge metadata.

Rafael's changes.

Bigcheese updated this revision to Diff 30415.Jul 22 2015, 3:52 PM

Address review comments.

.s test.
Rename test dir.

Bigcheese marked an inline comment as done.Jul 22 2015, 3:53 PM
rafael added inline comments.Jul 22 2015, 4:21 PM
ELF/CMakeLists.txt
18 ↗(On Diff #30433)

Please delete the LTO dependency. I added it to CMakeList to fix the build, but it should be removed and the code changed to avoid the dependency.

ELF/Chunks.cpp
80 ↗(On Diff #30433)

You don't need this on the first patch.

85 ↗(On Diff #30433)

This test doesn't need common symbols. Delete this. It should be added in a followup patch. This is also copying over COFF specific code and comments.

ELF/Chunks.h
91 ↗(On Diff #30433)

Delete the garbage collector. It should be added in another patch and the terminology for ELF is different anyway.

ELF/DriverUtils.cpp
112 ↗(On Diff #30433)

Delete this. It should be added when we add a test that uses response files.

ELF/InputFiles.cpp
47 ↗(On Diff #30433)

Handle the error in here instead of returning it.

test/elf2/basic.test
8 ↗(On Diff #30433)

The type is not needed.

Bigcheese updated this revision to Diff 30433.Jul 22 2015, 5:30 PM
Bigcheese marked 6 inline comments as done.

Remove GC and LTO remnants.

This comment was removed by Bigcheese.

The attached patch looks good to me.

It is your patch, but I have:

  • removed code that had COFF logic.
  • removed untested code.
  • added a few more tests.
  • simplified error handling.
  • removed unused dependencies.

Even with the extra tests this patch adds 1255 lines. Yours was 1995.

Cheers,
Rafael

emaste added a subscriber: emaste.Jul 24 2015, 7:40 AM
This revision was automatically updated to reflect the committed changes.