test-read-dwarf silently succeeded even if the input elf file was not
existing. Hence, make distcheck succeeded even though the testfiles were
not distributed. Assert on the existence of the input file and add the
missing test case files.
* tests/data/Makefile.am: add missing test case files
* tests/test-read-dwarf.cc (test_task::perform): assert the
input elf file exists.
Signed-off-by: Matthias Maennich <maennich@google.com>
Similarly to asan, tsan and ubsan, add support for msan conditionally at
configure time. This allows us to track down issues caused by using
uninitialized values.
* configure.ac: Add configure options for -fsanitize=memory
Signed-off-by: Matthias Maennich <maennich@google.com>
Update the help string of the --impacted-interface option to make it
match the one from the abidiff tool.
* tools/abipkgdiff.cc (display_usage): Use the same help string
for the --impacted-interface option as abidiff does.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
abidiff --impacted-interfaces is supposed to show the impacted
interfaces. Hence fix the documentation to reflect that.
* tools/abidiff.cc(display_usage): Fix doc string for
--impacted-interfaces.
Reported-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
These tags were previously only emitted by the default reporter if the
there were more than 100 (hard-coded constant) items in a a list. The
leaf reporter emitted them unconditionally. This change simplifies the
code, makes output more consistent and makes it easier to interpret
diffs of diff output.
Additionally, in the reporting of changed unreachable types, the
indentation and quoting for the deleted and added cases was missing.
This patch corrects these issues.
Finally, when doing package differences, there were no tags for
deleted/added binaries. This patch adds them.
* src/abg-default-reporter.cc (report): In the corpus_diff
override, remove calculations of number of changes (total) and
comparisons against arbitrary threshold (large_num); emit [A],
[D], [C] tags unconditionally.
* src/abg-reporter-priv.cc
(maybe_report_unreachable_type_changes): Remove comparisons of
number of changes against arbitrary threshold (large_num);
emit [A], [D], [C] tags unconditionally; fix quoting of
deleted unreachable types; fix indentation of changed
unreachable types.
* tools/abipkgdiff.cc (compare_prepared_userspace_packages):
Emit [D] and [A] tags for removed and added binaries.
* tests/data/test-*/*report*.txt: In 109 report files, add
tags [A], [D], [C] tags and correct some indentation and
quoting.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Since bring_workers_down is not atomic, a worker thread can make a racy
read on it while do_bring_workers_down() writes it. That can lead to a
deadlock between the worker thread waiting for more work and
do_bring_workers_down() waiting for the worker to finish.
Address this by guarding all access to bring_workers_down with locking
tasks_todo_mutex. This is likely to be dropped after migrating to newer
C++ standards supporting std::atomic.
* src/abg-workers.cc(do_bring_workers_down): keep
task_todo_mutex locked while writing bring_workers_down,
(wait_to_execute_a_task): rewrite the loop condition to ensure
safe access to bring_workers_down.
Signed-off-by: Matthias Maennich <maennich@google.com>
Similarly to asan and ubsan, add support for tsan conditionally at
configure time. This allows us to track down race conditions etc.
* configure.ac: Add configure options for -fsanitize=thread
Signed-off-by: Matthias Maennich <maennich@google.com>
For valid values of h1/h2 and c, the signed integer left shift
expression (1 << (h1 % c)) might overflow, exposing undefined behaviour.
Fix that by using a data type that can hold the value.
That issue had been reported by ASAN when running test-lookup-syms:
src/abg-dwarf-reader.cc:2028:50: runtime error:
shift exponent 53 is too large for 32-bit type 'int'
* src/abg-dwarf-reader.cc(lookup_symbol_from_gnu_hash_tab): Fix
signed integer overflow.
Signed-off-by: Matthias Maennich <maennich@google.com>
In leaf-changes-only mode, if the type of a struct's function pointer
member changes it currently gets categorised as a non-local change and
so is not reported. The change to any function passing such a struct
is considered non-local and also not reported.
This patch broadens the definition of local changes to include these
cases and so have them be reported in leaf-changes-only mode. It may
be the first of a sequence of such patches,
* src/abg-ir.cc (types_have_similar_structure): Always compare
function types (instead of just returning true) regardless of
whether they are components of pointer-to-function or
reference-to-function types.
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-leaf2-report.txt: New test
case.
* tests/data/test-abidiff-exit/test-leaf2-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf2-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf2-v1.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf2-v1.o: Ditto.
* tests/test-abidiff-exit.cc: Run new test case.
Signed-off-by: Giuliano Procida <gprocida@google.com>
abidiff emits hierarchical difference information using 2-space
indentation, almost everywhere.
In a few places, long lines are split up and 1-space is used for
clarity. Otherwise 1-space indentation appears to be only used when
reporting:
- data member changes (not additions or removals)
- the change of the type of a variable
This patch resolves these inconsistencies in favour of 2-space
indentation.
* src/abg-default-reporter.cc (report): In the
class_or_union_diff override, use 2-space indentation when
listing changed members. In the var_diff override, do the same
for variable type changes.
* src/abg-leaf-reporter.cc: Ditto.
* tests/data/test-*/*report*.txt: Update many test cases.
Signed-off-by: Giuliano Procida <gprocida@google.com>
v2: More code simplification. Tests unchanged.
There is distributed responsibility for horizontal and vertical
whitespace in the reporting code with indent and new line control
information being manipulated by and passed in and out of functions.
Occasionally, this information is ignored or incorrect and the code
tends to err on the side of more rather than fewer new lines.
The outcome is that abidiff output sometimes contains extra blank
lines which can be confusing.
This patch eliminates some of the more obvious cases:
- after data member deletions
- in enumerator change lists
- after "type size hasn't changed"
- before listing impacted interfaces
A lot of passing of "new line needed" booleans between functions has
been eliminated in the process.
The patch cleans up the reporting of data members. The code will
either emit indentation, a short description and a new line or do
nothing at all.
The patch also removes some stray location reporting code for array
diffs which would have produced some oddly placed output. I could not
get this code to trigger as loc = decl->get_location() was never
present on the array decls in question.
* src/abg-default-reporter.cc (report): In the type_decl_diff,
enum_diff, array_diff, class_diff, union_diff and var_diff
overrides, simplify new line logic which no longer needs to be
threaded through report_name_size_and_alignment_changes. In
the distinct_diff override, simplify new line logic which no
longer needs to be threaded through
report_size_and_alignment_changes. In the enum_diff override,
emit just one blank line after each enum. In the array_diff
override, remove stray location reporting which doesn't appear
to ever trigger; fix new line logic. In the
class_or_union_diff override, simplify new line logic for
deleted members; pass indentation to represent_data_member.
* src/abg-leaf-reporter.cc (report): In the array_diff,
class_diff, union_diff and var_diff overrides, simplify new
line logic which no longer needs to be threaded through
report_name_size_and_alignment_changes. In the distinct_diff
override, simplify new line logic which no longer needs to be
threaded through report_size_and_alignment_changes. In the
array_diff override, remove stray location reporting which
doesn't appear to ever trigger; fix new line handling. In the
class_or_union_diff override, simplify new line logic for
deleted members; pass indentation to represent_data_member.
In the corpus_diff override, tabify source indentation.
* src/abg-reporter-priv.cc (represent_data_member): Handle
indentation; fix new line logic.
(report_size_and_alignment_changes): Fix new line logic
for "type size hasn't changed" message; simplify new line
logic and replace local bool n with argument bool nl for
clarity.
(report_size_and_alignment_changes): Remove bool nl argument
and associated code as it had become always false; take
responsibility for emitting terminating new lines and change
return type to void.
(report_name_size_and_alignment_changes): Fix new line logic;
remove bool nl argument and associated code as it had become
always false; take responsibility for emitting terminating new
lines and change return type to void.
(maybe_report_interfaces_impacted_by_diff) In both overrides,
remove new line prefix code and new_line_prefix argument.
* src/abg-reporter-priv.h (represent_data_member): Add indent
argument.
(report_size_and_alignment_changes) Remove bool nl argument;
change return type to void.
(report_name_size_and_alignment_changes) Remove bool nl
argument; change return type to void.
(maybe_report_interfaces_impacted_by_diff) In both overrides,
remove new_line_prefix argument.
* tests/data/test-*/*report*.txt: Remove some blank lines.
Signed-off-by: Giuliano Procida <gprocida@google.com>
When reporting the details of changes to function parameter
differences in leaf-changes-only mode, the details are output at the
same level of indentation as the introductory text. In default mode
the usual 2-space indentation is used.
This patch fixes this discrepancy, making the output more readable.
* src/abg-leaf-reporter.cc (report): In the fn_parm_diff
override, indent the lines of detail by 2 spaces.
* tests/data/test-abidiff-exit/test-leaf3-report.txt: Update
report with correct indentation.
Signed-off-by: Giuliano Procida <gprocida@google.com>
The --redundant (meaning show-redundant-changes) option is supposed to
be implied by --leaf-changes-only and this is currently implemented by
making diff_context's --leaf-changes-only setter also duplicate the
behaviour of its --redundant setter.
In both abidiff and abipkgdiff, the diff_context setters are called
unconditionally, but the relative order of the calls for these two
options is different in each case, resulting in two different issues.
In abidiff, the --redundant setter is called second, undoing the
intended side-effect of any --leaf-changes-only flag. So --redundant
is not actually turned on in --leaf-changes-only mode unless requested
explicitly.
In abipkgdiff, the leaf-changes-only setter is called second, undoing
(in non-leaf mode) the effect of any --redundant flag. So --redundant
has no effect in default reporting mode.
The fix is move to move the "--leaf-changes-only implies --redundant"
logic from the setter to the set_diff_context_from_opts functions.
This patch also documents the implied behaviour in the usage strings.
* src/abg-comparison.cc (diff_context::show_leaf_changes_only):
Remove "--leaf-changes-only implies --redundant" logic.
* tools/abidiff.cc (display_usage): Mention that
--leaf-changes-only implies --redundant.
(set_diff_context_from_opts): Make --leaf-changes-only imply
--redundant; document this behaviour in a comment.
* tools/abipkgdiff.cc: Ditto.
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-leaf3-report.txt: Add new
test case, to show --leaf-changes-only implies --redundant.
* tests/data/test-abidiff-exit/test-leaf3-v0.c: Ditto.
* tests/data/test-abidiff-exit/test-leaf3-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf3-v1.c: Ditto.
* tests/data/test-abidiff-exit/test-leaf3-v1.o: Ditto.
* tests/test-abidiff-exit.cc: Run new test case.
* tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt:
Update abipkgdiff report with --redundant output.
* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-1.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>
The leaf-changes-only reporting path does not report on all the same
kinds of differences as the default reporting path does, such as
reporting about changes to variables, even though they can be
considered leaf changs.
- The addition or removal of any symbol affects the ABI and is
clearly a leaf change.
- A change to a variable's declaration may be local rather than
caused by a type change elsewhere.
This patch adds these missing pieces and reorders some of the existing
leaf reporting, bringing the default and leaf corpus_diff functions
closer to the point where they can be trivially merged or refactored.
This patch also corrects an error in reporting the total number of
leaf changes.
* doc/manuals/abidiff.rst: Update the documentation for
--leaf-changes-only.
* doc/manuals/abipkgdiff.rst: Likewise.
* src/abg-comparison.cc (emit_diff_stats): Exclude non-leaf
changes to variables from the reported total of leaf changes.
* src/abg-default-reporter.cc (report): In the corpus_diff
override, move some code and comments for clarity.
* src/abg-leaf-reporter.cc (report): In the corpus_diff
override, additionally report removed/added/changed variables
and removed/added symbols absent from debug info.
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-leaf0-report.txt: Update
to include reporting of variable diff (change of type).
* tests/data/test-abidiff-exit/test-leaf1-report.txt: New test
case with added/removed variables/functions and changed
variables (both local and non-local type changes).
* tests/data/test-abidiff-exit/test-leaf1-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf1-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf1-v1.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf1-v1.o: Ditto.
* tests/test-abidiff-exit.cc: Run new test case. Supply
--redundant otherwise the test isn't meaningful.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This patch is a follow-up to this commit:
b602f46c Fix spurious new lines after diff sections.
The intent is to update the tests that relate to the fedpkgdiff tool.
I didn't notice that those tests reference output were not updated by
commit b602f46c because I didn't have 'koji' installed on the system
I ran the regression test suite on; and without koji installed, the
fedpkgdiff test is automatically disabled.
The patch just mechanically adjusts the reference output that needed
it to comply with the newer better output of fedpkgdiff.
* tests/data/test-fedabipkgdiff/test0-from-fc20-to-fc23-dbus-glib-report-0.txt:
Adjust for useless whitespace removal.
* 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>
The top-level corpus diff routines in abidiff have varied ways of
tracking whether or not to emit a new line after each section. Reuse
of state variables (which aren't always cleared) between sections
means that spurious new lines are sometimes output.
This patch replaces this new line logic in the functions with the same
simple pattern of using a local boolean state variable.
* src/abg-default-reporter.cc (report): In the corpus_diff
overload, just use a local boolean emitted state variable
within each section to determine whether or not to follow the
section with an extra new line.
* src/abg-leaf-reporter.cc: Ditto.
* tests/data/test-*/*report*.txt: Remove unwanted new lines
from 27 files.
Signed-off-by: Giuliano Procida <gprocida@google.com>
There was a stray file-scoped declaration of show_relative_offset_changes. This
function is now a member of diff_context.
* src/abg-comparison.cc (show_relative_offset_changes): Remove
this stray function declaration.
Signed-off-by: Giuliano Procida <gprocida@google.com>
When abisym reports a symbol as found, it currently emits a leading
space. It does not do this when reporting a symbol as not found.
This patch removes the leading space.
* tools/abisym.cc (main): Remove leading space from output.
* tests/data/test-lookup-syms/test0-report.txt: Remove leading
space from expected output.
* tests/data/test-lookup-syms/test01-report.txt: Ditto.
* tests/data/test-lookup-syms/test02-report.txt: Ditto.
* tests/data/test-lookup-syms/test1-1-report.txt: Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Leaf changes (as reported with --leaf-changes-only) to variables were
miscounted as changes to functions.
* src/abg-comparison.cc
(apply_filters_and_compute_diff_stats): Increment the correct
counter for leaf variable changes.
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-leaf0-report.txt: New test
case.
* tests/data/test-abidiff-exit/test-leaf0-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf0-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf0-v1.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf0-v1.o: Ditto.
* tests/test-abidiff-exit.cc: Run new test case.
Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
All such tags are now followed by a space and are more readable.
* src/abg-default-reporter.cc (report): In the overload for
corpus_diff, output space after "[C]".
* src/abg-leaf-reporter.cc (report): Likewise.
* tests/data/test-*/*report*.txt: Update all the test
reports.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
In the leaf reporter member subtype changes are labelled as plain
changes and vice versa. This was probably due to the different
ordering of the code sections in the default and leaf reporters.
Output is unchanged as these tags currently map to the same strings.
When generating diff reports there were some rare cases where a
pretty representation might have been emitted twice or the trailing
whitespace might have been missing.
* src/abg-leaf-reporter.cc (report): In the class_or_union_diff
overoad, swap calls to report_mem_header to match the rest of the
code.
* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
overload, add some missing whitespace; remember we've emitted the
pretty representation in 2 cases where this was omitted (though 1
of these is the last case where it makes no difference).
maybe_report_diff_for_symbol Add some missing whitespace; remember
we've reported a diff (and need a trailing newline) in 1 case
where this was omitted, also affecting the return value of the
function (but no caller cares).
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
* gen-changelog.py (process_commit): Use the functional notation
for the print function invocation required by python3.
(output_commits, get_rel_tags, ): Specify that the output stream
of the subprocess running the git command is in the text format.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When the BAD_FTS macro was defined at compile time (to handle the use
of fts.h on glibc < 2.23), we needed to re-map fopen to fopen64 if
_FILE_OFFSET_BITS=64 was defined. We do not need this anymore because
we don't use fopen in that module anymore. Furthermore, as we now use
fstream, doing the fopen to fopen64 remapping is now preventing the
fstream c++ headers to compile on el6 systems.
This patch just simply does away with the fopen to fopen64 remapping and
thus fixes the compilation on el6 systems.
* src/abg-tools-utils.cc: Do not remap fopen to fopen64 as we
don't use fopen explicitly anymore.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
When comparing binary files (using abidiff for instance) libabigail
can interpret the [suppress_file] section of a suppression
specification. If the suppression specification matches either of the
compared files, no comparison is performed.
At the moment, that doesn't work when comparing abixml files.
Thus, this patch implements that feature for abixml files.
With this patch, one can now write a suppression specification like
this:
[suppress_file]
soname_regexp = <some-regexp>
or
[suppress_file]
file_name_regexp = <some-regexp>
If either abixml file has a soname matched by such a regexp, then no
comparison is performed.
* doc/manuals/libabigail-concepts.rst: Update the documentation to
mention soname_regexp and soname_not_regexp is supported in the
[suppress_file] section.
* include/abg-suppression.h (suppression_matches_soname)
(suppression_matches_soname_or_filename): Declare new functions.
Make them be friends of class suppression_base.
* src/abg-reader.cc
(read_context::corpus_is_suppressed_by_soname_or_filename): Define
new member function.
(read_corpus_from_input): Apply file suppression.
* src/abg-suppression.cc (read_file_suppression): Support
"soname_regexp" and "soname_not_regexp" in the [suppress_file]
section.
(suppression_matches_soname)
(suppression_matches_soname_or_filename): Define new functions.
* tests/data/test-diff-suppr/libtest48-soname-abixml-report-{1,2}.txt:
New test reference output files.
Likewise.
* tests/data/test-diff-suppr/libtest48-soname-abixml-suppr.txt:
New test suppression file.
* tests/data/test-diff-suppr/libtest48-soname-abixml-suppr-{2,3,4}.txt::
Likewise.
* tests/data/test-diff-suppr/libtest48-soname-abixml-v{0,1}.so: New
test binary input files.
* tests/data/test-diff-suppr/libtest48-soname-abixml-v{0,1}.so.abi:
New abixml for the binary input files above.
* tests/data/test-diff-suppr/test48-soname-abixml-v{0,1}.c: Source
code of the binary input files above.
* tests/data/Makefile.am: Add the above test material to source
distribution.
* tests/test-diff-suppr.cc (in_out_specs): Add the test input
above to this test harness.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
At the moment, we don't make any difference between these two cases:
1/ A suppression specification has no soname or file name related
property.
2/ A suppression specification has a soname or file name related
property that doesn't match the soname or file name of a given
corpus.
In both cases 1/ and 2/ libabigail currently assumes that the
suppression specification does not match the given corpus we are
looking at. This can be wrong, especially if we are in the case 1/
and if the suppression does have other properties that should make it
match the corpus.
This patch fixes this issue.
* include/abg-suppression.h
(suppression_base::has_{soname,file_name}_related_property): Add
new member functions.
* src/abg-dwarf-reader.cc (read_context::suppression_can_match):
Fix the logic to make a difference between the case where the
suppression doesn't have any soname/filename property and the case
where the suppression does have a soname/filename property that
does not match the current binary.
* src/abg-reader.cc (read_context::suppression_can_match):
Likewise.
* src/abg-suppression-priv.h
(suppression_base::priv::matches_soname): If the suppression does
not have any soname related property then it doesn't match the
soname we are looking at.
(suppression_base::priv::matches_binary_name): If the suppression
does not have any filename related property then it doesn't match
the filename we are looking at.
* src/abg-suppression.cc
(suppression_base::has_{soname,file_name}_related_property):
Define new member functions.
(sonames_of_binaries_match): If the suppression does not have any
soname related property then it doesn't match the corpora of the
diff we are looking at.
(names_of_binaries_match): If the suppression does not have any
filename related property then it doesn't match the corpora of the
diff we are looking at.
(type_suppression::suppresses_type): Fix the logic to make a
difference between the case where the suppression doesn't have any
soname/filename property and the case where the suppression does
have a soname/filename property that does not match the current
binary.
(function_suppression::suppresses_{function, function_symbol}):
Likewise.
(variable_suppression::suppresses_{variable, variable_symbol}):
Likewise.
(file_suppression::suppresses_file): Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This change replaces the "\n," sometimes seen in output (such as for
the PR25128 test cases) with a correctly indented "and".
* src/abg-reporter-priv.cc (represent): Don't try to follow
output of indented pretty representation with a comma, just
emit "and" unconditionally; remove unnecessary intermediate
ostringstream.
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-no-stray-comma-*: New test
cases.
* tests/data/test-diff-suppr/test46-PR25128-report-?.txt:
Replace unindented comma with indented "and".
* tests/test-abidiff-exit.cc: Add no-stray-comma test case.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
This imposes a deterministic ordering, making diffs more predictable
and allowing reproducible testing.
* src/abg-tools-utils.cc (get_binary_paths_from_kernel_dist):
Sort module_paths.
Signed-off-by: Giuliano Procida <gprocida@google.com>
This patch refactors the abigail::workers::queue and
abigail::workers::worker implementations to avoid holding locking
primitives longer than necessary.
In particular, the queue_cond_mutex was held during the entiry worker
runtime, effectively serializing the workers. Hence, use a mutex+cond
pair for each, the input and output queue and only synchronize around
the interaction with their corresponding queues. The
tasks_todo_(mutex|cond) are meant to synchronize scheduling and
distribution of work among workers, while tasks_done_(mutex|cond) are
used for synchronizing threads when putting back the tasks to the output
queue and to hold back threads waiting for the queue and workers to
drain.
Along that way, I did some cleanup that was now possible.
- Move entire implementation of abigail::workers::task into header.
- Make default_notify a static member.
- Replace the multiple constructors with one with default arguments.
* include/abg-workers.h (workers::task): move entire
implementation to header and drop superfluous forward declaration.
* src/abg-workers.cc (workers::task):: Likewise.
(workers::queue::priv): Drop queue_cond_mutex, rename queue_cond
to tasks_todo_cond, add task_done_cond, make default_notify
static.
(workers::queue::priv::priv): Add default arguments to fully
qualified constructor, drop the remaining ones.
(workers::queue:prive::more_tasks_to_execute): Drop method.
(workers::queue:prive::schedule_task): Do not synchronize access
to the queue condition variable, but only on the mutex.
(do_bring_workers_down): Likewise. Also await tasks_done to be
empty.
(workers::queue:prive::worker::wait_to_execute_a_task): Await
tasks on the tasks_todo with tasks_todo_(cond|mutex) and signal
task completion to tasks_done_cond.
Signed-off-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Not initializing those might lead to undefined behaviour. E.g. if the
call to 'dwarf_ranges' does not initialize 'addr', we pass that
uninitialized value to 'maybe_adjust_fn_sym_address' and test it for
zero as first action, depending on the random value. Hence, fix that by
initializing the values.
* src/abg-dwarf-reader.cc
(read_context::get_first_exported_fn_address_from_DW_AT_ranges):
initialize local Dwarf_Addr variables.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
size() is not guaranteed to be a constant-time function. Also, using
.empty() shows clearer intent. Hence switch to using .empty().
That issue was flagged by clang-tidy[1].
* src/abg-comparison.cc (corpus_diff::has_changes): prefer
!container.empty() over bool(container.size())
[1] https://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
Clang-tidy flags those name inconsistencies and they are trivial to fix,
hence just do it. No functional change.
* src/abg-comparison-priv.h
(corpus_diff::priv::count_unreachable_types): use consistent
parameter naming.
* tools/abidiff.cc(main): Likewise.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
There was an inconsistency in the way the diff context was used for
different file types. This change eliminates this and so .bi files now
have all the command line options applied to their diffs.
* tests/data/Makefile.am: Add test case files.
* tests/data/test-abidiff-exit/test-loc-*: New test cases.
* tests/test-abidiff-exit.cc (in_out_specs): Add new test cases.
* tools/abidiff.cc (main): Use populated ctxt for translation unit
diff.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This patch adds the Catch [1] unit test framework in version v1.12.2 [2]
along with its integration into the existing build and test definition.
While there is version v2 available, v1 still supports C++98, hence we
can make use of it. The framework is distributed as a single header
file. And since it is less then 500k and it comes with a permissive
license, I decided to directly add the file rather than requiring
users/developers/distributors to satisfy the new dependency.
The integration is fairly simple: A new library libcatch.a provides the
`main` for the tests that run with Catch. The tests themselves require
to include the header as well and to link against said library.
As an example I migrated the test-kmi-whitelist test to Catch. The test
becomes a bit more structured and error reporting significantly
improved. E.g. see this intentional breakage:
| --- a/tests/test-kmi-whitelist.cc
| +++ b/tests/test-kmi-whitelist.cc
| @@ -140,5 +140,5 @@ TEST_CASE("WhitelistWithTwoSections", "[whitelists]")
| suppressions_type suppr
| = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
| REQUIRE(!suppr.empty());
| - test_suppressions_are_consistent(suppr, "^test_symbol1$|^test_symbol2$");
| + test_suppressions_are_consistent(suppr, "^test_symbol$|^test_symbol2$");
It leads to this test output:
| ---------------------------------------------------------------------------
| WhitelistWithTwoSections
| ---------------------------------------------------------------------------
| ../../tests/test-kmi-whitelist.cc:136
| ...........................................................................
|
| ../../tests/test-kmi-whitelist.cc:81: FAILED:
| REQUIRE( left->get_symbol_name_not_regex_str() == expr )
| with expansion:
| "^test_symbol1$|^test_symbol2$"
| ==
| "^test_symbol$|^test_symbol2$"
|
| ===========================================================================
| test cases: 6 | 5 passed | 1 failed
| assertions: 41 | 40 passed | 1 failed
[1] https://github.com/catchorg/Catch2
[2] https://github.com/catchorg/Catch2/releases/tag/v1.12.2
* tests/.gitignore: Add entry for .dirstamp
* tests/Makefile.am: Add libcatch test library and use it for
runtestkmiwhitelist.
* tests/lib/catch.cc: New test driver implementation.
* tests/lib/catch.hpp: Add Catch v1.12.2 header only test library.
* tests/test-kmi-whitelist.cc: Migrate to use Catch test framework.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
This declaration has already been done some lines above. Drop the
duplicate.
* include/abg-fwd.h: drop superfluous forward declaration.
Signed-off-by: Matthias Maennich <maennich@google.com>
Fix the include guards of abg-dwarf-reader.h and abg-reporter-priv.h by
moving them before any other includes where they actually belongs.
Add missing include guards for abg-libxml-utils.h and abg-libzip-utils.h.
* include/abg-dwarf-reader.h: Move include guard to the beginning.
* include/abg-reporter-priv.h: Likewise.
* include/abg-libxml-utils.h: Add include guard.
* include/abg-libzip-utils.h: Likewise.
Signed-off-by: Matthias Maennich <maennich@google.com>
A broken elf file might not have a valid symtab. As of now we would hit
an ABG_ASSERT and crash. Let's catch that case and bail out instead.
* src/abg-dwarf-reader.cc (load_symbol_maps_from_symtab_section):
Handle elf file with missing symtab.
* tests/test-read-dwarf.cc (InOutSpec): add test case.
* tests/data/test-read-dwarf/test26-bogus-binary.elf: new test data.
Signed-off-by: Matthias Maennich <maennich@google.com>
A broken elf file with a sh_entsize of 0 makes the dwarf reader crash
due to a division by zero. Fix this by validating the input and exiting
early in that case.
* src/abg-dwarf-reader.cc (load_symbol_maps_from_symtab_section):
Handle elf file with invalid sh_entsize.
* tests/test-read-dwarf.cc (test_task::perform): handle empty
in_abi_path and out_abi_path as 'read only' test.
(InOutSpec): add test case.
* tests/data/test-read-dwarf/test25-bogus-binary.elf: new test data.
Signed-off-by: Matthias Maennich <maennich@google.com>
The project style requires assignment operators to be on the first line
of two if the line needs to break. Reflect that in the .clang-format
configuration to approximate the style better when using clang-format.
* .clang-format: Add BreakBeforeBinaryOperators option.
Signed-off-by: Matthias Maennich <maennich@google.com>
The previous commit introduces a new (tested) way of creating function
and variable suppressions from multiple whitelist definitions. Migrate
to this new way of processing KMI whitelists.
* include/abg-tools-utils.h
(gen_suppr_spec_from_kernel_abi_whitelist): Delete declaration.
* src/abg-tools-utils.cc
(gen_suppr_spec_from_kernel_abi_whitelist): Delete definition
and migrate users to gen_suppr_spec_from_kernel_abi_whitelists.
* tools/abidiff.cc (set_suppressions): Migrate from using
gen_suppr_spec_from_kernel_abi_whitelist to
gen_suppr_spec_from_kernel_abi_whitelists.
* tools/abidw.cc (set_suppressions): Likewise.
* tools/abipkgdiff.cc: Drop unused using definition.
* tools/kmidiff.cc: Likewise.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
If multiple KMI whitelists are specified, either by passing
--kmi-whitelist several times or by having multiple whitelist sections
in the whitelist files, the generated suppressions are created as an
intersection of symbols. That is rather unusual, as whitelisting should
rather work additive. That means that the symbols (or expressions
thereof) defined across several sections or files shall be considered a
union of symbols. This patch combines the whitelist parsing to create
exactly one function_suppression and one variable suppression. A test
case has been added to ensure the functionality is working.
Please note, migrating the existing code to this new functionality is
done in a separate commit.
* include/abg-tools-utils.h
(gen_suppr_spec_from_kernel_abi_whitelists): New function.
* src/abg-tools-utils.cc
(gen_suppr_spec_from_kernel_abi_whitelists): Likewise.
* tests/.gitignore: Ignore new test executable.
* tests/Makefile.am: Add new test executable.
* tests/data/test-kmi-whitelist/whitelist-with-another-single-entry:
New test input file.
* tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry:
Likewise.
* tests/data/test-kmi-whitelist/whitelist-with-single-entry:
Likewise.
* tests/data/test-kmi-whitelist/whitelist-with-two-sections:
Likewise.
* tests/data/Makefile.am: Add above test material.
* tests/test-kmi-whitelist.cc: Add new test executable.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
A corpus that has no symbols contributing to the ABI surface (e.g.
because of an exhaustive suppression), will not contribute in a later
comparison via abidiff and friends. Hence, there is no need for such
entries to appear in the ABI xml representation. This patch completely
suppresses empty corpora.
* src/abg-writer.cc (write_corpus): completely skip empty
corpora rather than creating an empty entry for them.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
A corpus with completely filtered out symbols (exhaustive whitelist),
still contains compilation units, but they are empty. A list of empty
translation units shall be considered empty as no entries need to be
considered. That is useful to skip empty corpora when writing out the
xml for them.
Hence, teach is_empty() to have a look at the actual translation units.
* src/abg-corpus.cc (corpus::is_empty): consider a list of
empty members to be empty.
Reviewed-by: Dodji Seketeli <dodji@seketeli.org>
Signed-off-by: Matthias Maennich <maennich@google.com>
