[PATCH] D11188: [LLD] New ELF implementation
Rui Ueyama
ruiu at google.com
Tue Jul 14 14:14:13 PDT 2015
ruiu added a comment.
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.
================
Comment at: ELF/CMakeLists.txt:17
@@ +16,3 @@
+ Core
+ LTO
+ MC
----------------
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.
================
Comment at: ELF/Chunks.cpp:27
@@ +26,3 @@
+template <class ELFT>
+SectionChunk<ELFT>::SectionChunk(elfv2::ObjectFile<ELFT> *F, const Elf_Shdr *H,
+ uint32_t SI)
----------------
You can remove "elfv2::".
================
Comment at: ELF/Chunks.h:20
@@ +19,3 @@
+namespace lld {
+namespace elfv2 {
+
----------------
I'd name elf2 instead of elfv2 because it's shorter and consistent with lldELF2.
================
Comment at: ELF/Driver.cpp:53-58
@@ +52,8 @@
+
+// Drop directory components and replace extension with ".exe".
+static std::string getOutputPath(StringRef Path) {
+ auto P = Path.find_last_of("\\/");
+ StringRef S = (P == StringRef::npos) ? Path : Path.substr(P + 1);
+ return (S.substr(0, S.rfind('.')) + ".exe").str();
+}
+
----------------
Let's remove this function and make -o option mandatory for now. This function is really Windows-specific.
================
Comment at: ELF/Driver.cpp:84-103
@@ +83,22 @@
+
+// Find file from search paths. You can omit ".obj", this function takes
+// care of that. Note that the returned path is not guaranteed to exist.
+StringRef LinkerDriver::doFindFile(StringRef Filename) {
+ bool hasPathSep = (Filename.find_first_of("/\\") != StringRef::npos);
+ if (hasPathSep)
+ return Filename;
+ bool hasExt = (Filename.find('.') != StringRef::npos);
+ for (StringRef Dir : SearchPaths) {
+ SmallString<128> Path = Dir;
+ llvm::sys::path::append(Path, Filename);
+ if (llvm::sys::fs::exists(Path.str()))
+ return Alloc.save(Path.str());
+ if (!hasExt) {
+ Path.append(".obj");
+ if (llvm::sys::fs::exists(Path.str()))
+ return Alloc.save(Path.str());
+ }
+ }
+ return Filename;
+}
+
----------------
atanasyan wrote:
> s/obj/o/
Ditto
================
Comment at: ELF/Driver.cpp:127
@@ +126,3 @@
+// it returns None).
+Optional<StringRef> LinkerDriver::findLib(StringRef Filename) {
+ StringRef Path = doFindLib(Filename);
----------------
Same for findFile, doFindLib and findLib.
================
Comment at: ELF/Driver.cpp:136-142
@@ +135,9 @@
+bool LinkerDriver::link(llvm::ArrayRef<const char *> ArgsArr) {
+ // Needed for LTO.
+ llvm::InitializeAllTargetInfos();
+ llvm::InitializeAllTargets();
+ llvm::InitializeAllTargetMCs();
+ llvm::InitializeAllAsmParsers();
+ llvm::InitializeAllAsmPrinters();
+ llvm::InitializeAllDisassemblers();
+
----------------
If you remove LTO, you can remove these function calls.
================
Comment at: ELF/InputFiles.cpp:123
@@ +122,3 @@
+std::error_code elfv2::ObjectFile<ELFT>::initializeChunks() {
+ auto Size = ELFObj->getNumSections();
+ Chunks.reserve(Size);
----------------
Replace "auto" with its real type.
================
Comment at: ELF/InputFiles.cpp:127
@@ +126,3 @@
+ int I = 0;
+ for (auto &&Sec : ELFObj->sections()) {
+ if (isIgnoredSectionType(Sec.sh_type) || Sec.sh_addralign == 0) {
----------------
Ditto
================
Comment at: ELF/InputFiles.cpp:142
@@ +141,3 @@
+std::error_code elfv2::ObjectFile<ELFT>::initializeSymbols() {
+ auto Syms = ELFObj->symbols();
+ Syms = typename ELFFile<ELFT>::Elf_Sym_Range(Syms.begin() + 1, Syms.end());
----------------
Ditto
================
Comment at: ELF/InputFiles.cpp:148
@@ +147,3 @@
+ int I = 1;
+ for (auto &&Sym : Syms) {
+ SymbolBody *Body = createSymbolBody(&Sym);
----------------
Ditto
================
Comment at: ELF/SymbolTable.cpp:79-88
@@ +78,12 @@
+ continue;
+ if (SymbolBody *Alias = Undef->getWeakAlias()) {
+ Sym->Body = Alias->getReplacement();
+ if (!isa<Defined>(Sym->Body)) {
+ // Aliases are yet another symbols pointed by other symbols
+ // that could also remain undefined.
+ llvm::errs() << "undefined symbol: " << Undef->getName() << "\n";
+ Ret = true;
+ }
+ continue;
+ }
+ llvm::errs() << "undefined symbol: " << Undef->getName() << "\n";
----------------
Does this also make sense for ELF? If in doubt, I'd remove for now.
================
Comment at: ELF/SymbolTable.h:66-67
@@ +65,4 @@
+
+ // Rename From -> To in the symbol table.
+ std::error_code rename(StringRef From, StringRef To);
+
----------------
I'd remove this for now. This maybe Windows-specific, and we can bring it back if we need it.
================
Comment at: ELF/Symbols.h:197-201
@@ +196,7 @@
+
+ // An undefined symbol can have a fallback symbol which gives an
+ // undefined symbol a second chance if it would remain undefined.
+ // If it remains undefined, it'll be replaced with whatever the
+ // Alias pointer points to.
+ SymbolBody *getWeakAlias() { return Alias ? *Alias : nullptr; }
+
----------------
Remove this if you don't use this.
================
Comment at: ELF/Writer.cpp:26
@@ +25,3 @@
+
+#include "llvm/Support/raw_ostream.h"
+
----------------
Remove
================
Comment at: ELF/Writer.cpp:54-55
@@ +53,4 @@
+
+using namespace lld;
+using namespace lld::elfv2;
+
----------------
Move this above "static const int PageSize...".
================
Comment at: ELF/Writer.cpp:143-148
@@ +142,8 @@
+
+template <class ELFT> void Writer<ELFT>::removeEmptySections() {
+ auto IsEmpty = [](OutputSection *S) { return S->getSize() == 0; };
+ OutputSections.erase(
+ std::remove_if(OutputSections.begin(), OutputSections.end(), IsEmpty),
+ OutputSections.end());
+}
+
----------------
I think you don't actually need this function for ELF.
http://reviews.llvm.org/D11188
More information about the llvm-commits
mailing list