abixml reader: Fix recursive type definition handling

This should fix self comparison bug https://bugzilla.redhat.com/show_bug.cgi?id=1944088

This arose from a self comparison check failing on the library
libgvpr.so.2 from the graphviz-2.44.0-17.el9.aarch64.rpm package.

Now that we have facilities to see what type (instantiated from the
abixml representation of the libgvpr.so library) exactly the
canonicalization process is failing for, I decided to use it ;-)

I extracted the package and its associating debug info into a
directory named 'extract' and ran abidw --debug-abidiff on it:

    $ build/tool/abidw  --debug-abidiff -d extract/usr/lib/debug extract/usr/lib64/libgvpr.so.2

That yielded the output below:

    error: problem detected with type 'typedef Vmalloc_t' from second corpus
    error: canonical type for type 'typedef Vmalloc_t' of type-id 'type-id-170' changed from 'd72618' to '14a7448'
    error: problem detected with type 'Vmalloc_t*' from second corpus
    error: canonical type for type 'Vmalloc_t*' of type-id 'type-id-188' changed from 'd72ba8' to '14a7968'
    [...]

This tells me that "typedef Vmalloc_t", created from the abixml
compares different from its originating peer that was created from the
binary directly.  The same goes for the pointer type "Vmalloc_t*", etc.

Using the new debugging/logging functionalities from the command line
of the debugger, I could see that in the abixml reader,
build_typedef_decl can fail subtly when the underlying type of the
typedef refers to the typedef itself.  In that case, we need to ensure
that the typedef created by build_typedef_decl is the same one that is
used by the underlying type.  which is not the case at the moment.  At
the moment, the underlying type would create a new typedef beside the
one currently being created by build_typedef_decl.  That leads to more
than one typedef in the system to designate "typedef Vmalloc_t".  And
that wreaks havoc later down the road.

This patch arranges so that build_typedef_decl creates the typedef
"early" before the underlying type is created.  That typedef
temporarily has no underlying type.  It's registered as being the
typedef for the type-id string that identifies it in the abixml.  And
then the function goes to create the underlying type.  This
arrangement ensures that if the underlying type refers to the typedef
being created (via its type-id string), then the typedef that was
created early is effectively re-used.  This ensures that a typedef
which recursively refer to itself is properly represented.  It's only
when the underlying type is fully created that it's added to the
typedef.

Something similar is done for pointer types, in
build_pointer_type_def.

Note that to do this, the patch adjusts the typedef_decl and
pointer_type_def classes so that they can be created with no
underlying/pointed-to types.  The underlying/pointed-to type can thus
be added later.

I believe this patch is the minimal patch necessary to fix this issue.
The graphviz RPM is added to the regression test suite for good
measure.

After visual inspection, I realized that there are other types besides
typedef and pointer types that exhibit the same class of problem even
if they are not involved in this issue on this particular binary.  A
subsequent patch is going to address the problem for those types,
namely, qualified and reference types.

	* include/abg-ir.h (pointer_type_def::pointer_type_def): Declare a
	constructor with no pointed-to type.
	(pointer_type_def::set_pointed_to_type): Declare new method.
	(typedef_decl::typedef_decl): Declare a constructor with no
	underlying type.
	* src/abg-ir.cc (pointer_type_def::pointer_type_def): Define a
	constructor with no pointed-to type.  The pointed-to type can thus
	later be set when it becomes available.
	(pointer_type_def::set_pointed_to_type): Define new method.
	(pointer_type_def::get_qualified_name): Make this work on a
	pointer type that (momentarily) has no pointed-to type.
	(typedef_decl::typedef_decl): Define a constructor with no
	underlying type.
	(typedef_decl::get_size_in_bits): Make this work on a typedef that
	has (momentarily) no underlying type.
	(typedef_decl::set_underlying_type): Update the size and alignment
	of the typedef from its new underlying type.
	* src/abg-reader.cc (build_pointer_type_def): Construct the
	pointer type early /BEFORE/ we even try to construct its
	pointed-to type.  Associate this incomplete type with the type-id.
	Then try to construct the pointed-to type.  During the
	construction of the pointed-to type, if this pointer is needed
	(due to recursion) then the incomplete pointer type can be used,
	leading to just one pointer type used (recursively) as it should
	be.
	(build_typedef_decl): Likewise for building typedef type early
	without its underlying type so that it can used by the underlying
	type if needed.
	* tests/data/test-diff-pkg/graphviz-2.44.0-18.el9.aarch64-self-check-report-0.txt:
	New test reference output.
	* tests/data/test-diff-pkg/graphviz-2.44.0-18.el9.aarch64.rpm: New
	binary test input.
	* tests/data/test-diff-pkg/graphviz-debuginfo-2.44.0-18.el9.aarch64.rpm: Likewise.
	* tests/data/Makefile.am: Add the new test material above to
	source distribution.
	* tests/test-diff-pkg.cc (in_out_specs): Add the test inputs above
	to this test harness.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This commit is contained in:
