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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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;
----------------
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.


================
Comment at: include/llvm/Object/COFFImportFile.h:85
+  friend bool operator==(const COFFShortExport &L, const COFFShortExport &R) {
+    return (L.Name == R.Name && L.ExtName == R.ExtName &&
+            L.Ordinal == R.Ordinal && L.Noname == R.Noname &&
----------------
nit: remove ().


================
Comment at: include/llvm/Object/COFFImportFile.h:91
+  friend bool operator!=(const COFFShortExport &L, const COFFShortExport &R) {
+    return (L.Name != R.Name || L.ExtName != R.ExtName ||
+            L.Ordinal != R.Ordinal || L.Noname != R.Noname ||
----------------
Can you return `!(L == R)`?


================
Comment at: include/llvm/Object/DEFParser.h:31
+
+struct DEFInfo {
+  COFF::MachineTypes Machine = COFF::IMAGE_FILE_MACHINE_UNKNOWN;
----------------
I don't think `llvm::object::DEFInfo` is a good name. How about `llvm::object::COFFModuleDefinition`?


================
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;
----------------
Please make sure that you are using all these fields in the library as well.


================
Comment at: include/llvm/Object/DEFParser.h:46
+
+Expected<bool> parseModuleDefs(MemoryBufferRef MB,
+                     std::vector<llvm::object::COFFShortExport> *Exports,
----------------
I think you want to return `Expected<DEFInfo>`


================
Comment at: include/llvm/Object/DEFParser.h:47
+Expected<bool> parseModuleDefs(MemoryBufferRef MB,
+                     std::vector<llvm::object::COFFShortExport> *Exports,
+                     DEFInfo *Info);
----------------
This vector should be a member of `DEFInfo` as it is just part of a parse result.


================
Comment at: lib/Object/COFFImportFile.cpp:87-88
+  // Backfill the length of the table now that it has been computed.
+  support::ulittle32_t Length(B.size() - Offset);
+  memcpy(&B[Offset], &Length, sizeof(Length));
+}
----------------
Use `write32le`.


================
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 "";
----------------
You want to return `Expected<std::string>`.


================
Comment at: lib/Object/DEFParser.cpp:21
+#include "llvm/Object/DEFParser.h"
+#include "Memory.h"
+#include "llvm/ADT/StringRef.h"
----------------
Remove.


================
Comment at: lib/Object/DEFParser.cpp:57-58
+
+static BumpPtrAllocator BAlloc;
+static StringSaver Saver{BAlloc};
+
----------------
Remove.


================
Comment at: lib/Object/DEFParser.cpp:130
+public:
+  explicit Parser(StringRef S, std::vector<COFFShortExport> *E,
+                  DEFInfo *I) : Lex(S), Exports(E), Info(I) {}
----------------
Remove `explicit`.


================
Comment at: lib/Object/DEFParser.cpp:167
+
+  Expected<bool> parseOne() {
+    read();
----------------
Replace all occurrences of `Expected<bool>` with `Error`.


================
Comment at: lib/Object/DEFParser.cpp:247
+      if (Tok.K == KwConstant) {
+        setWarn("CONSTANT keyword is obsolete; use DATA");
+        E.Constant = true;
----------------
Remove this.


================
Comment at: tools/lld/COFF/Config.h:69
 
-  llvm::COFF::MachineTypes Machine = IMAGE_FILE_MACHINE_UNKNOWN;
+  DEFInfo Info;
   bool Verbose = false;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D32689