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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Writer.cpp:1316
+         return (AS->Flags & SHF_ALLOC) && !(BS->Flags & SHF_ALLOC);
+    return false;
+  };
----------------
grimar wrote:
> 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;
> ```
Please ignore my comment above. Your version should be fine for `stable_sort`. I would change it a bit though:

```
    if (auto *A = dyn_cast<OutputSection>(ACmd))
      if (auto *B = dyn_cast<OutputSection>(BCmd))
         if ((A->Flags & SHF_ALLOC) != (B->Flags & SHF_ALLOC))
           return ((A->Flags & SHF_ALLOC) != 0)
    return false;
```


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50688