[PATCH] D11188: [LLD] New ELF implementation

[Rui Ueyama]

On Tue, Jul 14, 2015 at 1:36 PM, Simon Atanasyan <simon at atanasyan.com>
wrote:

> 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`
>

I'd rather not mark them as const because in the COFF linker we didn't do
that. I'd like to keep them in sync as much as possible. We can make both
const in a followup patch.


> ================
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150714/8f6e41ab/attachment.html>