[PATCH] [lld] Registry model for Readers and Reference Kind strings

[Rui Ueyama]

  LGTM with a few comments.

  There are format errors in this patch, but I don't think pointing every one of them does not make sense as this patch is huge. :) So please run clang-format on this patch before submitting.


================
Comment at: include/lld/ReaderWriter/CoreLinkingContext.h:32
@@ +31,3 @@
+    TEST_RELOC_LEA32_WAS_GOT = 5,
+  };
+  
----------------
Are these fields for testing? If so, is there any way to move this to a unit test file and remove them from this file?

================
Comment at: include/lld/ReaderWriter/YamlContext.h:40
@@ +39,3 @@
+  File                  *_file;
+  NormalizedFile        *_normalizeMachOFile;
+};
----------------
Why mach-o file only? If we need it, it looks weird that we don't have files for other formats.

================
Comment at: lib/Driver/GnuLdDriver.cpp:339
@@ -328,1 +338,3 @@
       }
+      // FIXME: Calling getFileMagic() is is expensive.  It would be better to
+      // wire up the LdScript parser into the registry.
----------------
Shankar Kalpathi Easwaran wrote:
> Agree. Perfect place.
nit: s/is is/is/

================
Comment at: lib/Driver/WinLinkDriver.cpp:654
@@ +653,3 @@
+  context.registry().addSupportNativeObjects();
+  context.registry().addSupportYamlFiles();
+
----------------
This code looks pretty clean. Nice!

================
Comment at: lib/Passes/LayoutPass.cpp:73
@@ -72,4 +72,3 @@
     for (const Reference *ref : *atom) {
-      llvm::dbgs() << "    " << ref->kindToString()
-                   << ": " << atomToDebugString(ref->target()) << "\n";
+      llvm::dbgs() << ": " << atomToDebugString(ref->target()) << "\n";
     }
----------------
Does it make sense to start a debug output line with ":"?

================
Comment at: lib/Passes/RoundTripNativePass.cpp:47
@@ +46,3 @@
+  const File *obj = dyn_cast<const File>(objFile);
+  assert(obj && "yaml generated file is not an relocatable file");
+  mergedFile.reset(new FileToMutable(_context, *obj));
----------------
s/yaml/native/

================
Comment at: lib/ReaderWriter/ELF/File.h:17
@@ -16,2 +16,3 @@
 #include "lld/Core/Reference.h"
+
 #include "lld/ReaderWriter/ELFLinkingContext.h"
----------------
Remove this empty line.

================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:119
@@ +118,3 @@
+    std::size_t maxAlignment =
+              1ULL << llvm::countTrailingZeros(uintptr_t(mb->getBufferStart()));
+    auto f = createELF<DynamicFileCreateELFTraits>( 
----------------
This is not new code, but it does not seem to be the right way to calculate an alignment from the buffer start address. If the start address happens to be aligned on a large alignment, maxAlignment will become a large number for no reason. We should probably revisit this after submitting this patch.

================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:126
@@ +125,3 @@
+    std::unique_ptr<File> f2(f->release());
+    result.push_back(std::move(f2));
+    return error_code::success();
----------------
Can we do this as push_back(std::unique_ptr<File>(f->release)), or just push_back(std::move(f))?

================
Comment at: lib/ReaderWriter/PECOFF/ReaderImportHeader.cpp:309
@@ +308,3 @@
+        memcmp(buf, "\0\0\xFF\xFF", 4))
+      return make_error_code(NativeReaderError::unknown_file_format);
+
----------------
I think you can remove this file magic check as it's already been checked.

================
Comment at: lib/ReaderWriter/Reader.cpp:33
@@ +32,3 @@
+  llvm::sys::fs::file_magic fileType = llvm::sys::fs::identify_magic(content);
+  // Get file extension.
+  StringRef extension = "";
----------------
You can use extension(StringRef) declared in llvm/include/llvm/Support/Path.h.

================
Comment at: lib/ReaderWriter/Reader.cpp:46
@@ +45,3 @@
+    }
+  }
+  
----------------
These brackets are not needed.


http://llvm-reviews.chandlerc.com/D2431

BRANCH
  svn

ARCANIST PROJECT
  lld