Dodji Seketeli 2021-05-21 23:55:44 +02:00
parent d94947440e
commit 7a9fa3fe5a
8 changed files with 241 additions and 84 deletions

View File

@ -2263,6 +2263,13 @@ public:
pointer_type_def(const type_base_sptr& pointed_to_type, size_t size_in_bits,
size_t alignment_in_bits, const location& locus);
pointer_type_def(environment* env, size_t size_in_bits,
size_t alignment_in_bits, const location& locus);
void
set_pointed_to_type(const type_base_sptr&);
virtual bool
operator==(const decl_base&) const;
@ -2737,6 +2744,12 @@ public:
const string& mangled_name = "",
visibility vis = VISIBILITY_DEFAULT);
typedef_decl(const string& name,
environment* env,
const location& locus,
const string& mangled_name = "",
visibility vis = VISIBILITY_DEFAULT);
virtual size_t
get_size_in_bits() const;

View File

@ -14756,6 +14756,16 @@ void
pointer_type_def::on_canonical_type_set()
{clear_qualified_name();}
///Constructor of @ref pointer_type_def.
///
/// @param pointed_to the pointed-to type.
///
/// @param size_in_bits the size of the type, in bits.
///
/// @param align_in_bits the alignment of the type, in bits.
///
/// @param locus the source location where the type was defined.
pointer_type_def::pointer_type_def(const type_base_sptr& pointed_to,
size_t size_in_bits,
size_t align_in_bits,
@ -14783,6 +14793,55 @@ pointer_type_def::pointer_type_def(const type_base_sptr& pointed_to,
{}
}
///Constructor of @ref pointer_type_def.
///
/// @param env the environment of the type.
///
/// @param size_in_bits the size of the type, in bits.
///
/// @param align_in_bits the alignment of the type, in bits.
///
/// @param locus the source location where the type was defined.
pointer_type_def::pointer_type_def(environment* env, size_t size_in_bits,
size_t alignment_in_bits,
const location& locus)
: type_or_decl_base(env,
POINTER_TYPE
| ABSTRACT_TYPE_BASE
| ABSTRACT_DECL_BASE),
type_base(env, size_in_bits, alignment_in_bits),
decl_base(env, "", locus, ""),
priv_(new priv())
{
runtime_type_instance(this);
string name = string("void") + "*";
set_name(env->intern(name));
}
/// Set the pointed-to type of the pointer.
///
/// @param t the new pointed-to type.
void
pointer_type_def::set_pointed_to_type(const type_base_sptr& t)
{
ABG_ASSERT(t);
priv_->pointed_to_type_ = t;
priv_->naked_pointed_to_type_ = t.get();
try
{
const environment* env = t->get_environment();
ABG_ASSERT(get_environment() == env);
decl_base_sptr pto = dynamic_pointer_cast<decl_base>(t);
string name = (pto ? pto->get_name() : string("void")) + "*";
set_name(env->intern(name));
if (pto)
set_visibility(pto->get_visibility());
}
catch (...)
{}
}
/// Compares two instances of @ref pointer_type_def.
///
/// If the two intances are different, set a bitfield to give some
@ -14898,6 +14957,9 @@ pointer_type_def::get_qualified_name(interned_string& qn, bool internal) const
/// of @ref pointer_type_def. Subsequent invocations of this function
/// return the cached value.
///
/// Note that this function should work even if the underlying type is
/// momentarily empty.
///
/// @param internal set to true if the call is intended for an
/// internal use (for technical use inside the library itself), false
/// otherwise. If you don't know what this is for, then set it to
@ -14914,10 +14976,11 @@ pointer_type_def::get_qualified_name(bool internal) const
if (get_canonical_type())
{
if (priv_->internal_qualified_name_.empty())
priv_->internal_qualified_name_ =
get_name_of_pointer_to_type(*pointed_to_type,
/*qualified_name=*/true,
/*internal=*/true);
if (pointed_to_type)
priv_->internal_qualified_name_ =
get_name_of_pointer_to_type(*pointed_to_type,
/*qualified_name=*/true,
/*internal=*/true);
return priv_->internal_qualified_name_;
}
else
@ -14926,10 +14989,11 @@ pointer_type_def::get_qualified_name(bool internal) const
// (and so its name) can change. So let's invalidate the
// cache where we store its name at each invocation of this
// function.
priv_->temp_internal_qualified_name_ =
get_name_of_pointer_to_type(*pointed_to_type,
/*qualified_name=*/true,
/*internal=*/true);
if (pointed_to_type)
priv_->temp_internal_qualified_name_ =
get_name_of_pointer_to_type(*pointed_to_type,
/*qualified_name=*/true,
/*internal=*/true);
return priv_->temp_internal_qualified_name_;
}
}
@ -14950,10 +15014,11 @@ pointer_type_def::get_qualified_name(bool internal) const
// (and so its name) can change. So let's invalidate the
// cache where we store its name at each invocation of this
// function.
set_qualified_name
(get_name_of_pointer_to_type(*pointed_to_type,
/*qualified_name=*/true,
/*internal=*/false));
if (pointed_to_type)
set_qualified_name
(get_name_of_pointer_to_type(*pointed_to_type,
/*qualified_name=*/true,
/*internal=*/false));
return decl_base::peek_qualified_name();
}
}
@ -16755,6 +16820,34 @@ typedef_decl::typedef_decl(const string& name,
runtime_type_instance(this);
}
/// Constructor of the typedef_decl type.
///
/// @param name the name of the typedef.
///
/// @param env the environment of the current typedef.
///
/// @param locus the source location of the typedef declaration.
///
/// @param mangled_name the mangled name of the typedef.
///
/// @param vis the visibility of the typedef type.
typedef_decl::typedef_decl(const string& name,
environment* env,
const location& locus,
const string& mangled_name,
visibility vis)
: type_or_decl_base(env,
TYPEDEF_TYPE
| ABSTRACT_TYPE_BASE
| ABSTRACT_DECL_BASE),
type_base(env, /*size_in_bits=*/0,
/*alignment_in_bits=*/0),
decl_base(env, name, locus, mangled_name, vis),
priv_(new priv(nullptr))
{
runtime_type_instance(this);
}
/// Return the size of the typedef.
///
/// This function looks at the size of the underlying type and ensures
@ -16764,6 +16857,8 @@ typedef_decl::typedef_decl(const string& name,
size_t
typedef_decl::get_size_in_bits() const
{
if (!get_underlying_type())
return 0;
size_t s = get_underlying_type()->get_size_in_bits();
if (s != type_base::get_size_in_bits())
const_cast<typedef_decl*>(this)->set_size_in_bits(s);
@ -16779,7 +16874,9 @@ typedef_decl::get_size_in_bits() const
size_t
typedef_decl::get_alignment_in_bits() const
{
size_t s = get_underlying_type()->get_alignment_in_bits();
if (!get_underlying_type())
return 0;
size_t s = get_underlying_type()->get_alignment_in_bits();
if (s != type_base::get_alignment_in_bits())
const_cast<typedef_decl*>(this)->set_alignment_in_bits(s);
return type_base::get_alignment_in_bits();
@ -16903,7 +17000,11 @@ typedef_decl::get_underlying_type() const
/// @param t the new underlying type of the typedef.
void
typedef_decl::set_underlying_type(const type_base_sptr& t)
{priv_->underlying_type_ = t;}
{
priv_->underlying_type_ = t;
set_size_in_bits(t->get_size_in_bits());
set_alignment_in_bits(t->get_alignment_in_bits());
}
/// This implements the ir_traversable_base::traverse pure virtual
/// function.

View File

@ -3790,56 +3790,49 @@ build_pointer_type_def(read_context& ctxt,
return result;
}
string type_id;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "type-id"))
type_id = CHAR_STR(s);
shared_ptr<type_base> pointed_to_type =
ctxt.build_or_get_type_decl(type_id, true);
ABG_ASSERT(pointed_to_type);
// maybe building the underlying type triggered building this one in
// the mean time ...
if (decl_base_sptr d = ctxt.get_decl_for_xml_node(node))
{
pointer_type_def_sptr result =
dynamic_pointer_cast<pointer_type_def>(d);
ABG_ASSERT(result);
return result;
}
size_t size_in_bits = ctxt.get_translation_unit()->get_address_size();
size_t alignment_in_bits = 0;
read_size_and_alignment(node, size_in_bits, alignment_in_bits);
string id;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "id"))
id = CHAR_STR(s);
ABG_ASSERT(!id.empty());
if (type_base_sptr d = ctxt.get_type_decl(id))
if (type_base_sptr t = ctxt.get_type_decl(id))
{
pointer_type_def_sptr ty = is_pointer_type(d);
ABG_ASSERT(ty);
ABG_ASSERT(ctxt.types_equal(pointed_to_type,
ty->get_pointed_to_type()));
return ty;
pointer_type_def_sptr result = is_pointer_type(t);
ABG_ASSERT(result);
return result;
}
string type_id;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "type-id"))
type_id = CHAR_STR(s);
size_t size_in_bits = ctxt.get_translation_unit()->get_address_size();
size_t alignment_in_bits = 0;
read_size_and_alignment(node, size_in_bits, alignment_in_bits);
location loc;
read_location(ctxt, node, loc);
shared_ptr<pointer_type_def> t(new pointer_type_def(pointed_to_type,
size_in_bits,
alignment_in_bits,
loc));
// Create the pointer type /before/ the pointed-to type. After the
// creation, the type is 'keyed' using ctxt.push_and_key_type_decl.
// This means that the type can be retrieved from its type ID. This
// is so that if the pointed-to type indirectly uses this pointer
// type (via recursion) then that is made possible.
pointer_type_def_sptr t(new pointer_type_def(ctxt.get_environment(),
size_in_bits,
alignment_in_bits,
loc));
maybe_set_artificial_location(ctxt, node, t);
if (ctxt.push_and_key_type_decl(t, id, add_to_current_scope))
{
ctxt.map_xml_node_to_decl(node, t);
return t;
}
return nil;
if (ctxt.push_and_key_type_decl(t, id, add_to_current_scope))
ctxt.map_xml_node_to_decl(node, t);
type_base_sptr pointed_to_type =
ctxt.build_or_get_type_decl(type_id, true);
ABG_ASSERT(pointed_to_type);
t->set_pointed_to_type(pointed_to_type);
return t;
}
/// Build a reference_type_def from a pointer to 'reference-type-def'
@ -4466,47 +4459,39 @@ build_typedef_decl(read_context& ctxt,
id = CHAR_STR(s);
ABG_ASSERT(!id.empty());
string name;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "name"))
name = xml::unescape_xml_string(CHAR_STR(s));
string type_id;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "type-id"))
type_id = CHAR_STR(s);
shared_ptr<type_base> underlying_type(ctxt.build_or_get_type_decl(type_id,
true));
ABG_ASSERT(underlying_type);
// maybe building the underlying type triggered building this one in
// the mean time ...
if (decl_base_sptr d = ctxt.get_decl_for_xml_node(node))
if (type_base_sptr t = ctxt.get_type_decl(id))
{
typedef_decl_sptr result = dynamic_pointer_cast<typedef_decl>(d);
typedef_decl_sptr result = is_typedef(t);
ABG_ASSERT(result);
return result;
}
string name;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "name"))
name = xml::unescape_xml_string(CHAR_STR(s));
location loc;
read_location(ctxt, node, loc);
if (type_base_sptr d = ctxt.get_type_decl(id))
{
typedef_decl_sptr ty = dynamic_pointer_cast<typedef_decl>(d);
ABG_ASSERT(ty);
ABG_ASSERT(name == ty->get_name());
ABG_ASSERT(get_type_name(underlying_type)
== get_type_name(ty->get_underlying_type()));
// it's possible to have the same typedef several times.
}
typedef_decl_sptr t(new typedef_decl(name, underlying_type, loc));
maybe_set_artificial_location(ctxt, node, t);
if (ctxt.push_and_key_type_decl(t, id, add_to_current_scope))
{
ctxt.map_xml_node_to_decl(node, t);
return t;
}
string type_id;
if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "type-id"))
type_id = CHAR_STR(s);
ABG_ASSERT(!type_id.empty());
return nil;
// Create the typedef type /before/ the underlying type. After the
// creation, the type is 'keyed' using ctxt.push_and_key_type_decl.
// This means that the type can be retrieved from its type ID. This
// is so that if the underlying type indirectly (needs to) use(s)
// this very same typedef type (via recursion) then that is made
// possible.
typedef_decl_sptr t(new typedef_decl(name, ctxt.get_environment(), loc));
maybe_set_artificial_location(ctxt, node, t);
ctxt.push_and_key_type_decl(t, id, add_to_current_scope);
ctxt.map_xml_node_to_decl(node, t);
type_base_sptr underlying_type(ctxt.build_or_get_type_decl(type_id, true));
ABG_ASSERT(underlying_type);
t->set_underlying_type(underlying_type);
return t;
}
/// Build a class from its XML node if it is not suppressed by a

