[PATCH] D48146: ELF: Implement --icf=safe using address-significance tables.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the patches. Out of curiosity did you have any thoughts about how the proposal to ELF was going? https://groups.google.com/forum/#!topic/generic-abi/MPr8TVtnVn4 . In particular compressed SHT_SYMATTR seemed promising.



================
Comment at: lld/ELF/ICF.cpp:180
     return false;
 
   // Don't merge synthetic sections as their Data member is not valid and empty.
----------------
If I've read this right, we merge read-only sections when ICFLevel::Safe but not when ICFLevel::All?

This seems a bit non-intuitive based on the expectation that All will be more aggressive than Safe. On the assumption that the address significance tables permit this to be done safely then would it make sense to do:
- ICFLevel = All + IgnoreDataAddressEquality = Don't use address significance tables and merge RO-data.
- ICFLevel = All = Use address significance tables for RO-data but not executable sections.
- ICFLevel = Safe = Use address significance tables for RO-data and executable sections. 


================
Comment at: lld/ELF/InputFiles.cpp:436
+        // in the address-significance table, which refers to symbols by index.
+        if (Sec.sh_link != 0)
+          this->AddrsigSec = &Sec;
----------------
In the discussions on llvm-dev http://lists.llvm.org/pipermail/llvm-dev/2018-May/123514.html there was some concern that just checking for non preservation of the sh_link field wouldn't catch cases from other ELF processing tools that might preserve sh_link but not the symbol table ordering. I don't think it is necessary to get this in at first, but it may be worth making sure we can add one later without breaking existing object files. Do you have any plans for a stronger check?


================
Comment at: lld/test/ELF/icf-safe.s:2
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
----------------
Some ideas for some more tests. I don't think that they are necessary for the patch but are areas that are at higher risk of getting broken by future changes:
- Include some symbols with STB_LOCAL binding.
- Include some symbols with STV_HIDDEN visibility.
- Include some symbols that would be exported into the dynamic symbol table for an executable.



https://reviews.llvm.org/D48146