[PATCH] D55293: [LLD][COFF] Unmerged sections

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Writer.cpp:162
+  NameAndChars(StringRef SN, uint32_t C)
+      : SectionName(SN), Characteristics(C) { }
+  StringRef SectionName;
----------------
Format.


================
Comment at: COFF/Writer.cpp:166
+
+  bool operator<(const NameAndChars &NC) const {
+    int C = SectionName.compare(NC.SectionName);
----------------
NC -> Other by convention.


================
Comment at: COFF/Writer.cpp:170
+      return false;
+    if (C == 0 && Characteristics >= NC.Characteristics)
+      return false;
----------------
nit: I'd stick with `<` when defining `operator<`. You can just return the result of the comparison of characteristics.

  if (C == 0)
   return Characteristics < NC.Characteritics.


================
Comment at: COFF/Writer.cpp:601
+    UnmergedSection *RDataSec = createUnmergedSection(USec->Name, RDATA);
+    RDataSec->Chunks.insert(RDataSec->Chunks.end(), USec->Chunks.begin(), USec->Chunks.end());
+    USec->Chunks.clear();
----------------
80 cols?

I recommend you use clang-format.


================
Comment at: COFF/Writer.cpp:701-702
   // Try to match the section order used by link.exe.
-  TextSec = CreateSection(".text", CODE | R | X);
-  CreateSection(".bss", BSS | R | W);
-  RdataSec = CreateSection(".rdata", DATA | R);
-  BuildidSec = CreateSection(".buildid", DATA | R);
-  DataSec = CreateSection(".data", DATA | R | W);
-  PdataSec = CreateSection(".pdata", DATA | R);
-  IdataSec = CreateSection(".idata", DATA | R);
-  EdataSec = CreateSection(".edata", DATA | R);
-  DidatSec = CreateSection(".didat", DATA | R);
-  RsrcSec = CreateSection(".rsrc", DATA | R);
-  RelocSec = CreateSection(".reloc", DATA | DISCARDABLE | R);
-  CtorsSec = CreateSection(".ctors", DATA | R | W);
-  DtorsSec = CreateSection(".dtors", DATA | R | W);
+  TextSec = CreateOutSection(".text", CODE | R | X);
+  CreateOutSection(".bss", BSS | R | W);
+  RdataSec = CreateOutSection(".rdata", DATA | R);
----------------
Why did you have to rename the function?


================
Comment at: COFF/Writer.h:28-38
+// UnmergedSection represents all unmerged yet sections contributing to the
+// output file, including synthetic ones.
+class UnmergedSection {
+public:
+  UnmergedSection(llvm::StringRef N, uint32_t Chars)
+      : Name(N), Characteristics(Chars) {}
+  llvm::StringRef Name;
----------------
Since no one except one .cpp file uses this class declaration, you don't need to write it in a header. Please move to the .cpp file.


================
Comment at: COFF/Writer.h:30
+// output file, including synthetic ones.
+class UnmergedSection {
+public:
----------------
It feels that UnmergedSection is still a bit weird name. You don't remove members from "Unmerged" once they are "merged" right?


================
Comment at: COFF/Writer.h:37
+  std::vector<Chunk *> Chunks;
+  OutputSection *TargetSection = nullptr;
+};
----------------
You have an unused member variable.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55293/new/

https://reviews.llvm.org/D55293