Skip to content

Commit

Permalink
Don't consider type name when comparing typedefs
Browse files Browse the repository at this point in the history
This is from a problem report originating from Red Hat bugzilla at
https://bugzilla.redhat.com/show_bug.cgi?id=1925876

I some C programs, the same type can be defined more than once in a
binary, as there is no "One Definition Rule[1]" in C.

    [1]: https://en.wikipedia.org/wiki/One_Definition_Rule

The definition of those two types can be slightly different and yet be
equivalent.

For instance, the data member of a struct S might be defined once as
having a type which is a typedef Foo of, say, "long int" and that
struct S might be defined again elsewhere with a data member of type
typedef Bar of "long int" as well.

With the current code, because Foo and Bar have different names, they
are are going to compare different; so the two struct S are doing to
compare different even though they are equivalent.

Down the road, this is likely going to imply that the several 'struct
S' that are declaration-only will not be resolved as being
declarations of the definition of "struct S", because they is more
than one such definition, and those definitions are different.

This is going to lead to spurious (and hard to debug) type differences
that are going to be detected and reported by libabigail later down
the road.

This patch addresses the problem by not taking typedef names into
account when comparing typedefs before canonicalization.  That allows
the comparison of classes and structs that happens during the
resolution of declaration-only classes to correctly deduce their
equivalence even in cases like the one exposed above.  It thus reduces
the amount of declaration-only classes that are unnecessarily
considered to be different from the definition they ought to equal.

	* include/abg-ir.h (maybe_compare_as_member_decls): Declare new
	function.  Make it a friend of class decl_base.
	* src/abg-dwarf-reader.cc (maybe_canonicalize_type): Don't early
	canonicalize typedefs because they can be "part" of a type that is
	not yet completed, especially considering that class declaration
	resolution is part of type building, stricto sensu.
	* src/abg-ir.cc (maybe_compare_as_member_decls): Factorize this
	out of ...
	(equals): ... the overload for decl_base.  Use it in the overload
	for typedef_decl.
	* tests/data/test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64-self-check-report-0.txt:
	New test reference output.
	* tests/data/test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64.rpm: New
	binary input.
	* tests/data/test-diff-pkg/nmap-debuginfo-7.70-5.el8_testjcc.x86_64.rpm: Likewise.
	* tests/data/Makefile.am: Add these new testing material to source
	distribution.
 	* tests/test-diff-pkg.cc (in_out_specs): Add the new test input to
	the harness.
	* 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-suppr/test39-opaque-type-report-0.txt:
	Adjust.

Signed-off-by: Dodji Seketeli <[email protected]>
  • Loading branch information
Dodji Seketeli committed Feb 11, 2021
1 parent a8c0199 commit 77bc4b7
Show file tree
Hide file tree
Showing 10 changed files with 109 additions and 61 deletions.
10 changes: 10 additions & 0 deletions include/abg-ir.h
Original file line number Diff line number Diff line change
Expand Up @@ -1193,6 +1193,11 @@ operator|=(change_kind&, change_kind);
change_kind&
operator&=(change_kind&, change_kind);

bool
maybe_compare_as_member_decls(const decl_base& l,
const decl_base& r,
change_kind* k);

bool
equals(const decl_base&, const decl_base&, change_kind*);

Expand Down Expand Up @@ -1573,6 +1578,11 @@ class decl_base : public virtual type_or_decl_base
friend bool
equals(const var_decl&, const var_decl&, change_kind*);

friend bool
maybe_compare_as_member_decls(const decl_base& l,
const decl_base& r,
change_kind* k);

friend decl_base_sptr
add_decl_to_scope(decl_base_sptr decl, scope_decl* scpe);

Expand Down
3 changes: 2 additions & 1 deletion src/abg-dwarf-reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16546,7 +16546,8 @@ maybe_canonicalize_type(const Dwarf_Die *die, read_context& ctxt)
|| is_union_type(peeled_type)
|| is_function_type(peeled_type)
|| is_array_type(peeled_type)
|| is_qualified_type(peeled_type))
|| is_qualified_type(peeled_type)
|| is_typedef(t))
// We delay canonicalization of classes/unions or typedef,
// pointers, references and array to classes/unions. This is
// because the (underlying) class might not be finished yet and we
Expand Down
121 changes: 77 additions & 44 deletions src/abg-ir.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4078,6 +4078,74 @@ operator&=(change_kind& l, change_kind r)
return l;
}

