[PATCH] D11188: [LLD] New ELF implementation

[Simon Atanasyan]

atanasyan added a comment.

Just a few nits.

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


================
Comment at: ELF/Chunks.h:44-46
@@ +43,5 @@
+  // The writer sets and uses the addresses.
+  uint64_t getVA() { return VA; }
+  uint64_t getFileOff() { return FileOff; }
+  uint32_t getAlign() { return Align; }
+  void setVA(uint64_t V) { VA = V; }
----------------
Make these methods `const`

================
Comment at: ELF/Chunks.h:73-74
@@ +72,4 @@
+  // Used by the garbage collector.
+  bool isRoot() { return Root; }
+  bool isLive() { return Live; }
+  void markLive() {
----------------
Make these methods `const`

================
Comment at: ELF/Driver.cpp:57
@@ +56,3 @@
+  StringRef S = (P == StringRef::npos) ? Path : Path.substr(P + 1);
+  return (S.substr(0, S.rfind('.')) + ".exe").str();
+}
----------------
Does the `exe` extension has a sense for ELF file?

================
Comment at: ELF/Driver.cpp:84
@@ +83,3 @@
+
+// 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.
----------------
s/obj/o/

================
Comment at: ELF/Driver.cpp:97
@@ +96,3 @@
+    if (!hasExt) {
+      Path.append(".obj");
+      if (llvm::sys::fs::exists(Path.str()))
----------------
s/obj/o/

================
Comment at: ELF/Driver.cpp:120
@@ +119,3 @@
+  if (!hasExt)
+    Filename = Alloc.save(Filename + ".lib");
+  return doFindFile(Filename);
----------------
This code is not applicable for ELF

================
Comment at: ELF/Driver.cpp:124
@@ +123,3 @@
+
+// Resolves a library path. /nodefaultlib options are taken into
+// consideration. This never returns the same path (in that case,
----------------
/nodefaultlib is not applicable for ELF.

================
Comment at: ELF/Driver.cpp:135
@@ +134,3 @@
+
+bool LinkerDriver::link(llvm::ArrayRef<const char *> ArgsArr) {
+  // Needed for LTO.
----------------
The code below are from the COFF linker. Do we really need to commit it to ELF?

================
Comment at: ELF/InputFiles.cpp:33
@@ +32,3 @@
+// Returns the last element of a path, which is supposed to be a filename.
+static StringRef getBasename(StringRef Path) {
+  size_t Pos = Path.rfind('\\');
----------------
Is this code equal to `llvm::sys::path::filename`?

================
Comment at: ELF/InputFiles.cpp:137
@@ +136,3 @@
+  }
+  return std::error_code();
+}
----------------
Will this function return something else except "success" code?

================
Comment at: ELF/Symbols.h:166
@@ +165,3 @@
+// the same name, it will ask the Lazy to load a file.
+class Lazy : public SymbolBody {
+public:
----------------
I guess it is a COFF equivalent for ELF Weak symbol?


http://reviews.llvm.org/D11188