[PATCH] D45260: COFF: Layout sections in the same order as link.exe

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Writer.cpp:648-649
+static int sectionIndex(StringRef Name) {
+  const char *Order[] = {".text",  ".bss.",  ".rdata", ".data",
+                         ".pdata", ".idata", ".reloc"};
+
----------------
std::vector<StringRef> is perhaps better.


================
Comment at: COFF/Writer.cpp:651
+
+  for (size_t i = 0, e = llvm::array_lengthof(Order); i < e; i++)
+    if (Name == Order[i])
----------------
i -> I
e -> E


================
Comment at: COFF/Writer.cpp:670
+  // Put the sections in an order that matches what link.exe does. In
+  // particular, .rsrc must come after .text, otherwise the entry point could
+  // end up referring to garbage after .rsrc is updated with UpdateResource and
----------------
pcc wrote:
> pcc wrote:
> > What if I have a `.rsrc` section as well as a section whose name does not appear in the list above? It seems like this could still lay out `.rsrc` before my section, which would break things.
> > 
> > To me it seems like the correct fix would be to just move `.rsrc` to the end.
> That is what D45273 does.
I think I agree with Peter about ".rsrc" section ordering, but I believe sorting all output sections in the same order as MSVC does is beneficial if there's no reason to keep the current section ordering. Changing section ordering shouldn't affect anything, but in reality it should reduce "surprise" like this.


https://reviews.llvm.org/D45260