[PATCH] [LLD] Add a new PE/COFF port

[David Majnemer]

Some first round comments.

We should open archive files if you have symbols marked `IMAGE_WEAK_EXTERN_SEARCH_LIBRARY`.


================
Comment at: COFF/InputFiles.cpp:31
@@ +30,3 @@
+
+static StringRef basename(StringRef Path) {
+  size_t Pos = Path.rfind('\\');
----------------
Can we chose a better name? This overloads with basename(3).

================
Comment at: COFF/InputFiles.cpp:38
@@ +37,3 @@
+
+std::string InputFile::getShortName() {
+  StringRef Name = getName();
----------------
Can we not make this a member function?

================
Comment at: COFF/InputFiles.cpp:203-205
@@ +202,5 @@
+  }
+  if (Sym.getSectionNumber() == -1) {
+    return new (Alloc) DefinedAbsolute(Name, Sym.getValue());
+  }
+  if (Sym.isWeakExternal()) {
----------------
We should have an `isAbsolute()` and it should use `COFF::IMAGE_SYM_ABSOLUTE`.

================
Comment at: COFF/InputFiles.cpp:206
@@ +205,3 @@
+  }
+  if (Sym.isWeakExternal()) {
+    auto *Aux = (const coff_aux_weak_external *)AuxP;
----------------
Should we follow this path if we see a `IMAGE_WEAK_EXTERN_SEARCH_ALIAS` symbol?

================
Comment at: COFF/README.md:6
@@ +5,3 @@
+format. Because the fundamental design of this port is different from
+the other ports of the LLD, this port is separated to this directory.
+
----------------
s/the LLD/LLD

================
Comment at: COFF/README.md:9-10
@@ +8,4 @@
+The other ports are based on the Atom model, in which symbols and
+references are represented as vertices and edges of graphs. The port
+in this directory is on the other hand based on sections. The aim is
+simplicity and better performance. Our plan is to implement a linker
----------------
Perhaps we should prefer "instead" over "on the other hand" in this instance.

================
Comment at: COFF/README.md:31-34
@@ +30,6 @@
+  etc. Undefined symbols are for undefined symbols, which need to be
+  replaced by Defined symbols by the resolver. CanBeDefined symbols
+  represent symbols we found in archive file headers -- which can
+  turn into Defined symbols if we read archieve members, but we
+  haven't done that yet.
+
----------------
I think `ArchiveSymbol` might be a better fit than `CanBeDefined`.

================
Comment at: COFF/README.md:82
@@ +81,3 @@
+
+  OutputSection is a container of Chunks. A Chunk belong to at most
+  one OutputSection.
----------------
belongs

================
Comment at: COFF/Symbols.cpp:35-41
@@ +34,9 @@
+  // Common symbols are weaker than other types of defined symbols.
+  if (X->isCommon() && Y->isCommon())
+    return (X->getCommonSize() < Y->getCommonSize()) ? -1 : 1;
+  if (X->isCommon())
+    return -1;
+  if (Y->isCommon())
+    return 1;
+
+  if (X->isCOMDAT() && Y->isCOMDAT())
----------------
We should probably verify that MS LINK is OK with multiple definitions of a symbol where one of them is COMMON.  For example, consider a non-common DefinedRegular symbol whose size is smaller than a common DefinedRegular symbol.

================
Comment at: COFF/Symbols.h:48
@@ +47,3 @@
+    DefinedImportDataKind,
+    DefinedImportFuncKind,
+    UndefinedKind,
----------------
Perhaps `DefinedImportThunkKind`

================
Comment at: COFF/Symbols.h:93
@@ +92,3 @@
+    Kind K = S->kind();
+    return DefinedRegularKind <= K && K <= DefinedImportFuncKind;
+  }
----------------
If you are going to use comparisons here, please do it like LLVM does like `CmpInst::FIRST_ICMP_PREDICATE` and `CmpInst::LAST_ICMP_PREDICATE` by adding additional enumerators.

================
Comment at: COFF/Writer.cpp:54-55
@@ +53,4 @@
+void OutputSection::setFileOffset(uint64_t Off) {
+  if (Header.SizeOfRawData == 0)
+    return;
+  Header.PointerToRawData = Off;
----------------
This needs a comment.

================
Comment at: COFF/Writer.cpp:77
@@ +76,3 @@
+
+static StringRef dropDollar(StringRef S) { return S.substr(0, S.find('$')); }
+
----------------
You could do `S.split('$').first`

http://reviews.llvm.org/D10036

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/