View File

@ -1852,6 +1852,9 @@ test-diff-pkg/elfutils-libs-debuginfo-0.183-1.el9.x86_64-self-check-report-0.txt
test-diff-pkg/elfutils-libs-0.183-1.el9.x86_64.rpm \
test-diff-pkg/elfutils-libs-debuginfo-0.183-1.el9.x86_64.rpm \
test-diff-pkg/elfutils-debuginfo-0.183-1.el9.x86_64.rpm \
test-diff-pkg/graphviz-2.44.0-18.el9.aarch64-self-check-report-0.txt \
test-diff-pkg/graphviz-2.44.0-18.el9.aarch64.rpm \
test-diff-pkg/graphviz-debuginfo-2.44.0-18.el9.aarch64.rpm \
\
test-fedabipkgdiff/dbus-glib-0.104-3.fc23.x86_64.rpm \
test-fedabipkgdiff/dbus-glib-debuginfo-0.104-3.fc23.x86_64.rpm \

View File

@ -0,0 +1,43 @@
==== SELF CHECK SUCCEEDED for 'liblab_gamut.so.1.0.0' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_neato_layout.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libgvc.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libgvpr.so.2.0.0' ====
==== SELF CHECK SUCCEEDED for 'gvmap' ====
==== SELF CHECK SUCCEEDED for 'lefty' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_dot_layout.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_core.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libcgraph.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'edgepaint' ====
==== SELF CHECK SUCCEEDED for 'gvcolor' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_pango.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_visio.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_gdk.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'gxl2gv' ====
==== SELF CHECK SUCCEEDED for 'gml2gv' ====
==== SELF CHECK SUCCEEDED for 'cluster' ====
==== SELF CHECK SUCCEEDED for 'gvpack' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_gtk.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'graphml2gv' ====
==== SELF CHECK SUCCEEDED for 'libcdt.so.5.0.0' ====
==== SELF CHECK SUCCEEDED for 'ccomps' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_webp.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'libpathplan.so.4.0.0' ====
==== SELF CHECK SUCCEEDED for 'mm2gv' ====
==== SELF CHECK SUCCEEDED for 'dijkstra' ====
==== SELF CHECK SUCCEEDED for 'gvgen' ====
==== SELF CHECK SUCCEEDED for 'gv2gml' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_xlib.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'prune' ====
==== SELF CHECK SUCCEEDED for 'gc' ====
==== SELF CHECK SUCCEEDED for 'bcomps' ====
==== SELF CHECK SUCCEEDED for 'sccmap' ====
==== SELF CHECK SUCCEEDED for 'unflatten' ====
==== SELF CHECK SUCCEEDED for 'tred' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_gs.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'nop' ====
==== SELF CHECK SUCCEEDED for 'libxdot.so.4.0.0' ====
==== SELF CHECK SUCCEEDED for 'acyclic' ====
==== SELF CHECK SUCCEEDED for 'libgvplugin_rsvg.so.6.0.0' ====
==== SELF CHECK SUCCEEDED for 'gvpr' ====
==== SELF CHECK SUCCEEDED for 'diffimg' ====
==== SELF CHECK SUCCEEDED for 'dot' ====

View File

@ -696,6 +696,18 @@ static InOutSpec in_out_specs[] =
"data/test-diff-pkg/elfutils-libs-debuginfo-0.183-1.el9.x86_64-self-check-report-0.txt",
"output/test-diff-pkg/elfutils-libs-debuginfo-0.183-1.el9.x86_64-self-check-report-0.txt"
} ,
{
"data/test-diff-pkg/graphviz-2.44.0-18.el9.aarch64.rpm",
"data/test-diff-pkg/graphviz-2.44.0-18.el9.aarch64.rpm",
"--self-check",
"",
"data/test-diff-pkg/graphviz-debuginfo-2.44.0-18.el9.aarch64.rpm",
"data/test-diff-pkg/graphviz-debuginfo-2.44.0-18.el9.aarch64.rpm",
"",
"",
"data/test-diff-pkg/graphviz-2.44.0-18.el9.aarch64-self-check-report-0.txt",
"output/test-diff-pkg/graphviz-2.44.0-18.el9.aarch64-self-check-report-0.txt"
} ,
#endif // WITH_RPM_4_15
#endif //WITH_RPM