[PATCH] D38180: [ELF] - ICF: improve support of SHF_LINK_ORDER sections.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: ELF/ICF.cpp:278
 bool ICF<ELFT>::equalsConstant(const InputSection *A, const InputSection *B) {
-  if (A->NumRelocations != B->NumRelocations || A->Flags != B->Flags ||
-      A->getSize() != B->getSize() || A->Data != B->Data)
+  if (!A || !B || A->NumRelocations != B->NumRelocations ||
+      A->Flags != B->Flags || A->getSize() != B->getSize() ||
----------------
ruiu wrote:
> You shouldn't make this change -- you can check it on the caller side.
OK.


================
Comment at: ELF/ICF.cpp:436-437
       Sections[Begin]->replace(Sections[I]);
+      if (InputSection *LinkOrder = Sections[I]->getLinkOrderSec())
+        log("  removed " + LinkOrder->Name);
     }
----------------
ruiu wrote:
> This is pretty odd piece of code. You do not call `replace` on LinkOrder section, but you are reporting that you have removed it. It looks buggy even if it is not buggy.
Fixed.


================
Comment at: ELF/InputSection.cpp:244
 
+InputSection *InputSectionBase::getLinkOrderSec() const {
+  InputSection *Ret = nullptr;
----------------
ruiu wrote:
> Why don't you just add a `LinkOrderSection *` member to InputSection?
If D38170 be landed, we will have `InputSection *LinkOrderSection;` in `InputSectionBase`, but that is different pointer.
For `SHF_LINK_ORDER` sections it will contain section linked to via `sh_link` flag. So for section `X` it returns section `L`.
This metod returns `SHF_LINK_ORDER` section. It is reverse, for `L` it returns `X`. 

Doesn't seem we should add one more member for that, because we already have `X` in `DependentSections` member for `L`.
And `DependentSections` should be little vector normally I think, traversing is cheap.


https://reviews.llvm.org/D38180