[PATCH] D32689: DEF: migrate def parser from LLD to LLVM

[Martell Malone via Phabricator via llvm-commits]

martell marked 13 inline comments as done.
martell added inline comments.


================
Comment at: include/llvm/Object/COFFImportFile.h:73-80
+  StringRef Name;
+  StringRef ExtName;
+
+  uint16_t Ordinal = 0;
+  bool Noname = false;
+  bool Data = false;
+  bool Private = false;
----------------
ruiu wrote:
> Please make sure that you are actually using all these variables in the library. If some of them are used only by LLD, move them to LLD.
All of these fields are used by `writeImportLibrary`.
I left any unnecessary variables in the `Export` Struct in `Config.h` of LLD 


================
Comment at: include/llvm/Object/DEFParser.h:32-43
+  COFF::MachineTypes Machine = COFF::IMAGE_FILE_MACHINE_UNKNOWN;
+  COFF::WindowsSubsystem Subsystem = COFF::IMAGE_SUBSYSTEM_UNKNOWN;
+  std::string OutputFile;
+  uint64_t ImageBase = -1;
+  uint64_t StackReserve = 1024 * 1024;
+  uint64_t StackCommit = 4096;
+  uint64_t HeapReserve = 1024 * 1024;
----------------
ruiu wrote:
> Please make sure that you are using all these fields in the library as well.
Same as above but for `parseModuleDefs`


================
Comment at: include/llvm/Object/DEFParser.h:46
+
+Expected<bool> parseModuleDefs(MemoryBufferRef MB,
+                     std::vector<llvm::object::COFFShortExport> *Exports,
----------------
ruiu wrote:
> I think you want to return `Expected<DEFInfo>`
I will set all the default values to `-1` so we can tell which values we actually updated.
I changed the second argument to default to `nullptr` making it optional to override the default values when needed.



================
Comment at: lib/Object/COFFImportFile.cpp:111
+    // TODO: What do I do here with the lld style error?
+    //error(S + ": replacing '" + From + "' with '" + To + "' failed");
+    return "";
----------------
ruiu wrote:
> You want to return `Expected<std::string>`.
`Expected<StringRef>` looked a lot more elegant rather then converting twice.
Marked as done.


================
Comment at: lib/Object/DEFParser.cpp:57-58
+
+static BumpPtrAllocator BAlloc;
+static StringSaver Saver{BAlloc};
+
----------------
ruiu wrote:
> Remove.
How would you suggest I allocate the StringRef when decorating `E.Name` and `E.ExtName`?


================
Comment at: tools/lld/COFF/Config.h:69
 
-  llvm::COFF::MachineTypes Machine = IMAGE_FILE_MACHINE_UNKNOWN;
+  DEFInfo Info;
   bool Verbose = false;
----------------
ruiu wrote:
> Instead of adding an entire struct as a member, please keep the original members.
> 
> If you add `DEFInfo` as a member like this, there's a chance that you will need to modify that struct just for LLD. But since LLD is just a user of the module-definition parser, LLD-specific changes shouldn't affect the parser.
The members only comprise of elements that the parser can change.
I will revert this as you wish though.


Repository:
  rL LLVM

https://reviews.llvm.org/D32689