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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D48146#1132138, @peter.smith wrote:

> 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.


It does seem promising indeed, and as I mentioned on the thread I think I'd be in favour of that for standardisation, but I think that we should stick with a simple representation to start with since a full implementation of SHT_SYMATTR is beneficial only if it ends up being standardised (and then used for something else), and there's still no concensus as far as I can tell on what (if anything) we're going to do in the gABI.



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

Correct.

>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: [...]

I think that's exactly what the end state should be. I was going to do that in a followup since it would involve some rethinking of how `KeepUnique` and the `--keep-unique` flag interact.


================
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;
----------------
peter.smith wrote:
> 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?
It should be possible to add one later.  I think your proposal was to compute a hash of the `.symtab` and `.strtab` sections, and we can avoid changing the format by storing it in `sh_info`.


================
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
----------------
peter.smith wrote:
> 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.
> 
Will do.


https://reviews.llvm.org/D48146