/// Compare the properties that belong to the "is-a-member-relation"
/// of a decl.
///
/// For instance, access specifiers are part of the
/// "is-a-member-relation" of a decl.
///
/// This comparison however doesn't take decl names into account. So
/// typedefs for instance are decls that we want to compare with this
/// function.
///
/// This function is a sub-routine of the more general 'equals'
/// overload for instances of decl_base.
///
/// @param l the left-hand side operand of the comparison.
///
/// @param r the right-hand side operand of the comparison.
///
/// @return true iff @p l compare equals, as a member decl, to @p r.
bool
maybe_compare_as_member_decls(const decl_base& l,
const decl_base& r,
change_kind* k)
{
bool result = true;
if (is_member_decl(l) && is_member_decl(r))
{
context_rel* r1 = const_cast<context_rel*>(l.get_context_rel());
context_rel *r2 = const_cast<context_rel*>(r.get_context_rel());

access_specifier la = no_access, ra = no_access;
bool member_types_or_functions =
((is_type(l) && is_type(r))
|| (is_function_decl(l) && is_function_decl(r)));

if (member_types_or_functions)
{
// Access specifiers on member types in DWARF is not
// reliable; in the same DSO, the same struct can be either
// a class or a struct, and the access specifiers of its
// member types are not necessarily given, so they
// effectively can be considered differently, again, in the
// same DSO. So, here, let's avoid considering those!
// during comparison.
la = r1->get_access_specifier();
ra = r2->get_access_specifier();
r1->set_access_specifier(no_access);
r2->set_access_specifier(no_access);
}

bool rels_are_different = *r1 != *r2;

if (member_types_or_functions)
{
// restore the access specifiers.
r1->set_access_specifier(la);
r2->set_access_specifier(ra);
}

if (rels_are_different)
{
result = false;
if (k)
*k |= LOCAL_NON_TYPE_CHANGE_KIND;
}
}
return result;
}

/// Compares two instances of @ref decl_base.
///
/// If the two intances are different, set a bitfield to give some
Expand Down Expand Up @@ -4165,49 +4233,7 @@ equals(const decl_base& l, const decl_base& r, change_kind* k)
return false;
}

if (is_member_decl(l) && is_member_decl(r))
{
context_rel* r1 = const_cast<context_rel*>(l.get_context_rel());
context_rel *r2 = const_cast<context_rel*>(r.get_context_rel());

access_specifier la = no_access, ra = no_access;
bool member_types_or_functions =
((is_type(l) && is_type(r))
|| (is_function_decl(l) && is_function_decl(r)));

if (member_types_or_functions)
{
// Access specifiers on member types in DWARF is not
// reliable; in the same DSO, the same struct can be either
// a class or a struct, and the access specifiers of its
// member types are not necessarily given, so they
// effectively can be considered differently, again, in the
// same DSO. So, here, let's avoid considering those!
// during comparison.
la = r1->get_access_specifier();
ra = r2->get_access_specifier();
r1->set_access_specifier(no_access);
r2->set_access_specifier(no_access);
}

bool rels_are_different = *r1 != *r2;

if (member_types_or_functions)
{
// restore the access specifiers.
r1->set_access_specifier(la);
r2->set_access_specifier(ra);
}

if (rels_are_different)
{
result = false;
if (k)
*k |= LOCAL_NON_TYPE_CHANGE_KIND;
else
return false;
}
}
result &= maybe_compare_as_member_decls(l, r, k);

