[PATCH] [lld] Atoms created from command line options should be available in YAML

[Shankar Kalpathi Easwaran]

================
Comment at: include/lld/Core/LinkingContext.h:193
@@ -193,3 +192,3 @@
+  virtual void setEntrySymbolName(StringRef name) {
     // Entry function have to be resolved as an undefined symbol.
     _entrySymbolName = name;
----------------
Rui Ueyama wrote:
> Remove this comment as this was for the next line you just removed.
ok.

================
Comment at: include/lld/Core/LinkingContext.h:250
@@ +249,3 @@
+  /// the actual input files.
+  std::vector<std::unique_ptr<lld::File>> createInternalFiles();
+
----------------
Rui Ueyama wrote:
> You may want to make it virtual?
Yes. 

================
Comment at: include/lld/ReaderWriter/ELFLinkingContext.h:176
@@ +175,3 @@
+  /// Add internal files for ELF
+  virtual std::vector<std::unique_ptr<File> > createInternalFiles();
+
----------------
Rui Ueyama wrote:
> >>
clang-format makes it this way.

================
Comment at: lib/Core/LinkingContext.cpp:58
@@ +57,3 @@
+  std::unique_ptr<SimpleFile> simpleFile(
+      new SimpleFile(*this, "command line option -u"));
+  for (auto ai : _initialUndefinedSymbols)
----------------
Rui Ueyama wrote:
> It's not always -u. Link.exe's option is /c.
will copy over the functionality to PECOFFLinkingContext.

================
Comment at: lib/Core/LinkingContext.cpp:59
@@ +58,3 @@
+      new SimpleFile(*this, "command line option -u"));
+  for (auto ai : _initialUndefinedSymbols)
+    simpleFile->addAtom(
----------------
Rui Ueyama wrote:
> Is ai atom, symbol or string? I'd write the type explicitly rather than using auto because the type declaration of this data is too far from this. It's even in a different file.
ok.

================
Comment at: lib/Core/LinkingContext.cpp:64
@@ +63,3 @@
+  StringRef entrySymbol = entrySymbolName();
+  if (!entrySymbol.empty()) {
+    std::unique_ptr<SimpleFile> entryFile(
----------------
Rui Ueyama wrote:
> We prefer early return (I remember this was noted somewhere in the LLVM coding standard), so it should be if (entrySymbol.empty()) return result;.
ok.

================
Comment at: lib/Driver/LDOptions.td:105
@@ -104,2 +104,3 @@
 
+
 // extensions
----------------
Rui Ueyama wrote:
> Is this intended?
will remove blank lines.

================
Comment at: lib/ReaderWriter/ELF/File.h:754
@@ +753,3 @@
+  virtual Atom *addAbsoluteAtom(StringRef symbolName) {
+    Elf_Sym *symbol = new (_allocator.Allocate<Elf_Sym>()) Elf_Sym;
+    symbol->st_name = 0;
----------------
Rui Ueyama wrote:
> new (_allocator) suffices?
Yes. Thanks for noting this.

================
Comment at: lib/ReaderWriter/ELF/File.h:762
@@ +761,3 @@
+    auto *newAtom =
+        new (_allocator) ELFAbsoluteAtom<ELFT>(*this, symbolName, symbol, -1);
+    _absoluteAtoms._atoms.push_back(newAtom);
----------------
Rui Ueyama wrote:
> What does -1 mean?
By default absolute symbols have a value 0xfffffff, which -1 sets it to.

================
Comment at: include/lld/Driver/InputGraph.h:79
@@ +78,3 @@
+    for (auto &ai : inputFiles)
+      _internalFiles.push_back(std::move(ai));
+  }
----------------
Rui Ueyama wrote:
> Probably doing this is better to add a vector to another vector.
> 
>  _internalFiles.insert(_internalFiles.end(), inputFiles.begin(), inputFiles.end())
Unfortunately this will not work, as I need to explicitly move each unique_ptr to the new vector.


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