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

Rui Ueyama ruiu at google.com
Tue May 26 14:34:08 PDT 2015


Added folks who may be interested by this patch to the CC list.


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

================
Comment at: COFF/InputFiles.cpp:38
@@ +37,3 @@
+
+std::string InputFile::getShortName() {
+  StringRef Name = getName();
----------------
majnemer wrote:
> Can we not make this a member function?
This function uses ParentName and getName() defined in InputFile class, so maybe it's better to keep it as 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()) {
----------------
majnemer wrote:
> We should have an `isAbsolute()` and it should use `COFF::IMAGE_SYM_ABSOLUTE`.
Done.

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

================
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.
+
----------------
majnemer wrote:
> I think `ArchiveSymbol` might be a better fit than `CanBeDefined`.
As we discussed offline, "Archive" would conflict with libObject's Archive class, so let's rename "Lazy".

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

================
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())
----------------
majnemer wrote:
> 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.
Added a TODO.

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

================
Comment at: COFF/Symbols.h:93
@@ +92,3 @@
+    Kind K = S->kind();
+    return DefinedRegularKind <= K && K <= DefinedImportFuncKind;
+  }
----------------
majnemer wrote:
> 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.
Done.

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

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

http://reviews.llvm.org/D10036

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






More information about the llvm-commits mailing list