[PATCH 4/8, applied] Bug 27995 - Self comparison error from abixml file

[PATCH 4/8, applied] Bug 27995 - Self comparison error from abixml file

[Dodji Seketeli]

[-- Attachment #1: Type: text/plain, Size: 11998 bytes --]

Hello,

There are several self comparison issues uncovered by comparing the
file test-PR27995.abi (provided in the bug report) against itself.
This patch address them all as well as the regressions induced on some
of the test suite and then and updates the other reference test suite
output that need it.

In the equals overload for decl_base, we compare the non-internal
versions of qualified decl names.  For var_decls of anonymous class or
union types, the non-internal version is the flat-representation of
the type.  Thus a benign change in a data member name of the anonymous
type might cause the equals function to consider the var_decls to be
wrongly different.  The internal version of the qualified decl name
should return a name that is stable for types, irrespective of these
benign variations.  The patch thus makes the equals overload for
decl_base to compare internal versions of qualified decl names instead.

The patch ensures that enum_type_decl::get_pretty_representation
return and internal pretty representation that is "stable" for
anonymous types.  Basically, all anonymous enums will have the same of
name that looks like "__anonymous_enum__".  This is to ensure two
things: first, that all anonymous enums are compared against each
other during type canonicalization, ensuring that when two anonymous
enums are canonically different, it really is because of changes in
their enumerators or basic type, not because of anything having to do
with their artificial names.  Second, that in the equals overload for
decl_base, their internal qualified name always compare equal.  This
nullifies the risk of having anonymous types compare different because
of their (non existent) name.  This is because libabigail's dwarf
reader assigns artificial unique names to anonymous types, so we don't
want to use these during actual type comparison.

We do something similar for class_decl::get_pretty_representation and
union_decl::get_pretty_representation where the pretty internal
representation for class/union decl would now be
__anonymous_{struct,union}__.

The patch scouts the uses of get_pretty_representation() to make sure
to use avoid using the internal-form of the pretty representations
when it's not warranted.  It also updates the doxygen comments of the
overloads of that function.

In the abixml reader, we were wrongly canonicalizing array types
early, even before they were fully created.  The was leading to
spurious type chances down the road.

The patch also fixes the caching of the name of function types by
making it consistent with caching of the names of the other types of
the system.  The idea is that we don't cache the name of a function
type until it's canonicalize.  This is because the type might be
edited during its pre-canonicalization life time; and that editing
might change its name.  However once the type is canonicalized, it
becomes immutable.  At that point we can cache its name, for
performance purposes.  Note that we need to do that both for the
"internal version" of the type name (used for canonilization purposes)
and the "non-internal version" one, which is used for other purposes.

This caching scheme wasn't respected for function types, so we were
caching a potentially wrong name for the type after its
canonicalization.

Last but not least, there is a problem that makes canonical type
comparison different from structural type comparison.
Let's consider these two declarations:

    typedef int FirstInt;
    typedef int SecondInt;

Now, consider these two pointer types: FirstInt* and SecondInt*;
These two pointer types are canonically different because they have
different type names.  This is because during type canonicalization,
types with the same "pretty representation" are compared against each
other.  So types with different type names will certainly have
different pretty representations and won't be compared; they are thus
going to have different canonical types.

However, FirstInt* and SecondInt* do compare equal, structurally,
because the equals overload for pointer_type_def compares the
pointed-to types of pointers by peeling off typedefs.  So, here, as
both pointed-to types are 'int' when the typedefs are peeled off, the
two pointers structurally compare equal.  This discrepancy between
structural and canonical equality introduces subtle and spurious type
changes depending on the order in which types are canonicalized.  For
instance:

    struct {FirstInt* m0;};   /* First type.  */

    struct {SecondInt* m0;};  /* Second type. */

If FirstInt* and SecondInt* are canonicalized before their containing
anonymous types, then the two anonymous types will compare different
(because FirstInt* and SecondInt* compare different) and have
different canonical types.  If, however, the anonymous types are
canonicalized before FirstInt* and SecondInt*, then will compare equal
because FirstInt* and SecondInt* are structurally equivalent.
FirstInt* and SecondInt* will be canonicalized latter and have
different canonical types (because they have different type names)
despite being structurally equivalent.

The change in the order of canonicalization can happen when
canonicalizing types from a corpus coming from DWARF as opposed to
canonicalizing types from a corpus coming from abixml.

The patch fixes this discrepancy by not peeling off typedefs from the
pointed-to types when comparing pointers.  Note that this makes us
regress on bug https://sourceware.org/bugzilla/show_bug.cgi?id=27236,
where the typedef peeling was introduced.  In hindsight, introducing
that typedef peeling was a mistake.  I'll try to address that bug
again in a subsequent patch.

	* doc/manuals/abidiff.rst: Add documentation for the --debug
	option.
	* src/abg-ir.cc (equals): In the overload for decl_base consider
	the internal version of qualified decl name.  In the overload for
	pointer_type_def do not peel typedefs off from the compared
	pointed-to types.  In the overload for typedef_decl compare the
	typedef as a decl as well.  In the overload for var_decl, compare
	variables that have the same ELF symbols without taking into
	account their qualified name, rather than their name.  Stop
	comparing data member without considering their names.
	In the overload for class_or_union, when a decl-only class that is
	ODR-relevant is compared against another type, assume that
	equality if names are equal.  This is useful in environments where
	some TUs are ODR-relevant and others aren't.
	(*::get_pretty_representation): Update doxygen comments.
	(enum_type_decl::get_pretty_representation): Return an internal
	pretty representation that is stable across all anonymous enums.
	(var_decl::get_anon_dm_reliable_name): Use the non-internal pretty
	representation for anonymous data members.
	(function_type::priv::temp_internal_cached_name_): New data
	member.
	(function_type::get_cached_name): Cache the internal name after
	the function type is canonicalized.  Make sure internal name and
	non-internal name are cached separately.
	(class_or_union::find_anonymous_data_member): Look for the anonymous
	data member by looking at its non-internal name.
	({class, union}_decl::get_pretty_representation): Use something like "class
	__anonymous_{union,struct}__" for all anonymous classes, so that they can
	all be compared against each other during type canonicalization.
	(type_has_sub_type_changes): Use non-internal pretty
	representation.
	(hash_type_or_decl, function_decl_is_less_than:): Use internal
	pretty representation for comparison here.
	* src/abg-reader.cc (read_context::maybe_canonicalize_type): Don't
	early canonicalize array types.
	* src/abg-writer.cc (annotate): Use non-internal pretty
	representation.
	* tests/data/test-diff-filter/test-PR27995-report-0.txt: New
	reference report.
	* tests/data/test-diff-filter/test-PR27995.abi: New test input
	abixml file.
	* tests/data/Makefile.am: Add test-PR27995.abi,
	test-PR27995-report-0.txt to the source distribution.
	* tests/data/test-annotate/libtest23.so.abi: Adjust.
	* tests/data/test-diff-dwarf/test6-report.txt: Adjust.
	* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-0.txt: Adjust.
	* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-1.txt: Adjust.
	* tests/data/test-diff-filter/test41-report-0.txt: Adjust.
	* tests/data/test-diff-filter/test43-decl-only-def-change-leaf-report-0.txt: Adjust.
	* tests/data/test-diff-filter/test8-report.txt: Adjust.
	* tests/data/test-diff-pkg/libICE-1.0.6-1.el6.x86_64.rpm--libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt:
	Adjust.
	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-0.txt:
	Adjust.
	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
	Adjust.
	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt:
	Adjust.
	* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt:
	Adjust.
	* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt:
	Adjust.
	* tests/data/test-diff-suppr/test39-opaque-type-report-0.txt: Adjust.
	* tests/data/test-read-dwarf/PR22015-libboost_iostreams.so.abi: Adjust.
	* tests/data/test-read-dwarf/PR22122-libftdc.so.abi: Adjust.
	* tests/data/test-read-dwarf/libtest23.so.abi: Adjust.
	* tests/data/test-read-dwarf/test-libandroid.so.abi: Adjust.
	* tests/data/test-read-dwarf/test11-pr18828.so.abi: Adjust.
	* tests/data/test-read-dwarf/test12-pr18844.so.abi: Adjust.
	* tests/data/test-read-dwarf/test9-pr18818-clang.so.abi: Adjust.
	* tests/test-diff-filter.cc (in_out_specs): Add the
	test-PR27995.abi to the test harness.
	* tools/abidiff.cc (options::do_debug): New data member.
	(options::options): Initialize it.
	(parse_command_line): Parse --debug.
	(main): Activate self comparison debug if the user provided
	--debug.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to master.
---
 doc/manuals/abidiff.rst                       |      9 +
 src/abg-ir.cc                                 |    332 +-
 src/abg-reader.cc                             |      1 +
 src/abg-writer.cc                             |      2 +-
 tests/data/Makefile.am                        |      2 +
 tests/data/test-annotate/libtest23.so.abi     |     24 +-
 tests/data/test-diff-dwarf/test6-report.txt   |     12 +
 .../test-PR27995-report-0.txt                 |      0
 tests/data/test-diff-filter/test-PR27995.abi  | 290129 +++++++++++++++
 .../test31-pr18535-libstdc++-report-0.txt     |     59 +-
 .../test31-pr18535-libstdc++-report-1.txt     |     59 +-
 .../data/test-diff-filter/test41-report-0.txt |      2 +-
 ...t43-decl-only-def-change-leaf-report-0.txt |      5 -
 tests/data/test-diff-filter/test8-report.txt  |      3 +
 ...libICE-1.0.9-2.el7.x86_64.rpm-report-0.txt |      2 +-
 ...l7.x86_64-0.12.8-1.el7.x86_64-report-0.txt |     10 +-
 ...l7.x86_64-0.12.8-1.el7.x86_64-report-2.txt |     17 +-
 ...l7.x86_64-0.12.8-1.el7.x86_64-report-3.txt |      4 +-
 ...bb-4.3-3.20141204.fc23.x86_64-report-0.txt |      2 +-
 ...bb-4.3-3.20141204.fc23.x86_64-report-1.txt |      2 +-
 .../test39-opaque-type-report-0.txt           |      9 +-
 .../PR22015-libboost_iostreams.so.abi         |      2 +-
 .../test-read-dwarf/PR22122-libftdc.so.abi    |    206 +-
 tests/data/test-read-dwarf/libtest23.so.abi   |     22 +-
 .../test-read-dwarf/test-libandroid.so.abi    |    266 +-
 .../test-read-dwarf/test11-pr18828.so.abi     |    250 +-
 .../test-read-dwarf/test12-pr18844.so.abi     |      2 +-
 .../test9-pr18818-clang.so.abi                |      2 +-
 tests/test-diff-filter.cc                     |      9 +-
 tools/abidiff.cc                              |     19 +
 30 files changed, 290891 insertions(+), 572 deletions(-)
 create mode 100644 tests/data/test-diff-filter/test-PR27995-report-0.txt
 create mode 100644 tests/data/test-diff-filter/test-PR27995.abi

The patch is big so I am attaching it gziped.


[-- Attachment #2: 0004-Bug-27995-Self-comparison-error-from-abixml-file.patch.gz --]
[-- Type: application/gzip, Size: 2046021 bytes --]

[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


-- 
		Dodji