[PATCH] D50688: [LLD] Sort alloc/non-alloc output sections in linkerscripts

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I think I agree with Peter that we either need to report an error or warning or to support this.
Since the complexity is probably the same, I am ok with this patch. (Though I think such scripts are just broken).



================
Comment at: ELF/Writer.cpp:1312
+  // non-alloc sections to come after the alloc sections.
+  auto isAllocSection = [](const BaseCommand *ACmd, const BaseCommand *BCmd) {
+    if (auto *AS = dyn_cast<OutputSection>(ACmd))
----------------
The name of variable should be uppercase. I think it should be `CompareXXX` for consistency with other predicates.


================
Comment at: ELF/Writer.cpp:1316
+         return (AS->Flags & SHF_ALLOC) && !(BS->Flags & SHF_ALLOC);
+    return false;
+  };
----------------
I am not sure the comparator is correct.
It should return true if the first argument is less than second.
But your version would always return false if you have 2 alloc sections.

I think it should be something like:


```
auto *A = dyn_cast<OutputSection>(ACmd);
auto *B = dyn_cast<OutputSection>(BCmd);
if (!A || !B)
  return false;

bool IsAllocA = A->Flags & SHF_ALLOC;
bool IsAllocB = B->Flags & SHF_ALLOC;
if (IsAllocA != IsAllocB)
  return IsAllocA;

return A->SectionIndex < B->SectionIndex;
```


================
Comment at: test/ELF/linkerscript/Inputs/nonalloc.s:10
+.short 10
+.byte 20
----------------
I think you do not need this input. You can use echo to inline it,
see for example:
https://github.com/llvm-mirror/lld/blob/master/test/ELF/linkerscript/bss-fill.test

I would expect something with less data instructions like
`echo '.section .text,"ax"; .quad 0; .section .data,"aw";  .long 0; .section .other,"";  .byte 0; '`


================
Comment at: test/ELF/linkerscript/nonalloc.test:30
+# CHECK:      filesz 0x000000000000000e memsz 0x000000000000000e flags r-x
\ No newline at end of file

----------------
Please add a new line.


================
Comment at: test/ELF/linkerscript/sections.s:29
 # Sections are put in order specified in linker script, other than alloc
 # sections going first.
 # RUN: echo "SECTIONS { \
----------------
This comment needs an update then.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50688