While emitting an abixml output for a corpus_group representing the
KMI of a Linux kernel tree, profiling shows that during comparison of
class types, calling class_decl::get_definition_of_declaration is a
hotspot. Especially, the fact the function returns a shared pointer
that has to be "handled" shows up in the profile.
This patch introduces a
class_decl::get_naked_definition_of_declaration that returns a
pointer. Not a shared pointer.
The patch then uses that get_naked_definition_of_declaration function
in the comparison code for class_decl.
The patch also uses get_naked_canonical_type instead of
get_canonical_type when comparing a bunch of other types.
This makes things a little bit faster when compiled without
optimization between 2% and 5%.
* include/abg-ir.h
(class_or_union::get_naked_definition_of_declaration): Declare a
new member function.
(class_decl::get_naked_definition_of_declaration): Likewise.
* src/abg-ir.cc ({type_decl, qualified_type_def,
array_type_def, enum_type_decl}::operator==): Use the
get_naked_canonical_type and get_naked.
(class_or_union::priv::naked_definition_of_declaration_): Define
new data member.
(class_or_union::priv::priv): Adjust to initialize the new data
member.
(class_or_union::get_naked_definition_of_declaration): Define new
member function.
({class_or_union, class_decl}::operator==): Use the new
get_naked_definition_of_declaration instead of
get_definition_of_declaration.
(equals): In the overload for class_or_union, do the same.
(class_decl::get_naked_definition_of_declaration): Define new
member function.
(hash_type_or_decl): Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Profiling shows that while writting out the abixml for a big
corpus_group, comparing decl-only classes is a hot spot.
This patch avoids calling the function
class_or_union::priv_->comparison_started (which appears to be
culprit) in that case.
This makes writting out the abixml of the corpus_group of the Linux
kernel be ~20% faster.
* src/abg-ir.cc (equals): In the overload for class_decl, if we
are looking at a decl-only class, then directly call the equals
function for class_or_union. That one knows how to perform the
comparison without calling the
class_or_union::priv_->comparison_started function, in that case.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This updates the description of what abipkgdiff in termes of sequence
of actions.
* tools/abipkgdiff.cc: Update the description of the sequence of
actions performed.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* src/abg-writer.cc (class write_context): Fix indentation.
(write_location, write_visibility, write_binding)
(write_array_size_and_alignment, write_size_and_alignment): Fix
these declarations to use the *_sptr typedefs rather than the
explicit shared_ptr<*> types.
(write_translation_unit): Fix comment.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Suppose we have two types struct A and struct B. Suppose the two
types have members of integer type. Suppose there are members of
integer type in struct A that are modified to become members of char
type. Suppose, at last, that we also have members of integer type in
struct B that are modified to become members of char type.
In that case, we want to see all the changes of members which types
got changed from integer type to char type. None of these changes
should be considered redundant.
Today, unfortunately, only the first change is reported by default.
The subsequent changes are considered to be redundant.
This patch fixes that by considering that changes that modifies the type of a
decl from a basic type into another are never considered redundant.
* include/abg-comparison.h (is_diff_of_basic_type)
(has_basic_type_change_only): Declare these functions ...
* src/abg-comparison.cc (is_diff_of_basic_type)
(has_basic_type_change_only): ... and define them.
(redundancy_marking_visitor::visit_begin): Use the new
has_basic_type_change_only.
* tests/data/test-diff-filter/libtest37-v0.so: New binary test input.
* tests/data/test-diff-filter/libtest37-v1.so: Likewise.
* tests/data/test-diff-filter/test37-report-0.txt: New test
reference output.
* tests/data/test-diff-filter/test37-v0.cc: Source code of the new
binary test input.
* tests/data/test-diff-filter/test37-v1.cc: Likewise.
* tests/data/Makefile.am: Update to add the new test material to
the source distribution.
* tests/test-diff-filter.cc (in_out_spec): Add the new test input
to this test harness.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When {function, var}_decl::set_symbol is invoked, the cache of of the
ID of the decl must be invalidated because that function changes the
ID of the decl.
This patch does just that.
* src/abg-ir.cc ({function, var}_decl::set_symbol): Invalidate the
ID cache.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Now that there is a is_type predicate that takes a a type_or_decl_base
type, the overloads that take a decl_base or a type_base are useless
and can even lead to overload resolution issues. This patch removes
those useless overloads.
* include/abg-fwd.h (is_type): Remove the overloads that take
decl_base and type_base types.
* src/abg-ir.cc (is_type): Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Const references are a useless redundancy because a reference is
always const.
Thus build_qualified_type can return a non-qualified type when it
figures out the qualifier is useless.
This patch adjusts build_qualified_type to recognize that. It makes
it return a type_base_sptr rather than a qualified_type_def_sptr.
In build_ir_node_from_die, in the case where we handle qualified
types, the call to build_qualified_type is adjusted accordingly.
* src/abg-dwarf-reader.cc (build_qualified_type): Return a
type_base_sptr.
(build_ir_node_from_die): Adjust the call to build_qualified_type.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
In some case, some array subranges would all have the same lower
bound.
Fixed thus.
* src/abg-dwarf-reader.cc (build_subranges_from_array_type_die):
Consider the 'lower_bound' parameter as the default lower bound
for each sub-ranges.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
I noticed a buffer overrun in the equals overload for arrays. This
patch fixes that.
* src/abg-ir.cc (equals): In the overload for arrays, check for
the end of the subranges of the two arrays, not just for the first
one.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
It can happen that queue::priv_::do_bring_workers_down stays forever
waiting for a task to finish (via pthread_join). The it waits for is
itself blocked in worker::wait_to_execute_a_task, in pthread_cond_wait.
It seems to me that this is because we forget to lock the
queue::priv::queue_cond_mutex before inspecting and updating the
variables on which the wait on the condition depend.
This patch fixes that.
The patch also moves tests/test-read-write.cc over to using the work queue
to increase test coverage for the work queue interface.
* src/abg-workers.cc (queue::priv::tasks_todo_mutex): Make this
data member mutable.
(more_tasks_to_execute):
(queue::priv::do_bring_workers_down): Update the
queue::priv::bring_workers_down only in the critical section
defined by queue::priv::queue_cond_mutex.
(worker::wait_to_execute_a_task): Testing for
queue::priv::bring_workers_down is done in the critical section
defined by queue::priv::queue_cond_mutex. The loop over waiting
ont the condition is also in the critical section, as it ought to
be.
* tests/test-read-write.cc (struct test_task): New type.
(main): Express in terms of the new test_task type.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* include/abg-diff-utils.h (print_snake): pass argument of type
snake by const reference.
* include/abg-ir.h (location::operator{==,<}): Likewise.
* include/abg-viz-dot.h (node_base::{node_base,parent_node,child_node}):
Likewise.
* include/abg-viz-svg.h (svg::svg) Likewise.
* src/abg-config.cc (config::config): Member initialization in ctor body.
* src/abg-dwarf-reader.cc (class_decl_sptr::add_or_update_class_type):
Initial value never used.
* src/abg-ir.cc: (decl_base::priv::priv) Member initialization in ctor body,
pass argument of type location by const reference.
(equals): Variable initial value never used.
* src/abg-reader.cc (read_corpus_from_input): Initial variable
value never used.
(build_elf_symbol_db): Use pre-increment.
* src/abg-suppression-priv.h
(suppression_matches_type_location): Pass argument of type
location by const reference.
* src/abg-suppression.cc: Likewise.
Signed-off-by: Ondrej Oprala <ondrej.oprala@gmail.com>
* src/abg-dwarf-reader.cc
(type_or_decl_base_sptr::lookup_artifact_from_per_tu_die_representation):
Fix an error found by cppcheck.
Signed-off-by: Ondrej Oprala <ondrej.oprala@gmail.com>
* scripts/dot_to_png.sh: Clean up the script according to
shellcheck warnings and remarks.
* scripts/dot_to_svg.sh: Likewise.
* scripts/svg_to_plain_svg.sh: Likewise.
* scripts/svg_to_png_and_pdf.sh: Likewise.
Signed-off-by: Ondrej Oprala <ondrej.oprala@gmail.com>
* src/abg-ir.cc (parse_integral_type): An attempt at clang
compilation has discovered there to be a comparison with
unused result, that apparently should be an assignment.
Signed-off-by: Ondrej Oprala <ondrej.oprala@gmail.com>
We are seeing some random cases where the regression test
runtestfedabipkgdiff.py hangs in what seems to be a deadlock. This
seems to happen in fedabipkgidiff when it spawns a process to run
abipkgdiff. More precisely, proc.communicate() hangs.
The documentation of that function at
https://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate
says:
Note: The data read is buffered in memory, so do not use this
method if the data size is large or unlimited.
So this patch avoids using proc.communicate() because the output of
abipkgdiff *can* be large. Rather, the patch manually waits for the
the spawned abipkgdiff to finish, and then, read its output.
* tools/fedabipkgdiff (abipkgidff): Do not use Popen.communicate()
as it might hang if the data is large. Rather, busy wait for the
abipkgdiff process to finish and then get its output.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Package fails to build for 32bit architectures since size_t is not 64 bits.
Make header consistent with source
* include/abg-fwd.h: Include stdint.h for uint64_t.
(ir::set_data_member_offset): Take uint64_t rather than size_t.
(ir::get_data_member_offset): Return uint64_t rather than size_t.
Signed-off-by: Slava Barinov <v.barinov@samsung.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This fixes a race condition that seems to occur sometimes. That is,
if the fedabipkgdiff test is run last, then it can stay idling for
ever. I'll need to investigate this later.
* tests/Makefile.am: Run the fedabipkgdiff test first.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
References are always const. But then GCC sometimes emits DWARF that
represents a const reference. This leads, for instance, to a given
reference to be considered as different from that same reference wraps
into a const qualifier. Which is wrong.
Libabigail then represents those const references as a particular case
of a "no-op qualifier". That is, a qualifier that should be ignored
by the comparison code.
In the case of this issue, the comparison engine considers the two
references (const and non-const) to be equal, but the reporting code
forgets to ignore the ignore no-op qualifier and thus (wrongly)
considers the two references as being different. That inconsistency
leads to an abort of the process.
This patch moves the code that ignores no-op qualifiers at a lower
level of the comparison engine so that whenever function parameters
are compared, no-op qualifiers are ignored as they should.
* include/abg-fwd.h (look_through_no_op_qualified_type): Declare
new function.
* src/abg-ir.cc (look_through_no_op_qualified_type): Define it.
(compute_diff_for_types): Use the new
look_through_no_op_qualified_type here rather than open-coding it.
(equals): In the overload for function_decl::parameter, use the
new look_through_no_op_qualified_type function.
* tests/data/test-diff-dwarf/test40-PR21296-clanggcc.cc: Source
code of the new test inputs.
* tests/data/test-diff-dwarf/test40-PR21296-clanggcc-report0.txt:
New test input.
* tests/data/test-diff-dwarf/test40-PR21296-libgcc.so: New binary
test input.
* tests/data/test-diff-dwarf/test40-PR21296-libclang.so: Likewise.
* tests/test-diff-dwarf.cc (in_out_specs): Add the new test inputs to
the test harness.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Cache data, currently containing downloaded RPM packages from Koji, is
stored in XDG_CACHE_HOME. This patch allows user to delete cache before
or after the ABI comparison, or both.
* configure.ac: Require shutil module.
* doc/manuals/fedabipkgdiff.rst: Add document for new option
clean-cache, clean-cache-before, and clean-cache-after.
* tools/fedabipkgdiff (build_commandline_args_parser): Add new
option --clean-cache, --clean-cache-before and
--clean-cache-after.
(diff_local_rpm_with_latest_rpm_from_koji): Delete download
cache directory before or after downloading RPMs.
(diff_latest_rpms_based_on_distros): Likewise.
(diff_two_nvras_from_koji): Likewise.
(diff_from_two_rpm_files): Likewise.
* bash-completion/fedabipkgdiff: Add new options.
* tests/mockfedabipkgdiff.in (get_download_dir): Rewrite to
behave just like the original get_download_dir.
(mock_get_download_dir): Removed.
(DOWNLOAD_CACHE_DIR): New global variable pointing directory
holding packages during tests.
(run_fedabipkgdiff): Mock original get_download_dir with the
rewrite get_download_dir.
* tests/runtestfedabipkgdiff.py.in (run_fedabipkgdiff_tests):
Add --clean-cache to run tests to ensure no regression.
Signed-off-by: Chenxiong Qi <cqi@redhat.com>
* tests/test-valgrind-suppressions.supp: Add a suppression that
occurs during an internal libc signal handling occasion.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When one thread calls queue::wait_to_execute_a_task and another calls
queue::priv::do_bring_workers_down, a data race occurs on the
queue::priv::bring_workers_down variable.
This patch protects the read of the queue::priv::bring_workers_down
variable (in worker::wait_to_execute_a_task) by the
queue::priv::tasks_todo_mutex mutex. Note that that mutex is the one
that protects the write to queue::priv::bring_workers_down in
queue::priv::do_bring_workers_down.
* src/abg-workers.cc (worker::wait_to_execute_a_task): Protect the
read of the queue::priv::bring_workers_down down variable with the
queue::priv::tasks_todo_mutex.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
It turns out we allow member function cloning only for functions that
are members of classes, not for functions that are member of unions.
This patch fixes that by turning the method
class_decl::add_member_function into the
class_or_union::add_member_function. Note that there was already an
overload of class_or_union::add_member_function; now, all the member
functions add_member_function are members of the class_or_union type.
The patch then modifies function_decl::clone to make that code avoid
assuming that only member functions of classes can be cloned.
* include/abg-ir.h (class_or_union::add_member_function): Move the
class_decl::add_member_function overload declaration into the
class class_or_union class.
(class class_decl): Make the class class_or_union be a friend of
class_decl.
* src/abg-ir.cc (class_decl::add_member_function): Transform the
definition of this overload into ...
(class_or_union::add_member_function): ... this one. Make sure
that when setting the virtual-ness attributes of the member
function, we are effectively looking at the a function that is a
member of a class.
(function_decl::clone): Do not assert that a member function is
necessarily a member of a class_decl. It can also a member of a
union_decl!. So, rather, assert that the scope of the member
function is of type class_or_union.
* tests/data/test-diff-pkg/tbb-2017-8.20161128.fc26.x86_64.rpm:
New test input RPM.
* tests/data/test-diff-pkg/tbb-2017-9.20170118.fc27.x86_64.rpm:
* tests/data/test-diff-pkg/tbb-debuginfo-2017-8.20161128.fc26.x86_64.rpm:
Likewise.
* tests/data/test-diff-pkg/tbb-debuginfo-2017-9.20170118.fc27.x86_64.rpm:
Likewise.
* tests/data/test-diff-pkg/tbb-2017-8.20161128.fc26.x86_64--tbb-2017-9.20170118.fc27.x86_64.txt:
New reference test output.
* tests/data/Makefile.am: Add the new test input RPMs to the
source distribution.
* tests/test-diff-pkg.cc (in_out_specs): Take the new input tests
above into account.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When two virtual member functions have the same index, same ELF
symbol, same pretty representation and only differ from their source
file paths, then this patch takes the source path into account in the
sorting.
Otherwise, this breaks the runtestreaddwarf test on EL6.
* src/abg-ir.cc (virtual_member_function_less_than::operator()):
Take the file path into account in the sorting.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Adjust.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When two virtual members functions have the same virtual index, the
same pretty representation, and one of them has no ELF symbols, then
they might be sorted in a way or in an other. This sorting hazard
breaks the runtestreaddwarf test on EL6.
This patch addresses that to make the virtual member function that has
no ELF symbol to come before the one with an ELF symbol.
Also, it turned out that runtestannotate is broken on EL6 too because
we are trying to demangle c++11-mangled symbols in there, and the
demangler (which is C++<11 only) on that platform doesn't really like
that. So this patch removes the tests in which there are c++11 symbols
that we try to demangle.
* src/abg-ir.cc (virtual_member_function_less_than::operator()):
Update comment. When two virtual functions have the same virtual
index and one of them has no ELF symbol, then that function is
less than the one with an ELF symbol.
* tests/data/Makefile.am: Remove
test-annotate/{test9-pr18818-clang.so.abi, test11-pr18828.so.abi,
test12-pr18844.so.abi, test16-pr18904.so.abi,
test22-pr19097-libstdc++.so.6.0.17.so.abi}.
* tests/data/test-annotate/test10-pr18818-gcc.so.abi: Remove.
* tests/data/test-annotate/test11-pr18828.so.abi: Likewise.
* tests/data/test-annotate/test12-pr18844.so.abi: Likewise.
* tests/data/test-annotate/test16-pr18904.so.abi: Likewise.
* tests/data/test-annotate/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Likewise.
* tests/test-annotate.cc (in_out_specs): Remove those tests above
which input files have been removed.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* tests/test-valgrind-suppressions.supp: Make Helgrind
suppressions less specific to libgcc_s version.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* tests/test-valgrind-suppressions.supp: Make the ostream writting
suppressions be less specific so that they can apply to all the
related false positives.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Moving this test away from using pthreads directly to use
abigail::workers.
This patch also updates the valgrind suppression file to suppress some
Helgrind false positives. Those are due to:
- libstdc++ apparently having some benign data races when emitting
data to ostream. This seems related to some facet manipulation that
happen at that point.
- some benign data race in some elfutils functions.
* tests/test-read-dwarf.cc (iospec, spec_lock, write_lock)
(out_abi_base, in_elf_base, in_abi_base): Remove these global
variables.
(handle_in_out_spec): Remove this.
(struct test_task): Write this task that does what
handle_in_out_spec was doing.
(test_task_sptr): Define new typedef.
(main): Remove the pthreads artifacts. Use the new test_task type
along with the abigail::workers interface.
* tests/test-valgrind-suppressions.supp: Add more helgrind
suppressions for ostream writting false positives.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* tests/runtestfedabipkgdiff.py.in (run_fedabipkgdiff_tests): When
A test fails, display the fedabipkgdiff command that triggered the failure.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
With this patch, the code of abipkgdiff.cc doesn't use the pthreads
API directly anymore. Rather, it uses the higher level "Workers
Queue" API provided by the abigail::workers namespace.
The main benefits are:
- better code readability and maintainability. The code base doesn't
have any global variable anymore. This is going to be helpful when
adding new features to the abipkgdiff.cc code base.
- faster code. The tests/runtestdiffpkg test program executes on ~
17s without the patch, and on ~ 11s with the patch on my old X220
laptop.
* tools/abipkgdiff.cc: Remove ftw.h, pthread.h, unistd.h, add
fts.h and abg-workers.h.
(verbose, elf_file_paths_tls_key, reports_map, env_map, map_lock)
(arg_lock, prog_options): Remove all these global variables.
(struct package_descriptor): Remove this type.
(pthread_routine_extract_package)
(first_package_tree_walker_callback_fn)
(second_package_tree_walker_callback_fn, pthread_routine_compare)
(pthread_join, pthread_routine_extract_pkg_and_map_its_content):
Remove these functions.
(options::{num_workers, verbose}): Define new data members.
(options::options): Initialize the new verbose and num_workers data members.
(package::erase_extraction_directory)
(erase_created_temporary_directories_parent): Take the program
options in parameter. Don't use the global verbose variable
anymore.
(package::erase_extraction_directories)
(erase_created_temporary_directories, extract_package): Take the
program options in parameter.
(extract_rpm, extract_deb, extract_tar): Likewise. And don't use
the global verbose variable anymore.
(compare): Don't use the global verbose variable anymore. Use the
new compare_task type along with the abigail::workers::queue type.
(pkg_extraction_task, pkg_prepare_task, compare_task)
(comparison_done_notify): Define new classes.
(maybe_update_vector_of_package_content): Define new static
function.
(create_maps_of_package_content): Don't take the ftw_cp_type
anymore. Don't use the global verbose variable anymore. Use the
fts_{open,read,close} functions, rather than the ftw one.
(extract_package_and_map_its_content): Don't use pthreads anymore.
Use the new pkg_extraction_task type created along with the
abigail::workers::queue type.
(prepare_packages): Don't use pthreads anymore. Use the new
pkg_prepare_task type along with the abigail::workers::queue type.
(elf_size_is_greater): Adjust to use
abigail::workers::queue::tasks, rather than the previous
compaer_args_sptr type.
(parse_command_line): Adjust to stop using the global verbose
variable.
(main): Remove use of global variables prog_options and also the
packages variable.
* tests/data/test-diff-pkg/dbus-glib-0.104-3.fc23.x86_64--dbus-glib-0.104-3.fc23.armv7hl-report-0.txt:
Adjust.
* tests/data/test-diff-pkg/dirpkg-0-report-0.txt: Likewise.
* tests/data/test-diff-pkg/test-dbus-glib-0.80-3.fc12.x86_64-report-0.txt:
Likewise.
* tests/data/test-diff-pkg/test-rpm-report-0.txt: Likewise.
* tests/data/test-diff-pkg/test-rpm-report-1.txt: Likewise.
* tests/data/test-diff-pkg/test-rpm-report-2.txt: Likewise.
* tests/data/test-diff-pkg/test-rpm-report-3.txt: Likewise.
* tests/data/test-fedabipkgdiff/test0-from-fc20-to-fc23-dbus-glib-report-0.txt:
Likewise.
* tests/data/test-fedabipkgdiff/test1-from-fc20-to-dbus-glib-0.106-1.fc23.x86_64-report-0.txt:
Likewise.
* tests/data/test-fedabipkgdiff/test2-dbus-glib-0.100.2-2.fc20--dbus-glib-0.106-1.fc23-report-0.txt:
Likewise.
* tests/data/test-fedabipkgdiff/test3-dbus-glib-0.100.2-2.fc20.i686--dbus-glib-0.106-1.fc23.i686-report-0.txt:
Likewise.
* tests/data/test-fedabipkgdiff/test4-glib-0.100.2-2.fc20.x86_64.rpm-glib-0.106-1.fc23.x86_64.rpm-report-0.txt:
Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Under "make check-valgrindk", when valgrind returns errors, these
errors are ignored by make. It turns out it is the autoconf
VALGRIND_CHECK_RULES macro that does this. Fixed thus.
* autoconf-archive/ax_valgrind_check.m4 (check-valgrind): Don't
ignore errors.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* tests/Makefile.am (check-valgrind-helgrind-recursive): New
target to run the tests recursively under the control of
Valgrind's Helgrind tool.
* tests/test-valgrind-suppressions.supp: Update this suppression
file with suppressions for Helgrind.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
While making abipkgdiff to use the abigail::workers API to do away
with using pthreads directory, it appeared that the abigail::workers
API needs fixes and enhancements.
Fixes
=====
* Don't try to schedule a task if the pointer to the task is nil
* Fix a data race when bringing workers (of a queue) down
* Always try to wake up all waiting threads when bringing down queue
workers.
* Fix a data race when accessing the queue condition variable
* Fix a data race when notifying listeners about the end of the job
performed by the task.
Enhancements
============
* Pass the "task done" notifier by reference, to the worker queue.
Without this, the worker queue needs to copy the "task done" notifier
by value. This implies that user code needs to provide task done
notifier instances that come with potentially complicated copy
constructors. By passing it by reference and by just re-using the
notifier from the user code, we do away with the need for copying
altogether. This also fixes some latent copying bugs.
* Add a workers::queue::schedule_tasks() method
This allows user code to schedule a vector of tasks at once.
* make workers::queue::get_completed_tasks() return a non-const vector
This enables user code to sort the completed tasks as they wish.
* include/abg-workers.h (queue::tasks_type): New typedef.
(queue::queue): Pass task_done_notify by reference.
(queue::schedule_tasks): Declare new member function.
(queue::get_completed_tasks): Return non-const vector.
* src/abg-workers.cc (queue::priv::default_notify): New data
member.
(queue::priv::notify): Make this data member be a reference.
(queue::priv::priv): Initialize the notify data member to either
the new default_notify (if no notifier is provided by the
constructor) or to the notifier provided by the constructor.
(queue::priv::schedule_task): Do not schedule a nil task. Update
comment.
(queue::priv::schedule_tasks): Add a new member function.
(queue::priv::do_bring_workers_down): Update comment. Protect
access to "bring_workers_down" with tasks_todo_mutex to prevent a
data race. Call pthread_cond_broadcast on the queue_cond
unconditionaly to prevent some worker threads to keep waiting for
ever. Also, protect the access to the queue_cond by the
queue_cond_mutex to precent a data race.
(queue::queue): Pass the notifier by reference. Update comment.
(queue::schedule_task): Update comment.
(queue::schedule_tasks): Define new member function.
(queue::wait_for_workers_to_complete): Update comment.
(queue::get_completed_tasks): Return a non-const vector. Update
comment.
(worker::wait_to_execute_a_task): Update several comments. Make
the execution of the notification code to be synchronized (on the
tasks_done_mutex).
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
I tried to run:
fedabipkgdiff vte291-0.39.1-1.fc22.x86_64 vte291-0.39.90-1.fc22.x86_64
And it wouldn't work :-(
The program considers the two packages as not being "peers". This has
to do with the RPM.is_peer method which considers the two package as
not being "peers", just because they have the same release number
(1.fc22).
They should be considered peers, though, because they have the same
name but different {version, release} pairs.
This patch fixes the RPM.is_peer method and adds the aforementioned
packages to the regression test suite for good measure.
* tools/fedabipkgdiff (RPM.is_peer): Update comment. Fix logic.
* tests/data/test-fedabipkgdiff/packages/vte291/0.39.1/1.fc22/x86_64/vte291-0.39.1-1.fc22.x86_64.rpm:
New test input file.
* tests/data/test-fedabipkgdiff/packages/vte291/0.39.1/1.fc22/x86_64/vte291-debuginfo-0.39.1-1.fc22.x86_64.rpm: Likewise.
* tests/data/test-fedabipkgdiff/packages/vte291/0.39.1/1.fc22/x86_64/vte291-devel-0.39.1-1.fc22.x86_64.rpm: Likewise.
* tests/data/test-fedabipkgdiff/packages/vte291/0.39.90/1.fc22/x86_64/vte291-0.39.90-1.fc22.x86_64.rpm: Likewise.
* tests/data/test-fedabipkgdiff/packages/vte291/0.39.90/1.fc22/x86_64/vte291-debuginfo-0.39.90-1.fc22.x86_64.rpm: Likewise.
* tests/data/test-fedabipkgdiff/packages/vte291/0.39.90/1.fc22/x86_64/vte291-devel-0.39.90-1.fc22.x86_64.rpm:
Likewise.
* tests/data/test-fedabipkgdiff/vte291-0.39.1-1.fc22.x86_64--vte291-0.39.90-1.fc22.x86_64-report-0.txt: Likewise.
* tests/data/Makefile.am: Add the new test input data to source
distribution.
* tests/mockfedabipkgdiff.in: Update the package and build
information to add the new vte291-0.39.1-1.fc22.x86_64.rpm and
vte291-0.39.90-1.fc22.x86_64.rpm packages (as well as their devel
and debuginfo packages) into the "mock" Koji build database.
* tests/runtestfedabipkgdiff.py.in: Make this test harness run
over the two aforementioned packages.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The runtestfedabipkgdiff.py test was missing some input files from the
source distribution.
This patch adds them back.
Also, the test file name
test-fedabipkgdiff/test6-missing-devel-debuginfo-nss-util-3.12.6-1.fc14.x86_64--nss-util-3.24.0-2.0.fc25.x86_64-report-0.txt
was too long for tar to handle so that file was left out of the final
source distribution tarball. This patch renames that file to a
smaller name.
make distcheck should pass again now.
* tests/data/Makefile.am: Add three missing test input files to
the source distribution tarball. Renamed
test-fedabipkgdiff/test6-missing-devel-debuginfo-nss-util-3.12.6-1.fc14.x86_64--nss-util-3.24.0-2.0.fc25.x86_64-report-0.txt
into
test-fedabipkgdiff/test6-nss-util-3.12.6-1.fc14.x86_64--nss-util-3.24.0-2.0.fc25.x86_64-report-0.txt.
* tests/runtestfedabipkgdiff.py.in (FEDABIPKGDIFF_TEST_SPECS):
Renamed
test6-missing-devel-debuginfo-nss-util-3.12.6-1.fc14.x86_64--nss-util-3.24.0-2.0.fc25.x86_64-report-0.txt
into
test6-nss-util-3.12.6-1.fc14.x86_64--nss-util-3.24.0-2.0.fc25.x86_64-report-0.txt.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This patch is a quick one liner to add a missing new line to an error
message from the runtestfedabipkgdiff.py test harness.
* tests/runtestfedabipkgdiff.py.in (run_fedabipkgdiff_tests): Add
missing new line to an error message.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
It turns out the test <builddir>/tests/runtestfedabipkgdiff.py is
failing, but "make check TESTS=runtestfedabipkgdiff.py" is not.
In other words, runtestfedabipkgdiff.py is failing silently; we don't
see it when doing "make check".
The reason why runtestfedabipkgdiff.py is failing is that
mockfedabipkgdiff is trying to patch the global variable
DEFAULT_KOJI_TOPDIR from the fedabipkgdiff file; and that global
variable is not present in that file. The right global variable that
we want is DEFAULT_KOJI_TOPURL.
This patch fixes that issue.
The reason why the failing above is silent is because the the main
function wasn't returning 0 upon success and 1 otherwise.
This patch fixes that issue as well.
So with this patch, <builddir>/tests/runtestfedabipkgdiff does not
fail anymore and when it does, the "make check TESTS=runtestfedabipkgdiff.py"
fails as well.
* tests/mockfedabipkgdiff.in (run_fedabipkgdiff): Patch
fedabipkgdiff.DEFAULT_KOJI_TOPURL instead of
fedabipkgdiff.DEFAULT_KOJI_TOPDIR.
* tests/runtestfedabipkgdiff.py.in (main): Properly return 0 upon
success, 1 otherwise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
There are cases where a virtual member function doesn't have an
implementation that is defined and publicly exported. This is the
cases for virtual pure classes.
Even in those cases, users might want to detect vtable changes on
virtual member pure classes (interfaces).
Now that, for C++ binaries, all the partial representations of a class
are merged into one class (in a given binary), we can envision to
compare virtual member functions of classes as part of comparing two
classes.
This is what this patch does. While comparing two classes, the
virtual member functions are compared too.
Note that as there can be several virtual member functions that have
the same vtable offset (think about a virtual destructor that might
have several generated functions that 'conceptually' share the same
vtable offset), comparing the virtual functions can be a little bit
non-trivial.
The approach taken is to check that in the first set of virtual
functions, each virtual function actually matches at least one virtual
function in the second set. And the match is a "loose" one. That is,
it doesn't take into account the symbol name or the mangled name.
The patch also fixes the "less than" operator used to sort virtual
member functions.
There is also a buglet in the way we compute the highest vtable offset
number today. This patch provides a new function named
class_decl::get_biggest_vtable_offset that is used in the change reports.
The patch also fixes a related bug in where we were forgetting to
report about added and removed virtual member functions without an
associated elf symbol. This patch fixes that.
* include/abg-ir.h (class_decl::get_biggest_vtable_offset):
Declare new member function.
* src/abg-ir.cc (virtual_member_function_less_than::operator()):
Either compare the symbol id strings if the functions have
symbols or just compare their pretty representations.
(class_decl::get_biggest_vtable_offset): Define new member
function.
(methods_equal_modulo_elf_symbol)
(method_matches_at_least_one_in_vector): New static methods.
(equals): In the overload for classes, compare the virtual member
functions while comparing classes.
* src/abg-comparison.cc (represent): In the overload for
method_decl_sptr, fix the way we compute the highest vtable offset
number for a give class_decl.
(class_decl::ensure_lookup_tables_populated): Don't forget added
and removed virtual member functions in the report for changed
classes.
* tests/data/test-diff-dwarf/libtest41-PR20476-hidden-old.so: New
test binary input file.
* tests/data/test-diff-dwarf/libtest41-PR20476-hidden-new.so: Likewise.
* tests/data/test-diff-dwarf/test41-PR20476-hidden-report-0.txt:
New reference output.
* tests/data/Makefile.am: Add the new test material above to the
source distribution.
* tests/test-diff-dwarf.cc (in_out_spec): Add the new tests
here.
* tests/data/test-annotate/test10-pr18818-gcc.so.abi: Adjust.
* tests/data/test-annotate/test11-pr18828.so.abi: Adjust.
* tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi: Adjust.
* tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi: Adjust.
* tests/data/test-annotate/test22-pr19097-libstdc++.so.6.0.17.so.abi: Adjust.
* tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Adjust.
* tests/data/test-read-dwarf/test11-pr18828.so.abi: Adjust.
* tests/data/test-read-dwarf/test13-pr18894.so.abi: Adjust.
* tests/data/test-read-dwarf/test14-pr18893.so.abi: Adjust.
* tests/data/test-read-dwarf/test15-pr18892.so.abi: Adjust.
* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Adjust.
* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Adjust.
* tests/data/test-read-dwarf/test21-pr19092.so.abi: Adjust.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Adjust.
* tools/abidw.cc (parse_command_line): Don't require an empty
output file when parsing the --headers-dir option. This was a
thinko.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
The runtestdiffpkg test has wrong paths to the spice-server packages.
Fixed thus.
* tests/test-diff-pkg.cc (in_out_specs): Fix paths to spice-server
packages.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This option that is present for the abidiff tool was missing for
abipkgdiff.
* doc/manuals/abidiff.rst: Fix a typo.
* doc/manuals/abipkgdiff.rst: Document the --harmless option.
* tools/abipkgdiff.cc: Update copyright year.
(options::show_harmless_changes): Add new data member.
(options::options): Initialize the new data member.
(display_usage): Add a help string for the new --harmless option.
(parse_command_line): Parse the new --harmless option.
(set_diff_context_from_opts): Configure the diff context
accordingly, if the user provided the --harmless option.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
