Re: [PATCH 18/20] dwarf-reader/writer: consider aliases when dealing with suppressions

[Dodji Seketeli <dodji@seketeli.org>]

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

> When a symbol is suppressed and it happens to be the main symbol of a
> group of aliased symbols where another symbol is not suppressed, the
> dwarf reader discards the DWARF information upon reading and the writer
> will not be able to connect dwarf information to the aliased elf symbol.

I think I understand what you mean here, but I'd rather say it this way
for the sake of avoiding confusion:

    When the symbol of a decl is suppressed and it happens to be the main
    symbol of a group of aliased symbols where another symbol is not
    suppressed, the dwarf reader discards the decl from the internal
    representation altogether upon reading and thus the writer will not be
    able to connect that decl to the non-suppressed aliased elf symbol.


To me, once the IR is built, there is more DWARF involved.  So the
writer doesn't deal with any DWARF anymore.  It deals with the IR.
Where that IR comes from is another matter.  It can come from DWARF,
abixml or maybe something else in the future.  That's why it can be
confusing.

OK, I am nit picking a little bit, but I think it's important to make
this clear.

> In order to address this, ensure we are not suppressing symbols
> (actually functions and variables) for which an alias is not suppressed.

Right, so you felt the need to talk about decls (functions and variable)
as well ;-) So how about saying:

    In order to address this, ensure we are not suppressing decls for
    which an alias is not suppressed.

> We therefore keep the DWARF information even if only a non-main symbol
> is asked for.


    We therefore keep the decl in the IR when at least one its underlying
    aliased symbols is non-suppressed.

> Likewise, when the abg-writer is having to attach an elf-symbol-id to
> the DWARF collected information (for functions and variables), 

    Likewise, when the abg-writer is having to attach an elf-symbol-id
    to the decl,

> instead of omitting the symbol altogether, rather make use of the property of
> aliases and connect the dwarf information to an alias instead. This way
> the function dwarf information stays connected to the elf symbol that we
> want to track.
>
> When reading from XML with a symbol whitelist that leads to suppression
> of aliased symbols, abidiff would hit an assertion and crash when
> looking up the aliased symbol due to it being suppressed. In the new
> symtab reader we can still suppress a symbol without removing it from
> the lookup. Make use of that property to fix this bug.
>
> A test has been added for this as well.
>
> 	* src/abg-dwarf-reader.cc(function_is_suppressed): Do not suppress
> 	  a function for which there is an alias that is not suppressed.
> 	(variable_is_suppressed): Likewise for variables.
> 	* src/abg-reader.cc (build_elf_symbol): Improve handling of
> 	suppressed aliased symbols when reading from XML.
> 	* src/abg-symtab-reader.cc (load): Likewise.
> 	* src/abg-writer.cc(write_elf_symbol_reference): Fall back to
> 	  any aliased symbol if the main symbol is suppressed.
> 	* tests/data/Makefile.am: Add new test files.
> 	* tests/data/test-abidiff-exit/test-missing-alias-report.txt: New test file.
> 	* tests/data/test-abidiff-exit/test-missing-alias.abi: Likewise.
> 	* tests/data/test-abidiff-exit/test-missing-alias.suppr: Likewise.
> 	* tests/test-abidiff-exit.cc: Add support for whitelists and add
> 	new testcase.
> 	* tests/data/test-read-dwarf/test-suppressed-alias.c: New test file.
> 	* tests/data/test-read-dwarf/test-suppressed-alias.o: Likewise.
> 	* tests/data/test-read-dwarf/test-suppressed-alias.o.abi: Likewise.
> 	* tests/data/test-read-dwarf/test-suppressed-alias.suppr: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-1.so.hash.abi: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-1.suppr: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-2.so.hash.abi: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-2.suppr: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-3.so.hash.abi: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-3.suppr: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-4.so.hash.abi: Likewise.
> 	* tests/data/test-read-dwarf/test3-alias-4.suppr: Likewise.
> 	* tests/test-read-dwarf.cc: Add new test cases.
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Matthias Maennich <maennich@google.com>

OK to apply master with those commit log changes when the prerequisite
patches are in.

Thanks!

[...]

Cheers,

-- 
		Dodji