[PATCH] D13275: [lld][elf2] Sort output sections.

[Michael Spencer via llvm-commits]

Bigcheese marked an inline comment as done.

================
Comment at: ELF/Writer.cpp:399
@@ +398,3 @@
+
+        auto rank = [](OutputSectionBase<ELFT::Is64Bits> *Sec) {
+          switch (Sec->getFlags() & (SHF_ALLOC | SHF_EXECINSTR | SHF_WRITE)) {
----------------
rafael wrote:
> No auto.
> Rank
> 
This is a lambda, it has an unutterable type.

================
Comment at: ELF/Writer.cpp:412
@@ +411,3 @@
+          default:
+            error("Unexpected section flags");
+          }
----------------
rafael wrote:
> There are 8 possible values.
> 
> The ones that are missing are 
> 
> SHF_EXECINSTR:
> SHF_WRITE:
> SHF_EXECINSTR | SHF_WRITE:
> 
> It might be more explicit to just list them all.
Explicitly list them as an error?

================
Comment at: ELF/Writer.cpp:416
@@ +415,3 @@
+
+        // Ignore SHT_NOBITS for non-allocated sections.
+        if (AFlags && AFlags == BFlags)
----------------
rafael wrote:
> Why is this relevant?
> 
> We need to put SHF_ALLOC first to reduce how much needs to be allocated and make sure debug info doesn't change actual code.
> 
> Other than that convertSectionFlagsToPHDRFlags (and therefore the creation of new PT_LOADs) only looks at SHF_WRITE and SHF_EXECINSTR, so looks like your rank function takes care of all cases.
> 
There's a testcase that creates a non allocated nobits section. If we don't ignore that, it ends up being placed after the section string table, which then causes an assert because the string table is finalized before the non allocated section's name is added.


http://reviews.llvm.org/D13275