Re: [PATCH 17/20] symtab/dwarf-reader: allow hinting of main symbols for aliases

[Dodji Seketeli <dodji@seketeli.org>]

Matthias Maennich <maennich@google.com> a écrit:

> In case of aliased symbols, the "main symbol" cannot be deduced from
> the symtab as this solely contains a name->addr mapping and aliases
> are represented by multiple names resolving to the same address.
> Therefore the main symbol can only be picked rather randomly and
> unpredictable.
>
> Unlike DWARF, which contains a single symbol entry for only the main
> symbol. Hence we can (late) detect the main symbol. Exploiting that
> property allows to correct the addr->symbol lookup in the symtab to
> return the correct main symbol and it also allows to correct the aliased
> symbols to maintain the correct main symbol.
>
> This patch adds the `update_main_symbol` functionality to `elf_symbol`
> to update the main symbol by name (ELF symbols need unique names) and
> adds `update_main_symbol` to `symtab` that makes use of said new method.
>
> When we discover a main symbol during DWARF reading, we instruct the
> symtab to update the mapping.
>
> This creates consistent representations across different builds of the
> same binary with the same ABI by pinning down the main symbol to the
> defined one. Knowing the main symbol also helps to keep the correct
> dwarf information in the representation in the presence of symbol
> suppressions. A later patch will address that.
>
> Some test cases in tests/data need adjustment and they have all been
> verified to be valid changes.
>
> - main symbol changed for various elf symbols
>   - test-annotate/test15-pr18892.so.abi
>   - test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi
>   - test-annotate/test3.so.abi
>   - test-read-dwarf/test15-pr18892.so.abi
>   - test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi
>   - test-read-dwarf/test3.so.abi
>   - test-read-dwarf/test3.so.hash.abi
>
> - due to main symbol changes, the symbol diff needs to be corrected
>   - test-diff-dwarf/test12-report.txt
>   - test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
>   - test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt
>
> - the test scenario needed adjustments as the main symbol changed
>   - test-diff-suppr/test23-alias-filter-4.suppr
>   - test-diff-suppr/test23-alias-filter-report-0.txt
>   - test-diff-suppr/test23-alias-filter-report-2.txt
>
> As usual, the complete changelog follows.

The patch itself looks good to me.

However, the part that update the tests won't apply anymore given the
cheer amount of changes that happen since the submission of this
patchset.

So I think you'll need to refresh them before being able to apply this
one.

>
> 	* include/abg-ir.h (elf_symbol::update_main_symbol): New method.
> 	* include/abg-symtab-reader.h (symtab::update_main_symbol): New method.
> 	* src/abg-dwarf-reader.cc
> 	(build_var_decl): Hint symtab about main symbol discovered in DWARF.
> 	(build_function_decl): Likewise.
> 	* src/abg-ir.cc (elf_symbol::get_main_symbol): Lock the weak_ptr
> 	  on access in both overloads.
> 	(update_main_symbol): New method to allow updating the main symbol.
> 	* src/abg-symtab-reader.cc (symtab::update_main_symbol): New method.
> 	* data/Makefile.am: Add new test data files.
> 	* tests/data/test-annotate/test15-pr18892.so.abi: Updated test file.
> 	* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi: Likewise.
> 	* tests/data/test-annotate/test2.so.abi: Likewise.
> 	* tests/data/test-annotate/test3.so.abi: Likewise.
> 	* tests/data/test-diff-dwarf/test12-report.txt: Likewise.
> 	* tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt: Likewise.
> 	* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-0.txt: Likewise.
> 	* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-0.txt: Likewise.
> 	* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt: Likewise.
> 	* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt: Likewise.
> 	* tests/data/test-diff-suppr/test23-alias-filter-4.suppr: Likewise.
> 	* tests/data/test-diff-suppr/test23-alias-filter-report-0.txt: Likewise.
> 	* tests/data/test-diff-suppr/test23-alias-filter-report-2.txt: Likewise.
> 	* tests/data/test-read-dwarf/PR22015-libboost_iostreams.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/PR22122-libftdc.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test11-pr18828.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test12-pr18844.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test15-pr18892.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test16-pr18904.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test2.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test2.so.hash.abi: Likewise.
> 	* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test3.so.abi: Likewise.
> 	* tests/data/test-read-dwarf/test3.so.hash.abi: Likewise.
> 	* tests/data/test-symtab/basic/aliases.c: New test source file.
> 	* tests/data/test-symtab/basic/aliases.so: Likewise.
> 	* tests/test-symtab.cc (Symtab::AliasedFunctionSymbols): New test case.
> 	(Symtab::AliasedVariableSymbols): Likewise.
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Matthias Maennich <maennich@google.com>

OK to apply to master after refreshing the test updates.

Thanks!

[...]

Cheers,

-- 
		Dodji