return result;
}
Expand Down Expand Up @@ -16157,7 +16183,14 @@ bool
equals(const typedef_decl& l, const typedef_decl& r, change_kind* k)
{
bool result = true;
if (!l.decl_base::operator==(r))
// Compare the properties of the 'is-a-member-decl" relation of this
// decl. For typedefs of a C program, this always return true as
// there is no "member typedef type" in C.
//
// In other words, in C, Only the underlying types of typedefs are
// compared. In C++ however, the properties of the
// 'is-a-member-decl' relation of the typedef are compared.
if (!maybe_compare_as_member_decls(l, r, k))
{
result = false;
if (k)
Expand Down
3 changes: 3 additions & 0 deletions tests/data/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -1806,6 +1806,9 @@ test-diff-pkg/glibc-debuginfo-2.32-3.fc33.aarch64.rpm \
test-diff-pkg/sshpass-1.07-1.fc34.x86_64-self-check-report-0.txt \
test-diff-pkg/sshpass-1.07-1.fc34.x86_64.rpm \
test-diff-pkg/sshpass-debuginfo-1.07-1.fc34.x86_64.rpm \
test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64-self-check-report-0.txt \
test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64.rpm \
test-diff-pkg/nmap-debuginfo-7.70-5.el8_testjcc.x86_64.rpm \
\
test-fedabipkgdiff/dbus-glib-0.104-3.fc23.x86_64.rpm \
test-fedabipkgdiff/dbus-glib-debuginfo-0.104-3.fc23.x86_64.rpm \
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
==== SELF CHECK SUCCEEDED for 'nmap' ====
==== SELF CHECK SUCCEEDED for 'nping' ====
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
================ changes of 'libspice-server.so.1.8.0'===============
Functions changes summary: 1 Removed, 2 Changed (77 filtered out), 8 Added functions
Functions changes summary: 1 Removed, 1 Changed (78 filtered out), 8 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 Removed function:
Expand All @@ -17,7 +17,7 @@
[A] 'function void spice_server_set_playback_rate(SpicePlaybackInstance*, uint32_t)' {spice_server_set_playback_rate@@SPICE_SERVER_0.12.5}
[A] 'function void spice_server_set_record_rate(SpiceRecordInstance*, uint32_t)' {spice_server_set_record_rate@@SPICE_SERVER_0.12.5}

2 functions with some indirect sub-type change:
1 function with some indirect sub-type change:

[C] 'function spice_image_compression_t spice_server_get_image_compression(SpiceServer*)' at reds.c:3618:1 has some indirect sub-type changes:
return type changed:
Expand All @@ -44,11 +44,5 @@
'SpiceImageCompression::SPICE_IMAGE_COMPRESSION_LZ4' value '7'
'SpiceImageCompression::SPICE_IMAGE_COMPRESSION_ENUM_END' value '8'

[C] 'function int spice_server_set_image_compression(SpiceServer*, spice_image_compression_t)' at reds.c:3602:1 has some indirect sub-type changes:
parameter 2 of type 'typedef spice_image_compression_t' changed:
typedef name changed from spice_image_compression_t to SpiceImageCompression at enums.h:197:1
underlying type 'enum __anonymous_enum__2' at spice.h:471:1 changed:
enum type 'enum __anonymous_enum__2' changed at spice.h:471:1, as reported earlier

================ end of changes of 'libspice-server.so.1.8.0'===============

9 changes: 1 addition & 8 deletions tests/data/test-diff-suppr/test39-opaque-type-report-0.txt
Original file line number Diff line number Diff line change
@@ -1,10 +1,3 @@
Functions changes summary: 0 Removed, 1 Changed (1 filtered out), 0 Added functions
Functions changes summary: 0 Removed, 0 Changed (2 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

[C] 'function void bar(StructType*)' at test39-opaque-type-v1.c:20:1 has some indirect sub-type changes:
parameter 1 of type 'StructType*' changed:
in pointed to type 'typedef StructType' at test39-header-v1.h:2:1:
typedef name changed from StructType to AnotherStructType at test39-header-v1.h:2:1

12 changes: 12 additions & 0 deletions tests/test-diff-pkg.cc
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,18 @@ static InOutSpec in_out_specs[] =
"data/test-diff-pkg/sshpass-1.07-1.fc34.x86_64-self-check-report-0.txt",
"output/test-diff-pkg/sshpass-1.07-1.fc34.x86_64-self-check-report-0.txt"
},
{
"data/test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64.rpm",
"data/test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64.rpm",
"--self-check",
"",
"data/test-diff-pkg/nmap-debuginfo-7.70-5.el8_testjcc.x86_64.rpm",
"data/test-diff-pkg/nmap-debuginfo-7.70-5.el8_testjcc.x86_64.rpm",
"",
"",
"data/test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64-self-check-report-0.txt",
"output/test-diff-pkg/nmap-7.70-5.el8_testjcc.x86_64-self-check-report-0.txt"
} ,
#endif // WITH_RPM_4_15
#endif //WITH_RPM

Expand Down

0 comments on commit 77bc4b7

Please sign in to comment.