[PATCH] D71163: [ELF] --icf: do not fold preemptible symbols

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: lld/ELF/ICF.cpp:262
 
+    if (da->isPreemptible || db->isPreemptible)
+      return false;
----------------
MaskRay wrote:
> I may place a comment that replicates the patch summary here
> 
> > When comparing a pair of relocations, if they refer to different symbols, and either symbol is preemptible, the containing sections should be considered different.
> 
> If you have a wording suggestion, please tell me:)
Only addition I'd make is a brief comment on why they should be different.
```
When comparing a pair of relocations, if they refer to different symbols, and either symbol is preemptible, the containing sections should be considered different. This is because even if the sections are identical in this DSO, they may not be after preemption.
```
    


================
Comment at: lld/test/ELF/icf-preemptible.s:19
+# NONLEAF:        removing identical section {{.*}}:(.text.g2)
+# LEAF-NEXT:      removing identical section {{.*}}:(.text.g3)
+
----------------
grimar wrote:
> Not sure `LEAF` and `NONLEAF` are clear to me.
> I have no suggestions about how to name them better though.
> It seems a bit hard to invent good names when checks are mixed.
> If they were separated you could use just "DSO" and "EXE" probably.
> May be others will have any ideas.
I'm clear about the meaning of LEAF and NONLEAF, although I suppose we could add a comment to make it clearer. Having said that it would be worth exploring George's suggestion to have a DSO and EXE case, it would be easier to map to the definitions of f and g as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71163/new/

https://reviews.llvm.org/D71163