Sometimes when amending a C++ class to add new members or properties
to it as directed by the DWARF debug information, we can end-up with
discrepancies related to declaration-only-ness. That is, an instances
of a given type Foo can be wrongly assigned declaration-only-ness that
should have been only carried by another instance Foo. Then, later,
comparing two pointers to Foo might wrongly lead to spurious reported
changes due to the spurious differences of declaration-only-ness in
two instances of Foo.
By fixing the setting of the declaration-only-ness, especially when
amending a C++ class this patch fixes that spurious change detected.
* src/abg-dwarf-reader.cc (add_or_update_class_type): When
creating a class, set declaration-only-ness unconditionally. When
updating the class however, only set the declaration-only-ness
when the current one is not consistent with the size of the class.
* tests/data/test-annotate/test14-pr18893.so.abi: Adjust.
* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi: Likewise.
* tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi: Likewise.
* tests/data/test-read-dwarf/test14-pr18893.so.abi: Likewise.
* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi: Likewise.
* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi: Likewise.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi: Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When using relative compilation unit paths in DWARF, the lookup for an
existing one was done with an incorrect path. In particular, a '/' was
prefixed to the path regardless of whether the compilation_dir is set.
That led to the instantiation of an additional translation unit that and
failed when adding it to the corpus due to violating translation unit
uniqueness. Fix that by correcting the lookup.
* src/abg-dwarf-reader.cc(build_translation_unit_and_add_to_ir):
Fix lookup for potentially already existing translation units.
Reported-by: Dan Albert <danalbert@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
Some types like unnamed-enum-underlying-type are not distinguished by
type_topo_comp. This can result in nondeterministic output and flakey
tests.
While a more complete ordering from type_topo_comp would be nice, the
nondeterminism can reduced by preserving the relative order of
identically-named types.
* src/abg-ir.cc (scope_decl::get_sorted_canonical_types): Sort
canonical types with std::stable_sort(..., type_topo_comp()).
Signed-off-by: Giuliano Procida <gprocida@google.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When running abidiff on test34-libjemalloc.so.2-intel-16.0.3 it would
crash in array_type_def::subrange_type::get_length on the ABG_ASSERT
get_upper_bound() >= get_lower_bound(). This was because that file
contained a subrange upper_bound value encoded with data1 or data2
without an underlying type. In that case we assumed the value was
encoded as a signed value which caused some of the upper bounds to
be negative (while the lower bound, which wasn't given, was assumed
to be zero).
* src/abg-dwarf-reader.cc (build_subrange_type): Default
is_signed to false.
Signed-off-by: Mark Wielaard <mark@klomp.org>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This patch was originally submitted by in a comment of a problem
report at https://sourceware.org/bugzilla/show_bug.cgi?id=26684#c10.
When reading the bounds of a subrange_type, we need to know if the
constant value of those bounds is signed or not.
For that, we used to just look at the form of those constant and infer
the signedness from there.
The problem is that the signedness is really determined by the value
of the DW_AT_encoding attribute present on the underlying type of the
subrange_type; the form of the bound is mere implementation detail
that is downstream of the information of carried by the DW_AT_encoding
attribute.
This patch thus does the right thing by looking at the underlying type
of the subrange_type and by doing away with the unnecessary messing
with attribute value forms.
* src/abg-dwarf-reader.cc (die_attribute_has_form)
(die_attribute_is_signed, die_attribute_is_unsigned)
(die_attribute_has_no_signedness): Remove static functions.
(die_constant_attribute): Add the 'is_signed' parameter.
(die_address_attribute): Adjust comment.
(build_subrange_type): Determine signedness of the bounds by
looking at the DW_AT_encoding attribute of the underlying type.
Signed-off-by: Mark Wielaard <mark@klomp.org>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When working in development environments with compiler versions that
might be very bleeding edge (like the Fedora Rawhide distribution) it
might be worthwhile to disable all compiler optimization to have a
better debugging experience. In practice, I bumped into this need
again and again.
So I am adding this ABIGAIL_NO_OPTIMIZATION_DEBUG environment variable
to basically allow the "-g -O0" combination, if need be.
This patch obviously doesn't change any existing behaviour if the user
doesn't set this newly introduced environment variable.
* configure.ac: Set the CXXFLAGS and CFLAGS to "-g -O0 -Wall
-Wextra -Werror" if the ABIGAIL_NO_OPTIMIZATION_DEBUG is set.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
In the abixml writer, we recently stopped using
type_base::dynamic_hash as a slow path to hash a non-canonicalized
type. Rather, we use an arbitrary constant as a hash for those
non-canonicalized types. This amounts to using structural comparison
for those types. The function that implements this hashing scheme is
hash_as_canonical_type_or_constant.
A potential concern for that approach was the possible negative impact
on speed. As it turns out since the change went in, there was no
noticeable speed impact raised after testing.
In insight, this is understandable as the number of non-canonicalized
types should be extremely reduced now that we canonicalize pretty much
all types. As far as I understand it at this point, the only types
that would not be canonicalized are unresolved declaration-only
types. And, all in all, structurally comparing unresolved
declaration-only types should be "fast enough".
If we ever stumble across any other non-canonicalized type, I think
that would be an artifact of a bug that ought to be fixed.
On that basis, I went ahead and used
hash_as_canonical_type_or_constant throughout the code base and did
away with the use of type_base::dynamic_hash for now, until it's
properly audited, regression tested and got ready for the use cases
where it might make sense.
This patch thus makes hash_type use
hash_as_canonical_type_or_constant. The writer is then back to using
hash_type again, as it used to; but at the same time, it's still using
structural comparison for non-canonilized types. So is
hash_type_or_decl, which now uses hash_type to hash types, rather than
using type_base::dynamic_hash. Note that the comparison engine
heavily uses hash_type_or_decl to hash diff nodes.
So with this small patch the comparison engine is now using structural
comparison of non-canonicalized types (and diff nodes), just as the
abixml writer does.
* include/abg-fwd.h (hash_as_canonical_type_or_constant): Remove
public declaration of this function.
* src/abg-hash.cc (type_base::dynamic_hash::operator()): Add a
comment.
* src/abg-ir.cc (hash_as_canonical_type_or_constant): Make this
function static now.
(hash_type_or_decl): Use hash_type for types.
* src/abg-writer.cc (type_hasher::operator()): Use hash_type.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
A recent change[1] triggered a variation in the ABI changes reported,
depending on the platform.
It turned out the change made the redundancy detector walk more diff
nodes (like inserted/deleted virtual member functions and their
implicit parameter type) and that uncovered several underlying issues
that has been latent for a long time.
First, we were not walking the inserted/deleted virtual member
functions in a deterministic manner at reporting time. Rather than
walking the unordered maps containing those functions, this patch now
walk them in lexicographic order. The patch also does something
similar for the changed data members, but this time during the diff
graph analysis. That order affects how we consider a given type
change to be redundant.
Second, when looking a diff node named N, if another diff node N'
equivalent to N has already been marked redundant (and thus filtered
out already), we were sometimes wrongly failing to detect and mark N
as redundant. This patch fixes that.
I realized that some code was now unnecessary so I removed it.
A lot of reference output of tests are adjusted by this patch.
Mostly, these were cases we were failing to properly detect (and
filter out) as redundant reports. So the change reports should
hopefully look more concise and to the point now.
[1] the recent change is this one:
2f92777d Consider the implicit 'this' parameter when comparing methods
* src/abg-comparison-priv.h
(diff_context::priv::last_visited_diff_node_): Remove unnecessary
data member.
(class_or_union_diff::priv::sorted_{deleted,inserted}_member_functions_):
Add new data members.
(sort_string_member_function_sptr_map): Declare new function.
* src/abg-comparison.cc (sort_string_member_function_sptr_map):
Define new function.
(redundancy_marking_visitor::visit_begin): If the current diff
node is equivalent to another one that has been already marked
redundant, then consider the current diff node as redundant as
well. Considering the fact an ancestor node has been filtered out
is now useless because if that's the case then the current
descendant node wouldn't even be walked at reporting time. So
remove the call to diff_has_ancestor_filtered_out.
(categorize_redundancy): Remove useless call here as well.
(diff_has_ancestor_filtered_out, diff_has_ancestor_filtered_out)
(diff_context::{mark_last_diff_visited_per_class_of_equivalence,
clear_last_diffs_visited_per_class_of_equivalence,
get_last_visited_diff_of_class_of_equivalence}): Remove
unnecessary functions.
(redundancy_marking_visitor::visit_end): Add comment.
(class_diff::ensure_lookup_tables_populated): Lexicographically
sort inserted/deleted member functions.
(class_or_union_diff::chain_into_hierarchy): Chain changed data
members diff nodes in a sorted manner.
* src/abg-default-reporter.cc (default_reporter::report): Report
deleted/inserted member functions in lexicographic order.
* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-0.txt:
Adjust.
* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-1.txt:
Likewise.
* 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:
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.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Since 2013 the implicit 'this' parameter has been excluded from the
function parameters taken into account while comparing class member
functions. This was an early measure to avoid infinite recursion that
would then occur when comparing classes (and thus their member
functions that are referenced in their vtable). But since then, we've
built descent infrastructure to prevent this kind of recursion in a
more generic manner.
This patch thus removes that restriction and should therefore lift the
concerns expressed in the bug
https://sourceware.org/bugzilla/show_bug.cgi?id=26672.
Namely, changes to (data members of) a class should now be detected
when comparing member functions of that class.
With this change, the reference output of several comparison
regression tests changed because, obviously, some impacted member
functions are now reported along with detecting changes in data
membrers of classes. The patch thus adjusts those reference ouputs.
The patch also adjust the behaviour of the predicate:
"accessed_through = pointer|reference|reference-or-pointer"
The idea is to make the predicate work on qualified version of a type.
* include/abg-ir.h (function_type::get_first_parm): Declare new
accessor.
* src/abg-ir.cc (function_type::get_first_parm): Define new
accessor.
(equals): In the overload for function_type,
always take the implicit "this" parameter into account in
parameter comparisons.
(function_type::get_first_non_implicit_parm): Adjust comment.
* src/abg-comp-filter.cc (function_name_changed_but_not_symbol):
Avoid potential NULL pointer dereferencing.
* src/abg-comparison.cc
(function_type_diff::ensure_lookup_tables_populated): Always take
the changes to the implicit 'this' parameter into account in the
function type diff.
(compute_diff): In the overload for function_type, Always compare
the implicit 'this' parameter when comparing function parameters.
* src/abg-default-reporter.cc (default_reporter::report): Refer to
"implicit parameter" when reporting changes on parameters
artificially generated by the compiler.
* src/abg-suppression.cc (type_suppression::suppresses_diff): Make
the 'access_through' predicate work on a qualified version of type
'S', even if it was meant to work on type 'S'. This allows it to
work on 'const S', especially when S is accessed through 'pointer
to const S', which happens when we consider the implicit 'this'
parameter of a const member function.
* tests/data/test-abicompat/test5-fn-changed-report-0.txt: Adjust.
* tests/data/test-abicompat/test5-fn-changed-report-1.txt: Likewise.
* tests/data/test-abidiff-exit/test1-voffset-change-report0.txt:
Likewise.
* tests/data/test-abidiff/test-PR18791-report0.txt: Likewise.
* tests/data/test-abidiff/test-struct1-report.txt: Likewise.
* tests/data/test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test0-report.txt: Likewise.
* tests/data/test-diff-dwarf/test28-vtable-changes-report-0.txt: Likewise.
* tests/data/test-diff-dwarf/test29-vtable-changes-report-0.txt: Likewise.
* tests/data/test-diff-dwarf/test30-vtable-changes-report-0.txt: Likewise.
* tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt: Likewise.
* tests/data/test-diff-dwarf/test36-ppc64-aliases-report-0.txt: Likewise.
* tests/data/test-diff-dwarf/test41-PR20476-hidden-report-0.txt: Likewise.
* tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt: Likewise.
* tests/data/test-diff-dwarf/test5-report.txt: Likewise.
* tests/data/test-diff-dwarf/test8-report.txt: Likewise.
* tests/data/test-diff-filter/test0-report.txt: Likewise.
* tests/data/test-diff-filter/test01-report.txt: Likewise.
* tests/data/test-diff-filter/test10-report.txt: Likewise.
* tests/data/test-diff-filter/test13-report.txt: Likewise.
* tests/data/test-diff-filter/test2-report.txt: Likewise.
* tests/data/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-0.txt:
Likewise.
* tests/data/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt:
Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt:
Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt:
Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt:
Likewise.
* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-0.txt:
Likewise.
* tests/data/test-diff-filter/test31-pr18535-libstdc++-report-1.txt:
Likewise.
* tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt:
Likewise.
* tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-1.txt:
Likewise.
* tests/data/test-diff-filter/test4-report.txt: Likewise.
* tests/data/test-diff-filter/test41-report-0.txt: Likewise.
* tests/data/test-diff-filter/test9-report.txt: Likewise.
* tests/data/test-diff-pkg/libsigc++-2.0-0c2a_2.4.0-1_amd64--libsigc++-2.0-0v5_2.4.1-1ubuntu2_amd64-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/test24-soname-report-0.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-1.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-10.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-11.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-12.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-13.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-14.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-15.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-16.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-2.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-3.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-4.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-5.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-6.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-7.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-8.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-9.txt: Likewise.
* tests/data/test-diff-suppr/test31-report-1.txt: Likewise.
* tests/data/test-diff-suppr/test33-report-0.txt: Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The suppression specification in test38-char-class-in-ini.abignore was
introduced in commit 1478d9cc1c.
Unfortunately it contains two errors. One causes the file name not to
match as the string is the full path, not the base name. The other is
a typo that causes the file name match not to even be attempted. The
two mistakes cancel in the test, but result in a suppression
specification that is broader than intended.
* tests/data/test-diff-suppr/test38-char-class-in-ini.abignore:
Don't anchor regex match to beginning of file name.
Change "filename_regexp" to "file_name_regexp".
Signed-off-by: Giuliano Procida <gprocida@google.com>
If there ever is a discrepancy between the architectures of the
corpuses of a corpus group, libabigail will just abort with an
assertion, if enabled. However, this is a case of invalid input and
the cause should be reported to the user.
* src/abg-corpus.cc (corpus_group::add_corpus): Report
architecture discrepancies.
Signed-off-by: Giuliano Procida <gprocida@google.com>
When building a union type we try to ensure that each member is
present only once. This is because the code to build the union is
also used to actually update a partially constructed union. To do so,
before adding a member to the union, the member is looked up (among
the current members) by name to see if it's already present or not.
But then for anonymous members, the name of the member is empty.
After the first anonymous member is added to the union, subsequent
look-ups with an empty name all succeed. So no more than one
anonymous member is added to the union. Oops.
A way to fix this is to perform the lookup by taking into account the
type of the anonymous data member, not its (empty) name. We already
do this for anonymous data members of classes/structs.
This patch thus uses that type-based anonymous data member lookup for
unions.
But then now that unions can have several anonymous members, another
issue was uncovered.
Array types whose elements are of anonymous type can be wrongly
considered different because of canonicalization issues.
Let's suppose we have these two arrays whose internal pretty
representation are:
"__anonymous_struct_1__ foo[5]"
and
"__anonymous_struct_2__ foo[5]"
These are arrays of 5 elements of type anonymous struct. Suppose the
anonymous structs "__anonymous_struct_1__" and
"__anonymous_struct_2__" are structurally equivalent. Because the
internal names of these array element types are different, the
internal pretty representations of the arrays are different. And thus
the canonical types of the two arrays are different. And that's
wrong. In this particular case, they should have the same canonical
type and thus be considered equivalent.
This patch thus teaches 'get_type_name' to make the internal type
name of anonymous types of a given kind be the same. Thus, making all
arrays of 5 anonymous struct have the same pretty representation:
"__anonymous_struct__ foo[5]"
This gives the type canonicalizer a chance to detect that those arrays
having the same canonical type.
These two changes while being seemingly unrelated need to be bundled
together to fix a number of issues in the existing test reference
outputs because fixing the first one is needed to uncover the later
issue.
* src/abg-dwarf-reader.cc (add_or_update_union_type): Don't use
the empty name of anonymous members in the lookup to ensure that
all data members are unique. Rather, use the whole anonymous
member itself for the lookup, just like is done to handle
anonymous data member in classes/structs.
* src/abg-reader.cc (build_union_decl): Likewise.
* src/abg-ir.cc (get_generic_anonymous_internal_type_name): Define
new static function.
(get_type_name): For internal purposes, make the type name of all
anonymous types of a given kind to be the same. This allows the
internal representation of anonymous types which are based on type
names to all be the same, so that they can be compared among
themselves during type canonicalization.
* tests/data/test-read-dwarf/test-PR26568-{1,2}.c: Source code of
binary test input.
* tests/data/test-read-dwarf/test-PR26568-{1,2}.o: New binary test input.
* tests/data/test-read-dwarf/test-PR26568-{1,2}.o.abi: New
reference test ouput.
* tests/data/Makefile.am: Add the new test material above to
source distribution.
* tests/test-read-dwarf.cc (in_out_specs): Add the new binary test
input above to this test harness.
* tests/data/test-diff-dwarf-abixml/PR25409-librte_bus_dpaa.so.20.0.abi: Adjust.
* tests/data/test-read-dwarf/PR22122-libftdc.so.abi: Likewise.
* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When handling a binary with abidiff or abidw it can be useful to
provide several different header files directories, for the cases
where the header files of the binary are scathered in several
different directories.
It thus becomes possible to invoke abidiff like this:
abidiff --headers-dir1 first-header-dir1 \
--headers-dir1 second-header-dir1 \
--headers-dir2 first-header-dir2 \
--headers-dir2 second-header-dir2 \
binary1 binary2
This patch adds support for that. It also modifies the
tests/test-abidiff-exit.cc test harness to make it take header
directories. With that modification done, a new test is added in that
harness to exercise this new feature.
This should close the feature request over at
https://sourceware.org/bugzilla/show_bug.cgi?id=26565.
* doc/manuals/abidiff.rst: Update documentation for the
--headers-dir{1,2} options.
* doc/manuals/abidw.rst: Likewise for the --header-dir option.
* include/abg-tools-utils.h (gen_suppr_spec_from_headers): Add new
overload that takes a vector of headers root dirs.
* src/abg-tools-utils.cc (gen_suppr_spec_from_headers_root_dir):
Define new function.
(gen_suppr_spec_from_headers): Define a new overload that takes a
vector of head_root_dir strings; it uses the new
gen_suppr_spec_from_headers function. Use the new overload in the
previous one that takes just one head_root_dir string.
* tools/abidiff.cc (options::headers_dirs{1,2}): Rename
option::headers_dir{1,2} into this one and make it be a vector of
strings rather than just a string.
(parse_command_line): Support several --headers-dir{1,2} on the
command line.
(set_diff_context_from_opts, set_suppressions): Adjust.
* tools/abidw.cc (options::headers_dirs): Renamed
options::headers_dir into this and make it be a vector of strings
rather than just a string.
(parse_command_line): Support several --headers-dir on the command
line.
(set_suppressions): Adjust.
* tests/data/test-abidiff-exit/test-headers-dirs/headers-a/header-a-v{0,1}.h:
Header files of new binary test input.
* tests/data/test-abidiff-exit/test-headers-dirs/headers-b/header-b-v{0,1}.h:
Likewise.
* tests/data/test-abidiff-exit/test-headers-dirs/test-headers-dir-v{0,1}.c:
Source code of new binary test input.
* tests/data/test-abidiff-exit/test-headers-dirs/test-headers-dir-report-{1,2}.txt:
Reference output of new binary test input.
* tests/data/test-abidiff-exit/test-headers-dirs/test-headers-dir-v{0,1}.o:
New binary test input.
* tests/data/Makefile.am: Add the new files above to source
distribution.
* tests/test-abidiff-exit.cc (InOutSpec::in_elfv{0,1}_path): Add
new data members.
(in_out_specs): Adjust the content of this array as its type
changed. Also, add two new entries to run the test over the new
binary test inputs above.
(do_prefix_strings): Define new static function.
(main): Use it the new do_prefix_strings here. Make abidiff
use the --header-dir{1,2} option whenever header directories are
specified in an entry of the in_out_specs array.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
In leaf mode, libabigail fails to report changes to the underlying
type of a typedef.
At its core, this is due to the fact that changes to the underlying
type of a typedef are not considered local. As the leaf reporter only
reports local changes (as opposed to non-local changes which are
changes to sub-types) it doesn't detect those non-local typedef
changes.
To handle this, this patch makes changes to the underlying type of a
typedef be considered as local changes. This is like what we already
do for pointer and qualified types.
Now that we have another set of changes to report in the leaf
reporter, we need to handle how to propagate the category and
redundancy status of those changes. The patch does this too.
Also, just like we do pointer and qualified type changes, the patch
avoids marking the diff node carrying the typedef change as being a
leaf change. That way, only existing leaf changes carrying that
typedef diff node will be reported. For instance, a function whose
parameter has a typedef change will be reported because that change to
the function is considered a leaf change. Otherwise, reporting the
typedef (or the pointer or qualified) type change on its own is not
useful unless it impacts those leaf changes that we deem useful.
The patch adds the example given in problem report to the testsuite.
* src/abg-ir.cc (equals): In the overload for typedef_decls,
report changes to the underlying type as being local of kind
LOCAL_TYPE_CHANGE_KIND.
* src/abg-comparison.cc
(leaf_diff_node_marker_visitor::visit_begin): Do not mark typedef
diff node as leaf node.
(suppression_categorization_visitor::visit_end): Propagate the
'suppressed' category of the underlying type to the parent typedef
unless the later has a local non-type change.
(redundancy_marking_visitor::visit_end): Likewise for the
'redundant' category.
* include/abg-reporter.h (report_non_type_typedef_changes): Rename ...
* src/abg-default-reporter.cc (report_non_type_typedef_changes):
... report_local_typedef_changes into this.
* src/abg-leaf-reporter.cc (leaf_reporter::report): Make the leaf
reporter invoke the reporting method of the default reporter for
typedefs as all typedef changes are now local.
* tests/data/test-diff-filter/test-PR26309-report-0.txt: Add new
test reference output.
* tests/data/test-diff-filter/test-PR26309-v{0,1}.o: Add new test
binary input.
* tests/data/test-diff-filter/test-PR26309-v{0,1}.c: Add source
code for new test binary input.
* tests/data/Makefile.am: Add the new text material above to
source distribution.
* tests/test-diff-filter.cc (in_out_specs): Add the new test input
above to this test harness.
* 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.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When using type maps the hash used for a given type T is usually the
pointer value of the canonical type of T.
In the rare cases where T doesn't have a canonical type, we were using
a 'dynamic hash' computed by recursively walking the components of T
and progressively building a hash that way.
The dynamic hashing code hasn't been updated in a while and would need
some overhaul to support hashing with schemes like MD5 or maybe sha
even. It might be useful for various use cases that have been
proposed by some users over the years but nobody was motivated enough
to implement it.
In the mean time, rather than trying to come up with a fully beefed up
dynamic hashing code, we'd rather just return a constant number for
non canonicalized types. In practise that amounts to forcing the code
of the maps to always use structural comparison for those non
canonicalized types.
Note that the amount of non-canonicalized types should be fairly
small. For now, the only non-canonicalized types should be
declaration-only types and those are quite fast to compare anyway.
This patch thus introduces a new hashing scheme for maps in the
writer which just uses a numerical constant as the hash for
non-canonicalized types.
* include/abg-fwd.h (hash_as_canonical_type_or_constant): Declare ...
* src/abg-ir.cc (hash_as_canonical_type_or_constant): ... new
function.
* src/abg-writer.cc (type_hasher::operator()): Use the new
hash_as_canonical_type_or_constant.
* tests/data/test-read-dwarf/test16-pr18904.so.abi: Adjust.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
During the canonicalization of a type T, the algorithm uses the
internal pretty representation of T to limit the number of types to
compare T to. That internal pretty representation is based on the
type name.
For anonymous types, the type name is not unique; it's constructed
just for internal purposes. So using that in the pretty
representation might negatively impact the accuracy of the
canonicalization; it might make it so that two types might wrongly be
considered canonicaly different.
To fix that, this change makes the internal pretty representation of
anonymous classes (and unions) use their flat representation.
For the record, the flat representation of an anonymous struct with a
an integer and a char data members is the string:
'struct {int i; char c;}'
* src/abg-ir.cc ({class, union}_decl::get_pretty_representation):
Use the flat representation of the class or union even for
internal purposes.
* tests/data/test-annotate/libtest23.so.abi: Adjust.
* tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt: Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt: Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt: Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt: Likewise.
* tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt: Likewise.
* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt: Likewise.
* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Likewise.
* tests/data/test-read-dwarf/libtest23.so.abi: Likewise.
* tests/data/test-read-dwarf/test11-pr18828.so.abi: Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Bug 26297: Possible misinterpretation of DW_AT_declaration via DW_AT_specification
A DIE representing a definition can refer to the DIE that represents
the matching declaration by using the DW_AT_specification attribute.
A similar situation exists for a cloned entity (like a cloned variable
or function) where the DW_AT_abstract_origin points to the original
entity.
Usually, to get the union of the attributes for a given definition
DIE, we also need to gather the attributes carried by the declaration
(or original) DIE accompanying the current definition DIE.
For the "is_declaration" attribute however, we should only look at the
current (definition) DIE at hand. Said otherwise, we should not
follow declaration/origin link when we want to know if a given entity
is declaration-only.
Thus, this commit causes the DWARF reader to only consider a DIE to be
"declaration only" if all DIEs in the chain leading to it have the
DW_AT_declaration attribute set.
It is a follow-up commit to a change making die_is_declaration_only
examine just the immediate DIE.
The responsibility of tracking the cumulative declaration-only status
of DIEs falls on build_ir_node_from_die which is the function that
makes recursive calls to itself on encountering a DW_AT_specification
or DW_AT_abstract_origin link.
Various other functions that would have previously called
die_is_declaration_only are modified so that get the cumulative value
for this flag, rather than just examing the DIE they are given.
This change eliminates a lot of spurious declaration-only types in ABI
output and may also prevent the same, particularly when anonymous,
from confusing libabigail's type equality and canonicalisation logic.
* src/abg-dwarf-reader.cc (add_or_update_class_type): Add an
is_declaration_only argument. Use this in favour of the
die_is_declaration_only helper function.
(add_or_update_union_type): Ditto.
(function_is_suppressed): Ditto.
(build_or_get_fn_decl_if_not_suppressed): Ditto.
(build_enum_type): Ditto.
(build_ir_node_from_die): To the main overload, add
is_declaration_only argument and default this to true.
Update this to false if the given DIE is not declaration
only and pass this on in recusrive calls and calls to
build_enum_type, add_or_update_union_type,
add_or_update_class_type and
build_or_get_fn_decl_if_not_suppressed.
* tests/data/test-annotate/test17-pr19027.so.abi: Update
test. This is mostly the removal of is-declaration-only
attributes, removal of unreachable parts of the type graph and
type id renumbering.
* tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Likewise.
* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Likewise.
* tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Likewise.
* tests/data/test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1-report-0.txt:
Likewise.
* tests/data/test-read-dwarf/test17-pr19027.so.abi: Likewise.
* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Likewise.
* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Likewise.
* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Likewise.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Bug 26297: Possible misinterpretation of DW_AT_declaration via DW_AT_specification
The DWARF attribute DW_AT_declaration indicates that a DIE is
"declaration only". DWARF DIEs can be linked with DW_AT_specification
and DW_AT_abstract_origin attributes, effectively combining them. A
lone DW_AT_declaration in a chain of such DIEs should not render the
whole chain declaration only.
The function die_is_declaration_only currently traverses such links in
search of the attribute which precludes being able to check for the
attribute at each DIE in the chain and some DIEs are mistakenly
treated as declaration-only by the DWARF reader.
This commit changes die_is_declaration_only to examine the given DIE
only. It extends the die_flag_attribute function so that it can
perform a direct as well as a recursive attribute search. The function
die_die_attribute's 'look_thru_abstract_origin' argument is renamed to
'recursively' to match.
A following commit will change the DWARF reader to ensure it takes
note of the declaration-only status of all DIEs in a chain.
* src/abg-dwarf-reader.cc (die_die_attribute): Rename
'look_thru_abstract_origin' argument to 'recursively' and
mention DW_AT_specification in its doc comment. Remove stale
comment for non-existent argument. Simplify code with the help
of the ternary operator. (die_flag_attribute): Add
recursively argument, defaulted to true. If this is false,
look for attribute using dwarf_attr rather than
dwarf_attr_integrate. (die_is_declaration_only): Call
die_flag_attribute specifying non-recursive attribute search.
* tests/data/test-annotate/test15-pr18892.so.abi: Update
tests. This is mostly the removal of unreachable parts of the type
graph and type id renumbering.
* tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Likewise.
* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Likewise.
* tests/data/test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1-report-0.txt:
Likewise.
* tests/data/test-read-dwarf/test15-pr18892.so.abi: Likewise.
* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Likewise.
* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Likewise.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Likewise.
Signed-off-by: Giuliano Procida <gprocida@google.com>
* tools/abidw.cc (display_usage): In documentation of
"--type-id-style" option, add a missing closing ')', spell
"type id" without a '-', split overly long string over two
lines, use "<...>" to indicate mandatory argument and improve
description of formats.
* doc/manuals/abidw.rst: In documentation of "--type-id-style"
option, use "<...>" to indicate mandatory argument.
Signed-off-by: Giuliano Procida <gprocida@google.com>
The function maybe_report_data_members_replaced_by_anon_dm has a bug
and some minor issues. This commit tidies these up.
The function came to my attention when a broken equality test
triggered an infinite loop.
The issues were:
- dms_replaced_by_same_anon_dm declared outside loop, gets reused
- self-comparison of first decl, potential infinite loop
- anonymous_data_member, assigned but not used
- two iterators i, j used, when one would suffice
The first issue results in incorrect diff reports if data members
in a structure are replaced by more than one anonymous data member.
This commit adds additional test cases for this, following the pattern
used for the existing PR25661 ones.
The second issue only affects behaviour if equality is defined
inconsistently with object identity.
* src/abg-reporter-priv.cc
(maybe_report_data_members_replaced_by_anon_dm): Move
declarations of anonymous_data_member and
dms_replaced_by_same_anon_dm into inner loop. Use
anonymous_data_member for testing and reporting, allowing
iterators i and j to be replaced by just iterator i. Push
first decl onto dms_replaced_by_same_anon_dm unconditionally
and move control flow logic into loop condition.
* tests/data/Makefile.am: Add new test cases.
* tests/data/test-diff-filter/test-PR25661-7-report-1.txt: New
test case file.
* tests/data/test-diff-filter/test-PR25661-7-report-2.txt:
Likewise.
* tests/data/test-diff-filter/test-PR25661-7-report-3.txt:
Likewise.
* tests/data/test-diff-filter/test-PR25661-7-report-4.txt:
Likewise.
* tests/data/test-diff-filter/test-PR25661-7-v0.c: Likewise.
* tests/data/test-diff-filter/test-PR25661-7-v0.o: Likewise.
* tests/data/test-diff-filter/test-PR25661-7-v1.c: Likewise.
* tests/data/test-diff-filter/test-PR25661-7-v1.o: Likewise.
* tests/test-diff-filter.cc: Call new test cases.
Signed-off-by: Giuliano Procida <gprocida@google.com>
When two decl_base ojects are compared, there are both fast and slow
paths to the name comparison. The latter is roughly equivalent to
comparing names after applying the regex
s/(__anonymous_(?:struct|union|enum)__)\d+/\1/g to the names before
comparing them while the former is a straight string comparison with
some tweaks for detecting anonymous types.
The slow path is taken care of by the helper function
tools_utils::decl_names_equal but unfortunately, there is a missing
negation of the returned bool. This commit fixes this and updates the
few affected tests.
Rather than just adding a '!', this commit replaces the negative
decls_are_different with a positive decls_are_same. I spent far too
long staring at the code before I spotted the mistake and having
positively-named things improves readability.
The same helper function is also called by has_harmless_name_change
and that should be reviewed as well.
* src/abg-ir.cc (equals): In the decl_base overload, note that
the value returned by decl_names_equal should be negated and
replace decls_are_different with decls_are_same, negating all
occurrences.
* tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt:
Update tests, removing some spurious anonymous union name change.
* tests/data/test-diff-filter/test33-report-0.txt: Diff now
completely empty.
* tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt:
3 functions previously considered to have harmless changes are
now deemed to have no changes.
* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
1 struct RedStore data member previously considered to have
harmless changes is now deemed to have no changes.
* tests/data/test-read-dwarf/test9-pr18818-clang.so.abi:
One instance of an anonymous struct removed and a typedef
repointed at another existing instance; many type ids
renumbered.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This utility function compares qualified names component-wise with
special handling of components that are anonymous names.
The code is incorrect for some cases when there are different numbers
of components on each side. Also, the control flow in the loop body is
more complex than it needs to be.
This commit simplifies the control flow, fixes the comparison bugs and
adds some extra tests to cover these cases.
* src/abg-tools-utils.cc (decl_names_equal): Move {l,r}_pos2
declarations into the loop and make {l,r}_length const. Avoid
chance of arithmetic on string::npos values. Rework
logic so there is a single test for "names compare equal" and
a single test for different numbers of name components.
* tests/test-tools-utils.cc (main): Add nine more tests.
Signed-off-by: Giuliano Procida <gprocida@google.com>
In the function compare_dies, the handling of DW_TAG_subroutine_type
was unfinished.
This DIE represents function types pointed-to by function pointer. It
doesn't represent the type of a "real" function type like the
abigail::ir::function_type would. The represent more an interface to
a function, which a pointer can point to or which can be used to issue
a call.
So intended idea is that compare_dies compares two DIEs of the
DW_TAG_subroutine_type and DW_TAG_subprogram kind (the later
represents real function definitions) among other kinds, structurally,
but by trying to optimize for speed, for the purpose of canonicalizing
DIEs even before type canonicalization happens at the libabigail IR
level. This is critical to save space (and time) by doing DIE
de-duplication on huge binaries.
This patch finishes to implement the comparison for
DW_TAG_subroutine_type and comes with a carefully crafted test case
that hits that code path.
* src/abg-dwarf-reader.cc (compare_dies): Get out early if we are
are in the middle of a potential recursive comparison of function
types. Likewise if we detect that the two function types have
different textual representations, linkage names, or have a the
same textual representation, linkage names and are defined in the
same translation unit.
* tests/data/test-read-dwarf/PR26261/PR26261-exe: New test binary
input file.
* tests/data/test-read-dwarf/PR26261/PR26261-exe.abi: New
reference test output file.
* tests/data/test-read-dwarf/PR26261/PR26261-main.c: Source code
of the binary above.
* tests/data/test-read-dwarf/PR26261/PR26261-obj{a,b}.{c,h}:
Likewise.
* tests/data/test-read-dwarf/PR26261/Makefile: Makefile to
build the exe out of the source files.
* tests/data/Makefile.am: Add the new test input files to source
distribution.
* tests/test-read-dwarf.cc (in_out_spec): Add the binary test
input above to the test harness.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
While looking at something else, I noticed that a block of code in
compare_dies wasn't properly indented. Fixed thus.
* src/abg-dwarf-reader.cc (compare_dies): Properly indent a
sub-block of the big switch case statement in there.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The commit
dbef379a Support incomplete enums in core and diff code.
added a duplicated line of code in error. This commit removes it.
* src/abg-ir.cc (decl_base::set_definition_of_declaration):
Remove duplicated assignment statement.
Signed-off-by: Giuliano Procida <gprocida@google.com>
The XML writer produces valid XML. However, it differs in a few
respects from that obtained with xmllint --format.
- there is no XML declaration at the start
- attributes use single quotes rather than double quotes
- indentation is mostly 2 spaces but this is broken in places
This commit fixes the last of these issues as it actually causes
readability issues when examining diffs. It also does this for every
test XML file, whether used as input, compared against output or not
used at all, to match what xmllint --format would do.
* src/abg-writer.cc (write_canonical_types_of_scope): Do not
add additional indentation. (write_translation_unit): Pass
additional indentation to write_canonical_types_of_scope.
(write_class_decl): Ensure optional annotations of base
classes have the same indentation as the base classes
themselves.
* tests/data/test-annotate/libtest23.so.abi: Fix indentation.
* tests/data/test-annotate/libtest24-drop-fns-2.so.abi: Ditto.
* tests/data/test-annotate/libtest24-drop-fns.so.abi: Ditto.
* tests/data/test-annotate/test-anonymous-members-0.o.abi:
Ditto.
* tests/data/test-annotate/test1.abi: Ditto.
* tests/data/test-annotate/test14-pr18893.so.abi: Ditto.
* tests/data/test-annotate/test15-pr18892.so.abi: Ditto.
* tests/data/test-annotate/test17-pr19027.so.abi: Ditto.
* tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Ditto.
* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Ditto.
* tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Ditto.
* tests/data/test-annotate/test21-pr19092.so.abi: Ditto.
* tests/data/test-diff-suppr/test45-abi-wl.xml: Ditto.
* tests/data/test-diff-suppr/test45-abi.xml: Ditto.
* tests/data/test-diff-suppr/test46-PR25128-base.xml: Ditto.
* tests/data/test-diff-suppr/test46-PR25128-new.xml: Ditto.
* tests/data/test-read-dwarf/PR22015-libboost_iostreams.so.abi:
Ditto.
* tests/data/test-read-dwarf/PR22122-libftdc.so.abi: Ditto.
* tests/data/test-read-dwarf/libtest23.so.abi: Ditto.
* tests/data/test-read-dwarf/libtest24-drop-fns-2.so.abi:
Ditto.
* tests/data/test-read-dwarf/libtest24-drop-fns.so.abi: Ditto.
* tests/data/test-read-dwarf/test1.abi: Ditto.
* tests/data/test-read-dwarf/test1.hash.abi: Ditto.
* tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Ditto.
* tests/data/test-read-dwarf/test11-pr18828.so.abi: Ditto.
* tests/data/test-read-dwarf/test12-pr18844.so.abi: Ditto.
* tests/data/test-read-dwarf/test14-pr18893.so.abi: Ditto.
* tests/data/test-read-dwarf/test15-pr18892.so.abi: Ditto.
* tests/data/test-read-dwarf/test16-pr18904.so.abi: Ditto.
* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Ditto.
* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Ditto.
* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Ditto.
* tests/data/test-read-dwarf/test21-pr19092.so.abi: Ditto.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Ditto.
* tests/data/test-read-dwarf/test9-pr18818-clang.so.abi: Ditto.
* tests/data/test-read-write/test10.xml: Ditto.
* tests/data/test-read-write/test15.xml: Ditto.
* tests/data/test-read-write/test21.xml: Ditto.
* tests/data/test-read-write/test28-without-std-fns-ref.xml:
Ditto.
* tests/data/test-read-write/test28-without-std-vars-ref.xml:
Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Following a change which eliminated the output of extra blank lines in
XML output, it now also makes sense to remove such blank lines from
saved generated XML files.
This commit does this.
* tests/data/test-abidiff/test-PR18166-libtirpc.so.abi: Remove
blank lines.
* tests/data/test-diff-dwarf-abixml/PR25409-librte_bus_dpaa.so.20.0.abi:
Ditto.
* tests/data/test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1.abi:
Ditto.
* tests/data/test-diff-suppr/test45-abi-wl.xml: Ditto.
* tests/data/test-diff-suppr/test45-abi.xml: Ditto.
* tests/data/test-diff-suppr/test46-PR25128-base.xml: Ditto.
* tests/data/test-diff-suppr/test46-PR25128-new.xml: Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
In one case there is an attempt to "pop" an in-progress type
comparison which hasn't actually happened.
This commit fixes this.
* src/abg-ir.cc (equals): In the class_or_union overload,
replace one instance of RETURN(false) with return false.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Ths warning is no longer triggered and can be reenabled.
* configure.ac: Remove the special clause that disabled
-Werror-overloaded-virtual for Clang builds.
Signed-off-by: Giuliano Procida <gprocida@google.com>
The classes class_decl, class_or_union and scope_decl derive from each
other. The method insert_member_decl is declared virtual and defined
in each of these. Unfortunately, it has different argument types in
the base scope_decl class.
Most calls to insert_member_decl are at a statically known class, but
in insert_decl_into_scope the method is called via a scope_decl
pointer. There is the possibility that this could be a type derived
from scope_decl rather than scope_decl itself, in which case the base
method would be called, not as intended.
This commit adjusts the type of the member argument to
scope_decl::insert_member_decl to match the other two classes and
eliminates the last trigger of Clang's -Werror-overloaded-virtual.
* include/abg-ir.h (scope_decl::insert_member_decl): Change
type of member argument from const decl_base_sptr& to plain
decl_base_sptr.
* src/abg-ir.cc (scope_decl::insert_member_decl): Likewise.
Signed-off-by: Giuliano Procida <gprocida@google.com>
The traverse method is defined in the traversable_base class but is
never used as the synonymous methods in the derived classes have
different argument types and so hide it.
It's never used because it's mostly intended as documentation for what
the implementations of this interface should look like. Namely they
should define a traversable method and its parameter type should
derive from the parameter type of traversable_base::traverse.
But apparently, clang's -Werror-overloaded-virtual is not happy about
this. It flags it as an error. It's not. But hey, let's work-around
it then.
So this patch just comments that method out and document its intent.
To make the change somewhat useful, this patch pimpl-ifies this
abg-traverse.h header file to get us one step closer to some a{b,p}i
stability. The definitions are moved into abg-traverse.cc.
* include/abg-traverse.h (traversable_base::priv): Declare new type.
(traverse_base::priv_sptr): Add pointer to private data
member.
(traverse_base::visiting_): Move this data member definition into
traverse_base::priv.
(traverse_base::{visiting, traverse_base, ~traverse_base}): Move
definitions out-of-oline.
(traverse_base::traverse): Comment out.
* src/abg-traverse.cc (struct traversable_base::priv): Define new
type.
(traversable_base::{traversable_base, ~traversable_base, traverse,
visiting}): Move these previous inline definitions here.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The second argument to this function controls whether CV qualifiers
should be stripped as well as the elements mentioned in the function
name. While it defaults to true, it is now always passed in as false.
The function contains incomplete code for peeling array types. This is
not needed or indeed wanted by its current callers.
This commits removes the peel_qual_type argument and removes the
associated behaviour. It removes the array peeling code.
There are no functional changes apart from no longer performing early
canonicalisation of certain array types.
I looked at the history of this function to see where the behaviours
originated.
bd161caa Make type_has_non_canonicalized_subtype() tighter
The function is added to help make more types, self-referential ones,
candidates for early canonicalisation.
5822798d Bug 18894 - Fix representation of enumerators in abixml format
This undid the previous change but the function remained.
c20c8c79 Fix infinite loop in peel_typedef_pointer_or_reference_type
As it says, fixing the overload that is currently in use.
6e36a438 Late canonicalize all types that reference classes when reading DWARF
This reintroduced the use of the function to control canonicalisation
by the detection of class types. It also added array peeling to the
function, but in a broken fashion as it would only work for certain
combinations of pointers, references or typedefs referring to arrays.
e9bdb488 Bug 19025 - abixml writer forgets to emit some member types
This added a use of the function in a map key comparison function.
8cc382c8 Fix emitting of referenced type in abixml writer
This undid the previous change.
1bee40c0 Do not forget to peel qualified type off when peeling types
This made the function remove CV qualifiers unconditionally.
e73901a5 Do not mark "distinct" diff nodes as being redundant
This made behaviour to remove CV qualifiers optional and newly added
is_mostly_distinct_diff disabled it.
5d6af8d5 Delay canonicalization for array and qualified types
This change switches maybe_canonicalize_type to not request CV
qualifer peeling from peel_typedef_pointer_or_reference_type.
It partially resolves the array type issue as they are separately
checked for. Presumably they shouldn't be peeled, but still are under
some circumstances.
The tests here could be subject to further refinement. Many types have
delayed canonicalisation already.
9cf76b11 abg-ir.cc: Improve types_have_similar_structure.
This change replaced the use of the function with a more delicate
matched peeling process for pointer and reference types plus
peel_qualified_or_typedef_type. It obsoleted the behaviour where CV
qualifiers were stripped.
* include/abg-fwd.h (peel_qualified_or_typedef_type): Remove
second argument in declarations of both overloads.
* src/abg-comp-filter.cc (is_mostly_distinct_diff): Remove
second argument to peel_qualified_or_typedef_type.
* src/abg-dwarf-reader.cc (maybe_canonicalize_type): Likewise.
* src/abg-ir.cc (peel_qualified_or_typedef_type): In both
overloads, remove second argument peel_qual_type, simplify
code with the assumption it was always false and remove
incomplete array type peeling logic. In type_base_sptr
overload, remove stray space.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The function is_reference_or_pointer_diff was added in commit
85929105 Fix redundancy marking for change of types used directly
and was updated to peel typedefs as a first step in
ef9d20c9 Fix redundancy detection through fn ptr and typedef paths
which, however, also made it obsolete.
This commit removes the function's declaration and definition.
There are no functional changes.
* include/abg-comparison.h (is_reference_or_pointer_diff):
Drop function declaration.
* src/abg-comparison.cc (is_reference_or_pointer_diff): Drop
function definition.
Signed-off-by: Giuliano Procida <gprocida@google.com>
The functon is_reference_or_pointer_diff_to_non_basic_distinct_types
was declared in
ef9d20c9 Fix redundancy detection through fn ptr and typedef paths
but never defined. This commit removes it. There are no functional
changes.
* include/abg-comparison
(is_reference_or_pointer_diff_to_non_basic_distinct_types):
Remove stray declaration.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Many of the operator== definitions in this source file follow the same
pattern:
- First, canonical comparison is attempted if canonical types are
present.
- Otherwise, the comparison is performed structurally using the
'equals' function.
This commit refactors the common logic into a templated helper
function named "try_canonical_compare".
There are no behavioural changes.
* src/abg-ir.cc (try_canonical_compare): New template function.
(type_decl::operator==): Use it here.
(scope_type_decl::operator==): Likewise.
(qualified_type_def::operator==): Likewise.
(pointer_type_def::operator==): Likewise.
(reference_type_def::operator==): Likewise.
(array_type_def::subrange_type::operator==): Likewise.
(array_type_def::operator==): Likewise.
(enum_type_decl::operator==): Likewise.
(typedef_decl::operator==): Likewise.
(function_type::operator==): Likewise.
(class_or_union::operator==): Likewise.
(class_decl::operator==): Likewise.
(union_decl::operator==): Likewise.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This commit fixes up some code whitespace for style and consitency,
renames a poorly-named variable and fixes a comment typo.
There are no behavioural changes.
* src/abg-comparison.cc (corpus_diff::priv::emit_diff_stats):
Adjust code whitespace; rename the second instance of
total_nb_variable_changes to
total_nb_unreachable_type_changes.
(corpus_diff::has_incompatible_changes): Fix comment typo.
Signed-off-by: Giuliano Procida <gprocida@google.com>
libabigail's intern_string class treats the empty string specially. It
is not safe to call the raw method without checking for a empty
pointer. It is safe to convert to std::string.
This commit changes the XML writer to convert interned strings to
std::strings before computing their hashes.
* src/abg-writer.cc (write_context::get_id_for_type): When
hashing internal type names, convert to std::string rather
than using the raw method directly as this will avoid a null
pointer dereference in the case of an empty string; tabify
code indentation.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Many of the operator== definitions in this source file follow the same
pattern:
- the address of the argument is dynamic_cast to type of 'this'
- naked canonical type pointers are compared, if both present
- the types are compared structurally with 'equals'
In a couple of cases extra work is done to fetch the canonical type
of the definition of a declaration.
This commit adjusts a few cases so they more closely follow the common
form. This is to make the next refactoring trivial.
There are no behavioural changes.
* src/abg-irc.cc (scope_type_decl::operator==): Compare naked
canonical type pointers instead of the shared pointers.
(qualified_type_def::operator==): Remove excess blank line.
(function_type::operator==): Do dynamic_cast and check of
argument before comparing naked canonical type pointers.
(class_or_union::operator==): Eliminate temporary reference.
(class_decl::operator==): Likewise.
(union_decl::operator==): Likewise.
Signed-off-by: Giuliano Procida <gprocida@google.com>
This function was not aware of --leaf-changes-only mode.
- Stats counters for changed variables and types have
different names in the different modes.
- Net leaf type changes were not included in leaf mode.
For some inputs, this resulted in abidiff producing an empty report
but returning a non-zero exit status in --leaf-changes-only mode.
For other inputs the combination of both issues still resulted in the
correct return code. This included the following test-abidiff-exit
test cases:
- test-leaf-peeling
- test-leaf2
- test-no-stray-comma
This patch makes corpus_diff::has_net_changes mirror emit_diff_stats,
modulo flags like --non-reachable-types which if absent can still
result in discrepancies between output and return code.
To achieve this in a more maintainable way, the patch introduces a new interface
reporter_base::diff_has_net_changes. That interface is implemented by
all current reporters. Each reporter focuses on its own
particularities to provide the required behavious. Then
corpus_diff:has_net_changes just has to invoke
reporter_base::diff_has_net_changes on the reporter that is currently
in used.
The tests below verify that the exit code is zero when all the changes
between the test files are suppressed.
* include/abg-reporter.h ({reporter_base, default_reporter,
leaf_reporter}::diff_has_net_changes): Add new virtual function.
This breaks binary compatibility but should conserve source
compatibility.
* src/abg-default-reporter.cc
(default_reporter::diff_has_net_changes): Define new member
function.
* src/abg-leaf-reporter.cc (leaf_reporter::diff_has_net_changes):
Likewise.
* src/abg-comparison.cc (corpus_diff::has_net_changes): Invoke
reporter_base::diff_has_net_changes on the current reporter,
rather than trying to handle all the different kinds of reporters
here.
(corpus_diff::priv::apply_filters_and_compute_diff_stats): Add a
TODO to possibly delegate the implementation of this function to
the reporters.
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-net-change-report0.txt:
Normal mode, nothing suppressed.
* tests/data/test-abidiff-exit/test-net-change-report1.txt:
Normal mode, everything suppressed.
* tests/data/test-abidiff-exit/test-net-change-report2.txt:
Leaf mode, nothing suppressed.
* tests/data/test-abidiff-exit/test-net-change-report3.txt:
Leaf mode, everything suppressions.
* tests/data/test-abidiff-exit/test-net-change-v0.c: Test file
* tests/data/test-abidiff-exit/test-net-change-v0.o: Test file
* tests/data/test-abidiff-exit/test-net-change-v1.c: Test file
* tests/data/test-abidiff-exit/test-net-change-v1.o: Test file
* tests/data/test-abidiff-exit/test-net-change.abignore: This
suppresses changes for all variables, functions and types in
the test files, except for the 'victim' function.
* tests/test-abidiff-exit.cc: Run new test cases.
Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The patch fixes two minor formatting typos.
* src/abg-reporter-priv.cc (represent): Add missing space to
string split across two lines in certain anonymous data member
diffs.
* src/abg-default-reporter.cc (report): In the array_diff
overload, eliminate trailing space at end of line.
* tests/data/test-diff-dwarf/test10-report.txt: Delete
trailing whitespace.
* tests/data/test-diff-filter/test24-compatible-vars-report-1.txt:
Ditto.
* 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:
Ditto.
* tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt:
Ditto.
* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt:
Ditto.
* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The XML writer has a few different styles of new line handling in
different places. Some functions are responsible for line termination,
others are not and there is bespoke logic and state variables in a few
places.
Extra or missing newlines should have no impact on the semantics of
any given ABI file but they do affect textual diffs and diff
statistics.
By insisting the XML emitted should have exactly one XML tag (or
comment) per line, we can simplify the code and make it more
composable.
This commit does this, yielding a modest reduction in code size and
eliminating all blank lines in XML output (7127 blank lines in current
tests). The commit also fixes some code whitespace.
* src/abg-writer.cc (annotate): In the
function_decl::parameter_sptr overload, fix code whitespace.
(write_decl_in_scope): Remove wrote_context state variable and
associated logic; emit new line unconditionally after end of
XML tags and nowhere else.
(write_canonical_types_of_scope): Emit new line after end of
XML comment and nowhere else.
(write_translation_unit): Emit new line after end of XML tags
and nowhere else.
(write_type_decl): Likewise.
(write_namespace_decl): Likewise.
(write_qualified_type_def): Emit new line after end of XML tag.
(write_pointer_type_def): Likewise.
(write_reference_type_def): Likewise.
(write_array_type_def): Emit new line after end of XML tags
and nowhere else.
(write_enum_type_decl): Emit new line after end of XML tag.
(write_elf_symbol): Likewise.
(write_elf_symbols_table): Emit no new lines.
(write_elf_needed): Emit new line unconditionally after end of
XML tags.
(write_typedef_decl): Emit new line after end of XML tag.
(write_var_decl): Emit new line after end of XML tag.
(write_function_decl): Likewise.
(write_function_type): Fold two output statements into
one; emit new line after end of XML tag.
(write_class_decl_opening_tag): Emit new line unconditionally
after end of XML tags and simplify empty element tag logic.
(write_union_decl_opening_tag): Likewise.
(write_class_decl): Emit new line after end of XML tag and
nowhere else.
(write_union_decl): Likewise.
(write_member_type_opening_tag): Emit new line after end of
XML tag.
(write_member_type): Emit new lines only after XML tags.
(write_type_tparameter): Emit new line after XML tag.
(write_non_type_tparameter): Likewise.
(write_template_tparameter): Emit new line after XML tag and
nowhere else.
(write_type_composition): Likewise.
(write_template_parameters): Emit no new lines.
(write_function_tdecl): Emit new line after XML tag and
nowhere else.
(write_class_tdecl): Likewise.
(write_corpus): Emit new lines only after XML tags.
(dump): In the decl_base_sptr overload, don't emit final new
line as this is now done by write_decl. In the var_decl_sptr
overload, don't emit final new line (mistakenly done to cerr
instead of o) as this is now done by write_var_decl. In the
translation_unit overload, don't emit final new line as this
doubles that emitted by write_translation_unit.
* tests/data/test-annotate/libtest23.so.abi: Delete all blank
lines.
* tests/data/test-annotate/libtest24-drop-fns-2.so.abi: Ditto.
* tests/data/test-annotate/libtest24-drop-fns.so.abi: Ditto.
* tests/data/test-annotate/test-anonymous-members-0.o.abi:
Ditto.
* tests/data/test-annotate/test1.abi: Ditto.
* tests/data/test-annotate/test13-pr18894.so.abi: Ditto.
* tests/data/test-annotate/test14-pr18893.so.abi: Ditto.
* tests/data/test-annotate/test15-pr18892.so.abi: Ditto.
* tests/data/test-annotate/test17-pr19027.so.abi: Ditto.
* tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Ditto.
* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Ditto.
* tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Ditto.
* tests/data/test-annotate/test21-pr19092.so.abi: Ditto.
* tests/data/test-annotate/test7.so.abi: Ditto.
* tests/data/test-read-dwarf/PR22015-libboost_iostreams.so.abi:
Ditto.
* tests/data/test-read-dwarf/PR22122-libftdc.so.abi: Ditto.
* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Ditto.
* tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
Ditto.
* tests/data/test-read-dwarf/libtest23.so.abi: Ditto.
* tests/data/test-read-dwarf/libtest24-drop-fns-2.so.abi:
Ditto.
* tests/data/test-read-dwarf/libtest24-drop-fns.so.abi: Ditto.
* tests/data/test-read-dwarf/test1.abi: Ditto.
* tests/data/test-read-dwarf/test1.hash.abi: Ditto.
* tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Ditto.
* tests/data/test-read-dwarf/test11-pr18828.so.abi: Ditto.
* tests/data/test-read-dwarf/test12-pr18844.so.abi: Ditto.
* tests/data/test-read-dwarf/test13-pr18894.so.abi: Ditto.
* tests/data/test-read-dwarf/test14-pr18893.so.abi: Ditto.
* tests/data/test-read-dwarf/test15-pr18892.so.abi: Ditto.
* tests/data/test-read-dwarf/test16-pr18904.so.abi: Ditto.
* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Ditto.
* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Ditto.
* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Ditto.
* tests/data/test-read-dwarf/test21-pr19092.so.abi: Ditto.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Ditto.
* tests/data/test-read-dwarf/test7.so.abi: Ditto.
* tests/data/test-read-dwarf/test7.so.hash.abi: Ditto.
* tests/data/test-read-dwarf/test9-pr18818-clang.so.abi:
Ditto.
* tests/data/test-read-write/test10.xml: Ditto.
* tests/data/test-read-write/test15.xml: Ditto.
* tests/data/test-read-write/test21.xml: Ditto.
* tests/data/test-read-write/test25.xml: Ditto.
* tests/data/test-read-write/test28-without-std-fns-ref.xml:
Ditto.
* tests/data/test-read-write/test28-without-std-vars-ref.xml:
Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
In the abixml reader, WIP types are tracked to know if a type has been
fully constructed yet or not. This information is later useful to
know if a given type should be canonicalized right away, or if its
canonicalization should be delayed until the entire abixml file has
been read.
Right now, with all the evolutions that happened in the abixml reader,
only scalar types are canonicalized right away. All other types are
canonicalized late, meaning, after the entire abixml file is read.
This doesn't have any noticeable performance impact because the volume
of types coming from an abixml file is relatively small enough,
compared to what we can see in a DWARF/ELF binary due to type
duplication.
So the whole WIP tracking becomes is now pretty much useless, in
practise. So this patch does away with it altogether.
* src/abg-reader.cc (read_context::m_wip_types_map): Remove data
member.
(read_context::{clear_wip_classes_map, mark_type_as_wip,
unmark_type_as_wip, is_wip_type}): Remove member functions.
(read_context::maybe_canonicalize_type): Remove use of
is_wip_type.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
During structural comparison of types there is the possibilitiy of
infinite recursion as types can have self-references and there can
be more elaborate mutual references between them.
The current comparison algorithm keeps track of currently seen (struct
and function) types by name. This causes earlier caching of names than
is needed and, more significantly, may result in types comparing equal
unexpectedly. This commit switches to storing their addresses instead.
This change affects some tests which show more diffs than previously.
src/abg-ir.cc: (environment::priv): Change types of
classes_being_compared_ and fn_types_being_compared_ to be
simple sets of pointers.
(function_type::priv::mark_as_being_compared): Just add
address to set.
(function_type::priv::unmark_as_being_compared): Just remove
address from set.
(function_type::priv::comparison_started): Just look up
address in set.
(class_or_union::priv::mark_as_being_compared): Just add
address to set.
(class_or_union::priv::unmark_as_being_compared): Just remove
address from set.
(class_or_union::priv::comparison_started): Just look up
address in set.
* tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt:
Update.
* tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt:
Update.
* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
Update.
* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-3.txt:
Update.
Signed-off-by: Giuliano Procida <gprocida@google.com>
* tests/data/test-abidiff-exit/test-decl-enum-report-2.txt: Add
new test reference output.
* tests/data/test-abidiff-exit/test-decl-enum-report-3.txt: Likewise.
* tests/data/test-abidiff-exit/test-decl-enum-report.txt: Likewise.
* tests/data/test-abidiff-exit/test-decl-enum-v{0,1}.c: Add source
code for the binaries below.
* tests/data/test-abidiff-exit/test-decl-enum-v{0,1}.o: Add new
binary test inputs.
* tests/data/Makefile.am: Add the new files above to source
distribution.
* tests/test-abidiff-exit.cc: Add the test inputs above to the
test harness.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This patch adds declaration-only handling enums to the DWARF reader.
* src/abg-dwarf-reader.cc (string_enums_map): Define new
convenience typedef.
(read_context::decl_only_enums_map_): Define new data member.
(read_context::{declaration_only_enums,
is_decl_only_enum_scheduled_for_resolution,
resolve_declaration_only_enums}): Define new member functions.
(build_internal_underlying_enum_type_name)
(build_enum_underlying_type): Factorize these functions out of ...
(build_enum_type): ... here. Detect a decl-only enum and flag it
as such. If the enum type is decl-only, then set its underlying
type as decl-only as well.
(build_enum_underlying_type): Mark the underlying type as
artificial.
(get_opaque_version_of_type): Make this handle enums as well. So
make its return type be type_or_decl_base_sptr, rather than just
class_or_union_sptr as it used to be.
(read_debug_info_into_corpus): Add logging to trace decl-only
enums resolution.
(build_ir_node_from_die): Detect when a suppression specification
makes an enum opaque. In that case, get an opaque version of the
enum type by invoking get_opaque_version_of_type. Note that
get_opaque_version_of_type doesn't support returning opaque
-- i.e, decl-only enum types -- yet, but this is going to be
handled in a subsequent patch.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
