* [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections.
@ 2020-03-11 10:35 Giuliano Procida
2020-03-11 10:35 ` [PATCH 2/2] [abidiff] Add more leaf change reporting Giuliano Procida
2020-03-12 13:03 ` [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections Dodji Seketeli
0 siblings, 2 replies; 9+ messages in thread
From: Giuliano Procida @ 2020-03-11 10:35 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida
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>
---
src/abg-default-reporter.cc | 76 ++++++++++---------
src/abg-leaf-reporter.cc | 31 ++++----
.../test1-fn-removed-report-0.txt | 1 -
.../test3-fn-removed-report-0.txt | 1 -
.../test2-filtered-removed-fns-report0.txt | 1 -
.../test-abidiff/test-PR18791-report0.txt | 1 -
.../test31-vtable-changes-report-0.txt | 1 -
.../test32-fnptr-changes-report-0.txt | 1 -
.../test33-fnref-changes-report-0.txt | 1 -
.../test42-PR21296-clanggcc-report0.txt | 1 -
tests/data/test-diff-filter/test10-report.txt | 1 -
.../data/test-diff-filter/test41-report-0.txt | 1 -
...4--libcdio-0.94-2.fc26.x86_64-report.1.txt | 1 -
...-2.0-0v5_2.4.1-1ubuntu2_amd64-report-0.txt | 1 -
...l7.x86_64-0.12.8-1.el7.x86_64-report-0.txt | 1 -
...l7.x86_64-0.12.8-1.el7.x86_64-report-1.txt | 1 -
...l7.x86_64-0.12.8-1.el7.x86_64-report-2.txt | 1 -
...bb-4.3-3.20141204.fc23.x86_64-report-0.txt | 2 -
...bb-4.3-3.20141204.fc23.x86_64-report-1.txt | 2 -
.../data/test-diff-pkg/test-rpm-report-0.txt | 1 -
.../data/test-diff-pkg/test-rpm-report-5.txt | 1 -
.../test16-suppr-removed-fn-report-0.txt | 1 -
.../test16-suppr-removed-fn-report-3.txt | 1 -
.../test19-suppr-added-fn-sym-report-1.txt | 1 -
.../test19-suppr-added-fn-sym-report-2.txt | 1 -
.../test19-suppr-added-fn-sym-report-4.txt | 1 -
.../test21-suppr-added-var-sym-report-5.txt | 1 -
.../test22-suppr-removed-var-sym-report-2.txt | 1 -
.../data/test-diff-suppr/test30-report-0.txt | 1 -
29 files changed, 52 insertions(+), 84 deletions(-)
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index aaf083b3..2ac93d41 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -224,7 +224,7 @@ default_reporter::report_local_typedef_changes(const typedef_diff &d,
{
out << indent << "typedef name changed from "
<< f->get_qualified_name()
- << " to "
+ << " to "
<< s->get_qualified_name();
report_loc_info(s, *d.context(), out);
out << "\n";
@@ -788,8 +788,8 @@ default_reporter::report(const scope_diff& d, ostream& out,
out << indent << " '"
<< (*dif)->first_subject()->get_pretty_representation()
- << "' was changed to '"
- << (*dif)->second_subject()->get_pretty_representation() << "'";
+ << "' was changed to '"
+ << (*dif)->second_subject()->get_pretty_representation() << "'";
report_loc_info((*dif)->second_subject(), *d.context(), out);
out << ":\n";
@@ -1761,7 +1761,7 @@ void
default_reporter::report(const corpus_diff& d, ostream& out,
const string& indent) const
{
- size_t total = 0, removed = 0, added = 0;
+ size_t total = 0;
const corpus_diff::diff_stats &s =
const_cast<corpus_diff&>(d).
apply_filters_and_suppressions_before_reporting();
@@ -1797,6 +1797,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
else if (s.net_num_func_removed() > 1)
out << indent << s.net_num_func_removed() << " Removed functions:\n\n";
+ bool emitted = false;
vector<function_decl*>sorted_deleted_fns;
sort_string_function_ptr_map(d.priv_->deleted_fns_, sorted_deleted_fns);
for (vector<function_decl*>::const_iterator i =
@@ -1830,9 +1831,9 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< c->get_pretty_representation()
<< "\n";
}
- ++removed;
+ emitted = true;;
}
- if (removed)
+ if (emitted)
out << "\n";
}
@@ -1843,6 +1844,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
else if (s.net_num_func_added() > 1)
out << indent << s.net_num_func_added()
<< " Added functions:\n\n";
+ bool emitted = false;
vector<function_decl*> sorted_added_fns;
sort_string_function_ptr_map(d.priv_->added_fns_, sorted_added_fns);
for (vector<function_decl*>::const_iterator i = sorted_added_fns.begin();
@@ -1879,13 +1881,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< c->get_pretty_representation()
<< "\n";
}
- ++added;
- }
- if (added)
- {
- out << "\n";
- added = false;
+ emitted = true;
}
+ if (emitted)
+ out << "\n";
}
if (ctxt->show_changed_fns())
@@ -1963,14 +1962,11 @@ default_reporter::report(const corpus_diff& d, ostream& out,
}
diff->report(out, indent + " ");
out << "\n";
- emitted |= true;
+ emitted = true;
}
}
if (emitted)
- {
- out << "\n";
- emitted = false;
- }
+ out << "\n";
}
// Report added/removed/changed variables.
@@ -1985,6 +1981,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
out << indent << s.net_num_vars_removed()
<< " Removed variables:\n\n";
string n;
+ bool emitted = false;
vector<var_decl*> sorted_deleted_vars;
sort_string_var_ptr_map(d.priv_->deleted_vars_, sorted_deleted_vars);
for (vector<var_decl*>::const_iterator i =
@@ -2012,13 +2009,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
out << "}";
}
out << "\n";
- ++removed;
+ emitted = true;
}
- if (removed)
- {
+ if (emitted)
out << "\n";
- removed = 0;
- }
}
if (ctxt->show_added_vars())
@@ -2029,6 +2023,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
out << indent << s.net_num_vars_added()
<< " Added variables:\n\n";
string n;
+ bool emitted = false;
vector<var_decl*> sorted_added_vars;
sort_string_var_ptr_map(d.priv_->added_vars_, sorted_added_vars);
for (vector<var_decl*>::const_iterator i =
@@ -2054,13 +2049,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
out << "}";
}
out << "\n";
- ++added;
- }
- if (added)
- {
- out << "\n";
- added = 0;
+ emitted = true;
}
+ if (emitted)
+ out << "\n";
}
if (ctxt->show_changed_vars())
@@ -2074,6 +2066,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< " Changed variables:\n\n";
string n1, n2;
+ bool emitted = false;
for (var_diff_sptrs_type::const_iterator i =
d.priv_->sorted_changed_vars_.begin();
i != d.priv_->sorted_changed_vars_.end();
@@ -2097,8 +2090,9 @@ default_reporter::report(const corpus_diff& d, ostream& out,
out << ":\n";
diff->report(out, indent + " ");
out << "\n";
+ emitted = true;
}
- if (num_changed)
+ if (emitted)
out << "\n";
}
@@ -2114,6 +2108,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< s.net_num_removed_func_syms()
<< " Removed function symbols not referenced by debug info:\n\n";
+ bool emitted = false;
vector<elf_symbol_sptr> sorted_deleted_unrefed_fn_syms;
sort_string_elf_symbol_map(d.priv_->deleted_unrefed_fn_syms_,
sorted_deleted_unrefed_fn_syms);
@@ -2132,9 +2127,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
show_linkage_name_and_aliases(out, "", **i,
d.first_corpus()->get_fun_symbol_map());
out << "\n";
+ emitted = true;
}
- if (sorted_deleted_unrefed_fn_syms.size())
- out << '\n';
+ if (emitted)
+ out << "\n";
}
// Report added function symbols not referenced by any debug info.
@@ -2150,6 +2146,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< s.net_num_added_func_syms()
<< " Added function symbols not referenced by debug info:\n\n";
+ bool emitted = false;
vector<elf_symbol_sptr> sorted_added_unrefed_fn_syms;
sort_string_elf_symbol_map(d.priv_->added_unrefed_fn_syms_,
sorted_added_unrefed_fn_syms);
@@ -2168,9 +2165,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
**i,
d.second_corpus()->get_fun_symbol_map());
out << "\n";
+ emitted = true;
}
- if (sorted_added_unrefed_fn_syms.size())
- out << '\n';
+ if (emitted)
+ out << "\n";
}
// Report removed variable symbols not referenced by any debug info.
@@ -2185,6 +2183,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< s.net_num_removed_var_syms()
<< " Removed variable symbols not referenced by debug info:\n\n";
+ bool emitted = false;
vector<elf_symbol_sptr> sorted_deleted_unrefed_var_syms;
sort_string_elf_symbol_map(d.priv_->deleted_unrefed_var_syms_,
sorted_deleted_unrefed_var_syms);
@@ -2205,9 +2204,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
d.first_corpus()->get_fun_symbol_map());
out << "\n";
+ emitted = true;
}
- if (sorted_deleted_unrefed_var_syms.size())
- out << '\n';
+ if (emitted)
+ out << "\n";
}
// Report added variable symbols not referenced by any debug info.
@@ -2223,6 +2223,7 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< s.net_num_added_var_syms()
<< " Added variable symbols not referenced by debug info:\n\n";
+ bool emitted = false;
vector<elf_symbol_sptr> sorted_added_unrefed_var_syms;
sort_string_elf_symbol_map(d.priv_->added_unrefed_var_syms_,
sorted_added_unrefed_var_syms);
@@ -2240,9 +2241,10 @@ default_reporter::report(const corpus_diff& d, ostream& out,
show_linkage_name_and_aliases(out, "", **i,
d.second_corpus()->get_fun_symbol_map());
out << "\n";
+ emitted = true;
}
- if (sorted_added_unrefed_var_syms.size())
- out << '\n';
+ if (emitted)
+ out << "\n";
}
// Report added/removed/changed types not reacheable from public
diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
index 85678013..4b85d816 100644
--- a/src/abg-leaf-reporter.cc
+++ b/src/abg-leaf-reporter.cc
@@ -333,8 +333,8 @@ leaf_reporter::report(const scope_diff& d,
out << indent << " '"
<< (*dif)->first_subject()->get_pretty_representation()
- << "' was changed to '"
- << (*dif)->second_subject()->get_pretty_representation() << "'";
+ << "' was changed to '"
+ << (*dif)->second_subject()->get_pretty_representation() << "'";
report_loc_info((*dif)->second_subject(), *d.context(), out);
out << ":\n";
@@ -1036,7 +1036,6 @@ leaf_reporter::report(const corpus_diff& d,
const diff_context_sptr& ctxt = d.context();
- size_t removed = 0, added = 0;
const corpus_diff::diff_stats &s =
const_cast<corpus_diff&>(d).
apply_filters_and_suppressions_before_reporting();
@@ -1066,6 +1065,7 @@ leaf_reporter::report(const corpus_diff& d,
else if (s.net_num_func_removed() > 1)
out << indent << s.net_num_func_removed() << " Removed functions:\n\n";
+ bool emitted = false;
vector<function_decl*>sorted_deleted_fns;
sort_string_function_ptr_map(d.priv_->deleted_fns_, sorted_deleted_fns);
for (vector<function_decl*>::const_iterator i =
@@ -1098,9 +1098,9 @@ leaf_reporter::report(const corpus_diff& d,
<< c->get_pretty_representation()
<< "\n";
}
- ++removed;
+ emitted = true;
}
- if (removed)
+ if (emitted)
out << "\n";
}
@@ -1114,7 +1114,7 @@ leaf_reporter::report(const corpus_diff& d,
out << indent << num_changed
<< " functions with some sub-type change:\n\n";
- bool changed = false;
+ bool emitted = false;
vector<function_decl_diff_sptr> sorted_changed_fns;
sort_string_function_decl_diff_sptr_map(d.priv_->changed_fns_map_,
sorted_changed_fns);
@@ -1167,14 +1167,11 @@ leaf_reporter::report(const corpus_diff& d,
}
diff->report(out, indent + " ");
out << "\n";
- changed |= true;
+ emitted = true;
}
}
- if (changed)
- {
- out << "\n";
- changed = false;
- }
+ if (emitted)
+ out << "\n";
}
if (ctxt->show_added_fns())
@@ -1184,6 +1181,7 @@ leaf_reporter::report(const corpus_diff& d,
else if (s.net_num_func_added() > 1)
out << indent << s.net_num_func_added()
<< " Added functions:\n\n";
+ bool emitted = false;
vector<function_decl*> sorted_added_fns;
sort_string_function_ptr_map(d.priv_->added_fns_, sorted_added_fns);
for (vector<function_decl*>::const_iterator i = sorted_added_fns.begin();
@@ -1219,13 +1217,10 @@ leaf_reporter::report(const corpus_diff& d,
<< c->get_pretty_representation()
<< "\n";
}
- ++added;
- }
- if (added)
- {
- out << "\n";
- added = false;
+ emitted = true;
}
+ if (emitted)
+ out << "\n";
}
// Now show the changed types.
diff --git a/tests/data/test-abicompat/test1-fn-removed-report-0.txt b/tests/data/test-abicompat/test1-fn-removed-report-0.txt
index acc1584a..bd8560de 100644
--- a/tests/data/test-abicompat/test1-fn-removed-report-0.txt
+++ b/tests/data/test-abicompat/test1-fn-removed-report-0.txt
@@ -6,4 +6,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
'function void fun2()' {_Z4fun2v}
-
diff --git a/tests/data/test-abicompat/test3-fn-removed-report-0.txt b/tests/data/test-abicompat/test3-fn-removed-report-0.txt
index ba05d574..63d5d2e8 100644
--- a/tests/data/test-abicompat/test3-fn-removed-report-0.txt
+++ b/tests/data/test-abicompat/test3-fn-removed-report-0.txt
@@ -6,4 +6,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
'function void _internal_fun1()' {_Z4fun1v@@VERSION_1.0}
-
diff --git a/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt
index 9ae258c9..bd34dbf5 100644
--- a/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt
+++ b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt
@@ -5,4 +5,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
'function void to_erase()' {to_erase}
-
diff --git a/tests/data/test-abidiff/test-PR18791-report0.txt b/tests/data/test-abidiff/test-PR18791-report0.txt
index e24ce778..5758077d 100644
--- a/tests/data/test-abidiff/test-PR18791-report0.txt
+++ b/tests/data/test-abidiff/test-PR18791-report0.txt
@@ -198,4 +198,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
unqualified underlying type 'struct sigc::trackable' changed, as reported earlier
-
diff --git a/tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt b/tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt
index 23e6fb52..5402280e 100644
--- a/tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt
+++ b/tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt
@@ -13,4 +13,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
note that this is an ABI incompatible change to the vtable of struct S
-
diff --git a/tests/data/test-diff-dwarf/test32-fnptr-changes-report-0.txt b/tests/data/test-diff-dwarf/test32-fnptr-changes-report-0.txt
index 1dd410ca..91b92d20 100644
--- a/tests/data/test-diff-dwarf/test32-fnptr-changes-report-0.txt
+++ b/tests/data/test-diff-dwarf/test32-fnptr-changes-report-0.txt
@@ -46,4 +46,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
-
diff --git a/tests/data/test-diff-dwarf/test33-fnref-changes-report-0.txt b/tests/data/test-diff-dwarf/test33-fnref-changes-report-0.txt
index 46e71c78..fcab9120 100644
--- a/tests/data/test-diff-dwarf/test33-fnref-changes-report-0.txt
+++ b/tests/data/test-diff-dwarf/test33-fnref-changes-report-0.txt
@@ -48,4 +48,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
-
diff --git a/tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt b/tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt
index 52692d9f..f0a25a2e 100644
--- a/tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt
+++ b/tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt
@@ -79,4 +79,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
details were reported earlier
-
diff --git a/tests/data/test-diff-filter/test10-report.txt b/tests/data/test-diff-filter/test10-report.txt
index 64969890..bdc31fd0 100644
--- a/tests/data/test-diff-filter/test10-report.txt
+++ b/tests/data/test-diff-filter/test10-report.txt
@@ -17,4 +17,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
-
diff --git a/tests/data/test-diff-filter/test41-report-0.txt b/tests/data/test-diff-filter/test41-report-0.txt
index cdae9c56..c908a059 100644
--- a/tests/data/test-diff-filter/test41-report-0.txt
+++ b/tests/data/test-diff-filter/test41-report-0.txt
@@ -50,7 +50,6 @@ Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referen
[C] 'method bool abigail::xml_writer::write_context::type_ptr_cmp::operator()(const abigail::ir::type_base*, const abigail::ir::type_base*) const' at abg-writer.cc:359:1 has some indirect sub-type changes:
-
1 Removed function symbol not referenced by debug info:
_ZN7abigail10xml_writer13write_contextD1Ev
diff --git a/tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt b/tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt
index b3be1535..d73fae52 100644
--- a/tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt
+++ b/tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt
@@ -52,6 +52,5 @@
'method void std::vector<ISO9660::Stat*, std::allocator<ISO9660::Stat*> >::_M_realloc_insert<ISO9660::Stat*>(std::vector<ISO9660::Stat*, std::allocator<ISO9660::Stat*> >::iterator, ISO9660::Stat*&&)' {_ZNSt6vectorIPN7ISO96604StatESaIS2_EE17_M_realloc_insertIJS2_EEEvN9__gnu_cxx17__normal_iteratorIPS2_S4_EEDpOT_}
-
================ end of changes of 'libiso9660++.so.0.0.0'===============
diff --git a/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 b/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
index 3819aed9..05d6fcad 100644
--- a/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
+++ b/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
@@ -92,6 +92,5 @@
-
================ end of changes of 'libsigc-2.0.so.0.0.0'===============
diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-0.txt b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-0.txt
index 9251c43f..bf081f52 100644
--- a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-0.txt
+++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-0.txt
@@ -54,6 +54,5 @@
enum type 'enum __anonymous_enum__2' changed at spice.h:471:1, as reported earlier
-
================ end of changes of 'libspice-server.so.1.8.0'===============
diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-1.txt b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-1.txt
index 9251c43f..bf081f52 100644
--- a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-1.txt
+++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-1.txt
@@ -54,6 +54,5 @@
enum type 'enum __anonymous_enum__2' changed at spice.h:471:1, as reported earlier
-
================ end of changes of 'libspice-server.so.1.8.0'===============
diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
index b885629e..b4104bda 100644
--- a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
+++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
@@ -392,6 +392,5 @@
enum type 'enum __anonymous_enum__2' changed at spice.h:471:1, as reported earlier
-
================ end of changes of 'libspice-server.so.1.8.0'===============
diff --git a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
index 69f438ea..f8e60b50 100644
--- a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
+++ b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
@@ -255,7 +255,6 @@
'function size_t safer_scalable_msize(void*, typedef size_t (void*)*)' {safer_scalable_msize}
'function void* safer_scalable_realloc(void*, size_t, void*)' {safer_scalable_realloc}
-
27 Added function symbols not referenced by debug info:
_Z10BitScanRevm
@@ -304,6 +303,5 @@
'function size_t malloc_usable_size(void*)' {malloc_usable_size}
'function void* valloc(size_t)' {__libc_valloc, aliases valloc}
-
================ end of changes of 'libtbbmalloc_proxy.so.2'===============
diff --git a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt
index de74f1c5..10816705 100644
--- a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt
+++ b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt
@@ -111,7 +111,6 @@
'function size_t safer_scalable_msize(void*, typedef size_t (void*)*)' {safer_scalable_msize}
'function void* safer_scalable_realloc(void*, size_t, void*)' {safer_scalable_realloc}
-
27 Added function symbols not referenced by debug info:
_Z10BitScanRevm
@@ -160,6 +159,5 @@
'function size_t malloc_usable_size(void*)' {malloc_usable_size}
'function void* valloc(size_t)' {__libc_valloc, aliases valloc}
-
================ end of changes of 'libtbbmalloc_proxy.so.2'===============
diff --git a/tests/data/test-diff-pkg/test-rpm-report-0.txt b/tests/data/test-diff-pkg/test-rpm-report-0.txt
index 3fb8b8cc..df340812 100644
--- a/tests/data/test-diff-pkg/test-rpm-report-0.txt
+++ b/tests/data/test-diff-pkg/test-rpm-report-0.txt
@@ -28,7 +28,6 @@
'function BaseInfo* base_info_ref(BaseInfo*)' {base_info_ref}
'function void base_info_unref(BaseInfo*)' {base_info_unref}
-
1 Added function symbol not referenced by debug info:
dbus_g_value_build_g_variant
diff --git a/tests/data/test-diff-pkg/test-rpm-report-5.txt b/tests/data/test-diff-pkg/test-rpm-report-5.txt
index a1015ef8..56140b44 100644
--- a/tests/data/test-diff-pkg/test-rpm-report-5.txt
+++ b/tests/data/test-diff-pkg/test-rpm-report-5.txt
@@ -8,6 +8,5 @@
'function BaseInfo* base_info_ref(BaseInfo*)' {base_info_ref}
'function void base_info_unref(BaseInfo*)' {base_info_unref}
-
================ end of changes of 'dbus-binding-tool'===============
diff --git a/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-0.txt b/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-0.txt
index e64a5b55..054254fb 100644
--- a/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-0.txt
+++ b/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-0.txt
@@ -15,4 +15,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
'unsigned int S::bar', at offset 32 (in bits)
-
diff --git a/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-3.txt b/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-3.txt
index 1d859f62..29d021e9 100644
--- a/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-3.txt
+++ b/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-3.txt
@@ -5,4 +5,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
'function void bar()' {_Z3barv}
-
diff --git a/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-1.txt b/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-1.txt
index 44031050..1dab9503 100644
--- a/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-1.txt
+++ b/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-1.txt
@@ -3,4 +3,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 0 Added (1 filtered out) function symbol not referenced by debug info
Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info
-
diff --git a/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-2.txt b/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-2.txt
index 44031050..1dab9503 100644
--- a/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-2.txt
+++ b/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-2.txt
@@ -3,4 +3,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 0 Added (1 filtered out) function symbol not referenced by debug info
Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info
-
diff --git a/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-4.txt b/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-4.txt
index 44031050..1dab9503 100644
--- a/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-4.txt
+++ b/tests/data/test-diff-suppr/test19-suppr-added-fn-sym-report-4.txt
@@ -3,4 +3,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 0 Added (1 filtered out) function symbol not referenced by debug info
Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info
-
diff --git a/tests/data/test-diff-suppr/test21-suppr-added-var-sym-report-5.txt b/tests/data/test-diff-suppr/test21-suppr-added-var-sym-report-5.txt
index d0ff1d37..1df0ef5c 100644
--- a/tests/data/test-diff-suppr/test21-suppr-added-var-sym-report-5.txt
+++ b/tests/data/test-diff-suppr/test21-suppr-added-var-sym-report-5.txt
@@ -3,4 +3,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 0 Added function symbol not referenced by debug info
Variable symbols changes summary: 0 Removed, 0 Added (2 filtered out) variable symbols not referenced by debug info
-
diff --git a/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-report-2.txt b/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-report-2.txt
index 2b4f9883..e0914d64 100644
--- a/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-report-2.txt
+++ b/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-report-2.txt
@@ -3,7 +3,6 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 0 Added function symbol not referenced by debug info
Variable symbols changes summary: 0 Removed (2 filtered out), 1 Added variable symbols not referenced by debug info
-
1 Added variable symbol not referenced by debug info:
global_var3
diff --git a/tests/data/test-diff-suppr/test30-report-0.txt b/tests/data/test-diff-suppr/test30-report-0.txt
index be32d3f7..9ac85d53 100644
--- a/tests/data/test-diff-suppr/test30-report-0.txt
+++ b/tests/data/test-diff-suppr/test30-report-0.txt
@@ -30,4 +30,3 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
-
--
2.25.1.481.gfbce0eb801-goog
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] [abidiff] Add more leaf change reporting.
2020-03-11 10:35 [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections Giuliano Procida
@ 2020-03-11 10:35 ` Giuliano Procida
2020-03-13 10:28 ` Dodji Seketeli
2020-03-12 13:03 ` [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections Dodji Seketeli
1 sibling, 1 reply; 9+ messages in thread
From: Giuliano Procida @ 2020-03-11 10:35 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida
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.
* 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.
* 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.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
src/abg-default-reporter.cc | 12 +-
src/abg-leaf-reporter.cc | 328 ++++++++++++++++--
tests/data/Makefile.am | 5 +
.../test-abidiff-exit/test-leaf0-report.txt | 9 +
.../test-abidiff-exit/test-leaf1-report.txt | 38 ++
tests/data/test-abidiff-exit/test-leaf1-v0.cc | 9 +
tests/data/test-abidiff-exit/test-leaf1-v0.o | Bin 0 -> 3160 bytes
tests/data/test-abidiff-exit/test-leaf1-v1.cc | 9 +
tests/data/test-abidiff-exit/test-leaf1-v1.o | Bin 0 -> 3176 bytes
tests/test-abidiff-exit.cc | 10 +
10 files changed, 383 insertions(+), 37 deletions(-)
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-report.txt
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v0.cc
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v0.o
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v1.cc
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v1.o
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index 2ac93d41..3bfdfbc4 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -1761,18 +1761,12 @@ void
default_reporter::report(const corpus_diff& d, ostream& out,
const string& indent) const
{
- size_t total = 0;
const corpus_diff::diff_stats &s =
const_cast<corpus_diff&>(d).
apply_filters_and_suppressions_before_reporting();
const diff_context_sptr& ctxt = d.context();
- /// Report removed/added/changed functions.
- total = s.net_num_func_removed() + s.net_num_func_added() +
- s.net_num_func_changed();
- const unsigned large_num = 100;
-
d.priv_->emit_diff_stats(s, out, indent);
if (ctxt->show_stats_only())
return;
@@ -1790,6 +1784,11 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< d.first_corpus()->get_architecture_name() << "' to '"
<< d.second_corpus()->get_architecture_name() << "'\n\n";
+ /// Report removed/added/changed functions.
+ size_t total = s.net_num_func_removed() + s.net_num_func_added() +
+ s.net_num_func_changed();
+ const unsigned large_num = 100;
+
if (ctxt->show_deleted_fns())
{
if (s.net_num_func_removed() == 1)
@@ -2249,7 +2248,6 @@ default_reporter::report(const corpus_diff& d, ostream& out,
// Report added/removed/changed types not reacheable from public
// interfaces.
-
maybe_report_unreachable_type_changes(d, s, indent, out);
d.priv_->maybe_dump_diff_tree();
diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
index 4b85d816..878e4125 100644
--- a/src/abg-leaf-reporter.cc
+++ b/src/abg-leaf-reporter.cc
@@ -1034,16 +1034,15 @@ leaf_reporter::report(const corpus_diff& d,
if (!d.has_changes())
return;
- const diff_context_sptr& ctxt = d.context();
-
const corpus_diff::diff_stats &s =
const_cast<corpus_diff&>(d).
apply_filters_and_suppressions_before_reporting();
+ const diff_context_sptr& ctxt = d.context();
+
d.priv_->emit_diff_stats(s, out, indent);
if (ctxt->show_stats_only())
return;
-
out << "\n";
if (ctxt->show_soname_change()
@@ -1058,6 +1057,7 @@ leaf_reporter::report(const corpus_diff& d,
<< d.first_corpus()->get_architecture_name() << "' to '"
<< d.second_corpus()->get_architecture_name() << "'\n\n";
+ /// Report removed/added/changed functions.
if (ctxt->show_deleted_fns())
{
if (s.net_num_func_removed() == 1)
@@ -1104,6 +1104,55 @@ leaf_reporter::report(const corpus_diff& d,
out << "\n";
}
+ if (ctxt->show_added_fns())
+ {
+ if (s.net_num_func_added() == 1)
+ out << indent << "1 Added function:\n\n";
+ else if (s.net_num_func_added() > 1)
+ out << indent << s.net_num_func_added()
+ << " Added functions:\n\n";
+ bool emitted = false;
+ vector<function_decl*> sorted_added_fns;
+ sort_string_function_ptr_map(d.priv_->added_fns_, sorted_added_fns);
+ for (vector<function_decl*>::const_iterator i = sorted_added_fns.begin();
+ i != sorted_added_fns.end();
+ ++i)
+ {
+ if (d.priv_->added_function_is_suppressed(*i))
+ continue;
+
+ out
+ << indent
+ << " ";
+ out << "[A] ";
+ out << "'"
+ << (*i)->get_pretty_representation()
+ << "'";
+ if (ctxt->show_linkage_names())
+ {
+ out << " {";
+ show_linkage_name_and_aliases
+ (out, "", *(*i)->get_symbol(),
+ d.second_corpus()->get_fun_symbol_map());
+ out << "}";
+ }
+ out << "\n";
+ if (is_member_function(*i) && get_member_function_is_virtual(*i))
+ {
+ class_decl_sptr c =
+ is_class_type(is_method_type((*i)->get_type())->get_class_type());
+ out << indent
+ << " "
+ << "note that this adds a new entry to the vtable of "
+ << c->get_pretty_representation()
+ << "\n";
+ }
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
if (ctxt->show_changed_fns())
{
// Show changed functions.
@@ -1174,55 +1223,274 @@ leaf_reporter::report(const corpus_diff& d,
out << "\n";
}
- if (ctxt->show_added_fns())
+ // Report removed/added/changed variables.
+ if (ctxt->show_deleted_vars())
{
- if (s.net_num_func_added() == 1)
- out << indent << "1 Added function:\n\n";
- else if (s.net_num_func_added() > 1)
- out << indent << s.net_num_func_added()
- << " Added functions:\n\n";
+ if (s.net_num_vars_removed() == 1)
+ out << indent << "1 Removed variable:\n\n";
+ else if (s.net_num_vars_removed() > 1)
+ out << indent << s.net_num_vars_removed()
+ << " Removed variables:\n\n";
+ string n;
bool emitted = false;
- vector<function_decl*> sorted_added_fns;
- sort_string_function_ptr_map(d.priv_->added_fns_, sorted_added_fns);
- for (vector<function_decl*>::const_iterator i = sorted_added_fns.begin();
- i != sorted_added_fns.end();
+ vector<var_decl*> sorted_deleted_vars;
+ sort_string_var_ptr_map(d.priv_->deleted_vars_, sorted_deleted_vars);
+ for (vector<var_decl*>::const_iterator i =
+ sorted_deleted_vars.begin();
+ i != sorted_deleted_vars.end();
++i)
{
- if (d.priv_->added_function_is_suppressed(*i))
+ if (d.priv_->deleted_variable_is_suppressed(*i))
continue;
- out
- << indent
- << " ";
- out << "[A] ";
+ n = (*i)->get_pretty_representation();
+
+ out << indent
+ << " ";
+ out << "[D] ";
out << "'"
- << (*i)->get_pretty_representation()
+ << n
<< "'";
if (ctxt->show_linkage_names())
{
out << " {";
- show_linkage_name_and_aliases
- (out, "", *(*i)->get_symbol(),
- d.second_corpus()->get_fun_symbol_map());
+ show_linkage_name_and_aliases(out, "", *(*i)->get_symbol(),
+ d.first_corpus()->get_var_symbol_map());
out << "}";
}
out << "\n";
- if (is_member_function(*i) && get_member_function_is_virtual(*i))
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ if (ctxt->show_added_vars())
+ {
+ if (s.net_num_vars_added() == 1)
+ out << indent << "1 Added variable:\n\n";
+ else if (s.net_num_vars_added() > 1)
+ out << indent << s.net_num_vars_added()
+ << " Added variables:\n\n";
+ string n;
+ bool emitted = false;
+ vector<var_decl*> sorted_added_vars;
+ sort_string_var_ptr_map(d.priv_->added_vars_, sorted_added_vars);
+ for (vector<var_decl*>::const_iterator i =
+ sorted_added_vars.begin();
+ i != sorted_added_vars.end();
+ ++i)
+ {
+ if (d.priv_->added_variable_is_suppressed(*i))
+ continue;
+
+ n = (*i)->get_pretty_representation();
+
+ out << indent
+ << " ";
+ out << "[A] ";
+ out << "'" << n << "'";
+ if (ctxt->show_linkage_names())
{
- class_decl_sptr c =
- is_class_type(is_method_type((*i)->get_type())->get_class_type());
- out << indent
- << " "
- << "note that this adds a new entry to the vtable of "
- << c->get_pretty_representation()
- << "\n";
+ out << " {";
+ show_linkage_name_and_aliases(out, "", *(*i)->get_symbol(),
+ d.second_corpus()->get_var_symbol_map());
+ out << "}";
}
+ out << "\n";
emitted = true;
}
if (emitted)
out << "\n";
}
+ if (ctxt->show_changed_vars())
+ {
+ size_t num_changed =
+ s.num_vars_changed() - s.num_changed_vars_filtered_out();
+ if (num_changed == 1)
+ out << indent << "1 Changed variable:\n\n";
+ else if (num_changed > 1)
+ out << indent << num_changed
+ << " Changed variables:\n\n";
+ string n1, n2;
+ bool emitted = false;
+ for (var_diff_sptrs_type::const_iterator i =
+ d.priv_->sorted_changed_vars_.begin();
+ i != d.priv_->sorted_changed_vars_.end();
+ ++i)
+ {
+ diff_sptr diff = *i;
+
+ if (!diff)
+ continue;
+
+ if (!diff->to_be_reported())
+ continue;
+
+ n1 = diff->first_subject()->get_pretty_representation();
+ n2 = diff->second_subject()->get_pretty_representation();
+
+ out << indent << " [C] '" << n1 << "' was changed";
+ if (n1 != n2)
+ out << " to '" << n2 << "'";
+ report_loc_info(diff->second_subject(), *ctxt, out);
+ out << ":\n";
+ diff->report(out, indent + " ");
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report removed function symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && d.priv_->deleted_unrefed_fn_syms_.size())
+ {
+ if (s.net_num_removed_func_syms() == 1)
+ out << indent
+ << "1 Removed function symbol not referenced by debug info:\n\n";
+ else if (s.net_num_removed_func_syms() > 0)
+ out << indent
+ << s.net_num_removed_func_syms()
+ << " Removed function symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_deleted_unrefed_fn_syms;
+ sort_string_elf_symbol_map(d.priv_->deleted_unrefed_fn_syms_,
+ sorted_deleted_unrefed_fn_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_deleted_unrefed_fn_syms.begin();
+ i != sorted_deleted_unrefed_fn_syms.end();
+ ++i)
+ {
+ if (d.priv_->deleted_unrefed_fn_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[D] ";
+
+ show_linkage_name_and_aliases(out, "", **i,
+ d.first_corpus()->get_fun_symbol_map());
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report added function symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && ctxt->show_added_symbols_unreferenced_by_debug_info()
+ && d.priv_->added_unrefed_fn_syms_.size())
+ {
+ if (s.net_num_added_func_syms() == 1)
+ out << indent
+ << "1 Added function symbol not referenced by debug info:\n\n";
+ else if (s.net_num_added_func_syms() > 0)
+ out << indent
+ << s.net_num_added_func_syms()
+ << " Added function symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_added_unrefed_fn_syms;
+ sort_string_elf_symbol_map(d.priv_->added_unrefed_fn_syms_,
+ sorted_added_unrefed_fn_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_added_unrefed_fn_syms.begin();
+ i != sorted_added_unrefed_fn_syms.end();
+ ++i)
+ {
+ if (d.priv_->added_unrefed_fn_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[A] ";
+ show_linkage_name_and_aliases(out, "",
+ **i,
+ d.second_corpus()->get_fun_symbol_map());
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report removed variable symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && d.priv_->deleted_unrefed_var_syms_.size())
+ {
+ if (s.net_num_removed_var_syms() == 1)
+ out << indent
+ << "1 Removed variable symbol not referenced by debug info:\n\n";
+ else if (s.net_num_removed_var_syms() > 0)
+ out << indent
+ << s.net_num_removed_var_syms()
+ << " Removed variable symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_deleted_unrefed_var_syms;
+ sort_string_elf_symbol_map(d.priv_->deleted_unrefed_var_syms_,
+ sorted_deleted_unrefed_var_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_deleted_unrefed_var_syms.begin();
+ i != sorted_deleted_unrefed_var_syms.end();
+ ++i)
+ {
+ if (d.priv_->deleted_unrefed_var_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[D] ";
+
+ show_linkage_name_and_aliases
+ (out, "", **i,
+ d.first_corpus()->get_fun_symbol_map());
+
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report added variable symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && ctxt->show_added_symbols_unreferenced_by_debug_info()
+ && d.priv_->added_unrefed_var_syms_.size())
+ {
+ if (s.net_num_added_var_syms() == 1)
+ out << indent
+ << "1 Added variable symbol not referenced by debug info:\n\n";
+ else if (s.net_num_added_var_syms() > 0)
+ out << indent
+ << s.net_num_added_var_syms()
+ << " Added variable symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_added_unrefed_var_syms;
+ sort_string_elf_symbol_map(d.priv_->added_unrefed_var_syms_,
+ sorted_added_unrefed_var_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_added_unrefed_var_syms.begin();
+ i != sorted_added_unrefed_var_syms.end();
+ ++i)
+ {
+ if (d.priv_->added_unrefed_var_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[A] ";
+ show_linkage_name_and_aliases(out, "", **i,
+ d.second_corpus()->get_fun_symbol_map());
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
// Now show the changed types.
const diff_maps& leaf_diffs = d.get_leaf_diffs();
report_type_changes_from_diff_maps(*this, leaf_diffs, out, indent);
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 07077608..5dab8685 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -115,6 +115,11 @@ test-abidiff-exit/test-leaf0-v0.o \
test-abidiff-exit/test-leaf0-v1.cc \
test-abidiff-exit/test-leaf0-v1.o \
test-abidiff-exit/test-leaf0-report.txt \
+test-abidiff-exit/test-leaf1-v0.cc \
+test-abidiff-exit/test-leaf1-v0.o \
+test-abidiff-exit/test-leaf1-v1.cc \
+test-abidiff-exit/test-leaf1-v1.o \
+test-abidiff-exit/test-leaf1-report.txt \
\
test-diff-dwarf/test0-v0.cc \
test-diff-dwarf/test0-v0.o \
diff --git a/tests/data/test-abidiff-exit/test-leaf0-report.txt b/tests/data/test-abidiff-exit/test-leaf0-report.txt
index f823789d..7d15e28f 100644
--- a/tests/data/test-abidiff-exit/test-leaf0-report.txt
+++ b/tests/data/test-abidiff-exit/test-leaf0-report.txt
@@ -11,3 +11,12 @@ Removed/Changed/Added variables summary: 0 Removed, 1 Changed, 0 Added variable
type size changed from 32 to 64 (in bits)
+1 Changed variable:
+
+ [C] 'int changed_var' was changed to 'long int changed_var':
+ size of symbol changed from 4 to 8
+ type of variable changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+
+
diff --git a/tests/data/test-abidiff-exit/test-leaf1-report.txt b/tests/data/test-abidiff-exit/test-leaf1-report.txt
new file mode 100644
index 00000000..9da015bf
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-leaf1-report.txt
@@ -0,0 +1,38 @@
+Leaf changes summary: 6 artifacts changed
+Changed leaf types summary: 0 leaf type changed
+Removed/Changed/Added functions summary: 1 Removed, 1 Changed, 1 Added function
+Removed/Changed/Added variables summary: 1 Removed, 1 Changed, 1 Added variable
+
+1 Removed function:
+
+ [D] 'function int deleted_fun()' {_Z11deleted_funv}
+
+1 Added function:
+
+ [A] 'function long int added_fun()' {_Z9added_funv}
+
+1 function with some sub-type change:
+
+ [C] 'function int changed_fun()' has some sub-type changes:
+ return type changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+
+
+1 Removed variable:
+
+ [D] 'int deleted_var' {deleted_var}
+
+1 Added variable:
+
+ [A] 'long int added_var' {added_var}
+
+1 Changed variable:
+
+ [C] 'int changed_var' was changed to 'long int changed_var':
+ size of symbol changed from 4 to 8
+ type of variable changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+
+
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v0.cc b/tests/data/test-abidiff-exit/test-leaf1-v0.cc
new file mode 100644
index 00000000..ae395419
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-leaf1-v0.cc
@@ -0,0 +1,9 @@
+int deleted_var = 0;
+int changed_var = 0;
+
+int deleted_fun() {
+ return 0;
+}
+int changed_fun() {
+ return 0;
+}
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v0.o b/tests/data/test-abidiff-exit/test-leaf1-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..fbdc54aa2dec2fe64aed01a82036e3f98ef8ff22
GIT binary patch
literal 3160
zcmbtV-HRJl6hD*Mba$I>$Zo85S(ONiwsoeNx-9P2ZKKiFvZWQJMFk0y<Yp60CQCAj
zTl=7(2#OCy1S|AKMCgkz`qHOT@ZJBycYPB?^qf2ABzNy*eeuA|Ip=pi?z#7#d+$fD
zUAdTX91wEgJWMq~0q)FC<%$+7P=pn@{`%*?-6#FMUknl8JA~%6--1q;$Qnp~j~a4@
zWXH)?kuADnaObj}D2BwCv5Y#OGoK}A3KPn);M`3-=KjM2oMq>kd@<+d*YeBG%6#E5
zfXq6$`K{c!+-B}_@xp561c3WA<Z|b8?gGxphYPD>kMfs`6jnL$%`2-o?DP2PVgWr3
zGG}VQa19j~p2ff8{FPtAzupI2E(c;YjJ>W3TY)z&`;CT<%NAiV835Je*`qsI=bdcv
zOd@15n4tD<5H#9hw5yu6)+ic7Q*~9GDCwNyISvPK>FV`T_4Mgry|n3{^@Gyca<Cbc
zUnrNnUObA__O6Q5pwmD#95mY7`!8;I8|x)+x8$|L;mxSg9`vLBXy}ci4?0oPi^F<X
z4M9f0T7YUZ${E9%xVJYckZOQS)#_I1xmQ)a6GllLUa)b}sz@&dJd!YQK713L{erW0
zY<}T3F5TmZ(?_Ix;pKecvrKjA$VGHOjBGEF5owk^v84ah#B=b+KWPhXqHgh&$>nyi
z4K7B^B9~8$ZFAv>4Qbs4<4_tNE-V__RCJRWPXU%(SW008xR5^tKbFE*5Lq_nCFlJ~
z#zhx?3h@<V-Q?V!V_bCM>F$+YqRx>%BRSP)sY8K7Ojh)$Arme$Bb-)oi$J91gn{5s
zuzuXae`K951S0B;$(cp@B!Nhj$Gj$djV3sa`xEQ59z>MK+@&3jPW>hRZN|yp+`Z89
zzs-8v!dZnq7NmaJhfgejS(h&?{4VQXS$SAJu>SgrW<%i+|BXZN663VS<~mm_T<QmZ
zc(aGM6Y_YF@U}>?+K<6+hH(gfeK-Vvpt_;oRQ1trtrNBSXw!07uMgDtfH@q{rF$@>
z+liEf;1J+9`n{gQ7tN3QvGVc8^@efSc%KugcC9rCdx{JHpKJ}|Y47Ztqr!RZM~Qq*
zdI}Z7?3?O_>14=uJUwgF-*&w8r`h|IM(zcYi30rWoP<mK52P4m`_f}fIV*+_iGT7y
zA9Dcp8$nVJzW188@o(oO!{6EpaFw|*+xHDL*ckclFj$sA=C&`L?^*Tl^M*=1)laX0
zU4IiXJ10VLi@~=@G066<p>5Xq_jr6+CmNsbD|`Gs#3-lKiK3=od7uXZFqiR#-$Hy=
z{Fi*lslQB;IN_h7QDoEaXY-OU47TsL9B;edXF&Q;Qpo&8{~Co^>-TWR{?R=x@v^Q|
zk3BZ!F7Sc6!v)j*OYedmPkUmYJ%r#M6@_#qVUxc-f66;6{&%kb`$=RX*zx}YtC{4n
literal 0
HcmV?d00001
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v1.cc b/tests/data/test-abidiff-exit/test-leaf1-v1.cc
new file mode 100644
index 00000000..099803ef
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-leaf1-v1.cc
@@ -0,0 +1,9 @@
+long changed_var = 0;
+long added_var = 0;
+
+long changed_fun() {
+ return 0;
+}
+long added_fun() {
+ return 0;
+}
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v1.o b/tests/data/test-abidiff-exit/test-leaf1-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..8b2fb3f775b0b8bb7d5a8019320154cd904dd860
GIT binary patch
literal 3176
zcmbtW-HRJl6hD*M>_^iL*=(!3tjH)9Tk1@A>asMg+eV|imX#_>MGF!p$;}5QlVvhl
zxAwsYMbw8PBKUzXqC#JM5eoY#_%8kvzUy-lJ?GwgvbQ%=5j>E4&iS2>d+xa(llNYD
zX*=gQz~sPXIMNsexHESoSEX2mWhle-7eD^}ko32HF+_lG5t@;H^D>>1C7_0f3^_-#
z<IL8OEx2rO=kp{gW6m<le9qh~c~BTrK?}||%46nVJiu9UP8F96ZgH)+<do-1594eb
z;1(|yE)^<;tIJnbb1MMu6Hq8zMlW1Z09RJo9v81JQ&@H7i|1Bx+Nba*i-mcvMsjnd
zrr~8&TzC>k$GKNr#8K`8w#$K3j8eZZqIT%-he5L`<MJn9K}~?_$^5b1yz^$hd`=0u
z944sW2*YMKiaVlJZ;#{s={#)Rj}nMlE%YO4=h}6zcJ^$z;Z=h3LFk=d4=dsN)9ao;
zNXN0*>WEk*y(X$r((G;>T-fwCH$1=N`R!<QGj4X1VLTj<{BitFFK+p1)aZ*5@LX7l
zo{OaZFz$H0IE8cRU=QjyDyj@wF6`857roQZi$*VsRV{wFdB&=WnsGvQ1Y7m)tKb}z
zoVDX~^Pght9zmRbMDi>=TP%H;t1ZrMqXS}OdxnfiS$1Wa9_bLzz@L9lTWAyI5uY--
z`Z(AI7bCLB)i=hrxgZa*4PzGLQW{nl77Xo3bk&NF04=$&IE4}50v`Dca^d(CT1I5a
zSQlS`M>Wnm^V5izjlS`{t!SKe=Ku91Pu&3PfN>{};XEv(I9U<l)V&J?A}uKfjI(~y
z!hh5{Jp@E_A|`hg<ue2#Q6BCA=>ZK$od0p9!X?cqkGVVB8fX9SwZ3cNoc9flQyz0S
zBg_A5t#>W_C#~;k&g<fRdEfHCt@TeVT&wVz7B~;%yce`C`Rr>$=>h(m55PT*Q}50G
zS1p|DhhTJbfbSLZG*P_EQYsEo2wG7ZLC_eDAV@?%3R<Eu?$mp6dx$nEM~y}z_U|)C
z2|dF1hxB`~;1HYwg642A5cm)V@h}wuzQO(|jhb)kM50@7C(%IY!v7~*!+7dB_nFgG
zwP1gw$k(LDP$A5Ixn4J&4B3vS&m5h5JD$%c?=z<`?+9Tb{_LEDbNmma7-aj>$C>^S
zR1F^z|B!zZ0KU`#R0lyk{|Y%FZQ;+(N%&thAYD^zGTOecpvA_>c~^t$lSn1lzI4B{
z>i<Y@D92O%^i{CyZy{#qL<nwa@HJ8lvVH4l%bNdC&(D3L`RV<%=ift&a&nz0O8Qk#
z^t0BP@%+qhp&={&w!Y+azD(jc<{zMuWz+Aj`A8TB+xIISZ@V9AfX^YP;PtcqISN_*
z`z>St=$+<x?km+}&rP{Y`a<#frSB7cFYI{Q6Z`HV1b3(?q#ea3qrHC0n-$N0LB5+r
ID#4Eb2jH*bIRF3v
literal 0
HcmV?d00001
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 5372b3fe..cb87f63f 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -129,6 +129,16 @@ InOutSpec in_out_specs[] =
"data/test-abidiff-exit/test-leaf0-report.txt",
"output/test-abidiff-exit/test-leaf0-report.txt"
},
+ {
+ "data/test-abidiff-exit/test-leaf1-v0.o",
+ "data/test-abidiff-exit/test-leaf1-v1.o",
+ "",
+ "--no-show-locs --leaf-changes-only",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE
+ | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
+ "data/test-abidiff-exit/test-leaf1-report.txt",
+ "output/test-abidiff-exit/test-leaf1-report.txt"
+ },
{0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
};
--
2.25.1.481.gfbce0eb801-goog
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections.
2020-03-11 10:35 [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections Giuliano Procida
2020-03-11 10:35 ` [PATCH 2/2] [abidiff] Add more leaf change reporting Giuliano Procida
@ 2020-03-12 13:03 ` Dodji Seketeli
1 sibling, 0 replies; 9+ messages in thread
From: Dodji Seketeli @ 2020-03-12 13:03 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team
Hello Giuliano,
Giuliano Procida <gprocida@google.com> a écrit:
> 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.
This looks good to me and I have applied to master.
Thanks!
--
Dodji
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] [abidiff] Add more leaf change reporting.
2020-03-11 10:35 ` [PATCH 2/2] [abidiff] Add more leaf change reporting Giuliano Procida
@ 2020-03-13 10:28 ` Dodji Seketeli
2020-03-13 14:00 ` Giuliano Procida
0 siblings, 1 reply; 9+ messages in thread
From: Dodji Seketeli @ 2020-03-13 10:28 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team
Hello Giuliano,
Giuliano Procida <gprocida@google.com> a écrit:
> 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.
Right. Thank you for looking into this. I totally agree with the
direction of the patch.
>
> - The addition or removal of any symbol affects the ABI and is
> clearly a leaf change.
Agreed, these ought to added.
> - A change to a variable's declaration may be local rather than
> caused by a type change elsewhere.
This is correct. We really need to be aware of the fact of there can be
two kinds of changes on a variable: local or indirect changes.
A local change would be a change that affects the variable directly,
like its type was changed from char to int.
An indirect change would be, for instance, a change in a sub-type of the
type of the variable. For instance, the variable is of type "struct
foo*" and there was a change in the type of one of the data members of
struct foo.
In the leaf change reporter, we want to report local (leaf) changes
only.
And I think the patch misses that point somewhat because it show *all*
changes to variables, including changes that are not local:
[...]
> diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
> index 4b85d816..878e4125 100644
> --- a/src/abg-leaf-reporter.cc
> +++ b/src/abg-leaf-reporter.cc
> @@ -1034,16 +1034,15 @@ leaf_reporter::report(const corpus_diff& d,
> if (!d.has_changes())
> return;
[...]
> + if (ctxt->show_changed_vars())
> + {
> + size_t num_changed =
> + s.num_vars_changed() - s.num_changed_vars_filtered_out();
Here, I think we should rather do:
size_t num_changed = s.net_num_leaf_var_changes()
For the record, this is what the doc of that function says:
/// Getter for the net number of leaf variable change diff nodes.
///
/// This the difference between the number of leaf variable change
/// diff nodes and the number of filtered out leaf variable change
/// diff nodes.
///
/// @return the net number of leaf variable change diff nodes.
size_t
corpus_diff::diff_stats::net_num_leaf_var_changes() const
Then ...
> + if (num_changed == 1)
> + out << indent << "1 Changed variable:\n\n";
> + else if (num_changed > 1)
> + out << indent << num_changed
> + << " Changed variables:\n\n";
> + string n1, n2;
> + bool emitted = false;
> + for (var_diff_sptrs_type::const_iterator i =
> + d.priv_->sorted_changed_vars_.begin();
> + i != d.priv_->sorted_changed_vars_.end();
> + ++i)
> + {
> + diff_sptr diff = *i;
> +
> + if (!diff)
> + continue;
> +
> + if (!diff->to_be_reported())
> + continue;
... here, I think you need to rather do:
if (diff_to_be_reported(diff.get()))
continue;
That is, use the member function leaf_reporter::diff_to_be_reported(),
rather than diff::to_be_reported().
The former function tells you if the diff is to be reported by the leaf
reporter (as opposed to by another reporter). That is, if the diff
carries a change that has not been filtered out and if it carries a
local change.
> +
> + n1 = diff->first_subject()->get_pretty_representation();
> + n2 = diff->second_subject()->get_pretty_representation();
> +
> + out << indent << " [C] '" << n1 << "' was changed";
> + if (n1 != n2)
> + out << " to '" << n2 << "'";
If leaf_reporter::diff_to_be_reported() returns true, then you know the
diff carries a local change. So you don't need to test if the pretty
representation of the variable has changed; note that testing if the
pretty representation of the variable has changed is a poor man's (and
not always precise) way of testing if the variable has a local change.
Doing that became unnecessary once we've acquired the capability of
detecting local versus non-local changes in libabigail. So we ought to
use that capability now.
I hope this makes sense. If not, please do not hesitate to tell me what
I am missing.
Thanks again for looking into this!
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] [abidiff] Add more leaf change reporting.
2020-03-13 10:28 ` Dodji Seketeli
@ 2020-03-13 14:00 ` Giuliano Procida
2020-03-13 15:34 ` [PATCH 2/2 v2] " Giuliano Procida
0 siblings, 1 reply; 9+ messages in thread
From: Giuliano Procida @ 2020-03-13 14:00 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team
Hi Dodji.
On Fri, 13 Mar 2020 at 10:28, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > 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.
>
> Right. Thank you for looking into this. I totally agree with the
> direction of the patch.
>
> >
> > - The addition or removal of any symbol affects the ABI and is
> > clearly a leaf change.
>
> Agreed, these ought to added.
>
> > - A change to a variable's declaration may be local rather than
> > caused by a type change elsewhere.
>
> This is correct. We really need to be aware of the fact of there can be
> two kinds of changes on a variable: local or indirect changes.
>
> A local change would be a change that affects the variable directly,
> like its type was changed from char to int.
>
> An indirect change would be, for instance, a change in a sub-type of the
> type of the variable. For instance, the variable is of type "struct
> foo*" and there was a change in the type of one of the data members of
> struct foo.
>
> In the leaf change reporter, we want to report local (leaf) changes
> only.
>
> And I think the patch misses that point somewhat because it show *all*
> changes to variables, including changes that are not local:
>
> [...]
>
> > diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
> > index 4b85d816..878e4125 100644
> > --- a/src/abg-leaf-reporter.cc
> > +++ b/src/abg-leaf-reporter.cc
> > @@ -1034,16 +1034,15 @@ leaf_reporter::report(const corpus_diff& d,
> > if (!d.has_changes())
> > return;
>
> [...]
>
>
> > + if (ctxt->show_changed_vars())
> > + {
> > + size_t num_changed =
> > + s.num_vars_changed() - s.num_changed_vars_filtered_out();
>
> Here, I think we should rather do:
>
> size_t num_changed = s.net_num_leaf_var_changes()
>
> For the record, this is what the doc of that function says:
>
> /// Getter for the net number of leaf variable change diff nodes.
> ///
> /// This the difference between the number of leaf variable change
> /// diff nodes and the number of filtered out leaf variable change
> /// diff nodes.
> ///
> /// @return the net number of leaf variable change diff nodes.
> size_t
> corpus_diff::diff_stats::net_num_leaf_var_changes() const
>
> Then ...
>
> > + if (num_changed == 1)
> > + out << indent << "1 Changed variable:\n\n";
> > + else if (num_changed > 1)
> > + out << indent << num_changed
> > + << " Changed variables:\n\n";
> > + string n1, n2;
> > + bool emitted = false;
> > + for (var_diff_sptrs_type::const_iterator i =
> > + d.priv_->sorted_changed_vars_.begin();
> > + i != d.priv_->sorted_changed_vars_.end();
> > + ++i)
> > + {
> > + diff_sptr diff = *i;
> > +
> > + if (!diff)
> > + continue;
> > +
> > + if (!diff->to_be_reported())
> > + continue;
>
> ... here, I think you need to rather do:
>
> if (diff_to_be_reported(diff.get()))
> continue;
>
> That is, use the member function leaf_reporter::diff_to_be_reported(),
> rather than diff::to_be_reported().
>
> The former function tells you if the diff is to be reported by the leaf
> reporter (as opposed to by another reporter). That is, if the diff
> carries a change that has not been filtered out and if it carries a
> local change.
>
> > +
> > + n1 = diff->first_subject()->get_pretty_representation();
> > + n2 = diff->second_subject()->get_pretty_representation();
> > +
> > + out << indent << " [C] '" << n1 << "' was changed";
> > + if (n1 != n2)
> > + out << " to '" << n2 << "'";
>
> If leaf_reporter::diff_to_be_reported() returns true, then you know the
> diff carries a local change. So you don't need to test if the pretty
> representation of the variable has changed; note that testing if the
> pretty representation of the variable has changed is a poor man's (and
> not always precise) way of testing if the variable has a local change.
> Doing that became unnecessary once we've acquired the capability of
> detecting local versus non-local changes in libabigail. So we ought to
> use that capability now.
>
> I hope this makes sense. If not, please do not hesitate to tell me what
> I am missing.
Thank you for the clear explanation.
I'll adjust the test case code to cover both possibilities and see if
I can get the right output for both.
> Thanks again for looking into this!
>
> Cheers,
>
> --
> Dodji
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2 v2] [abidiff] Add more leaf change reporting.
2020-03-13 14:00 ` Giuliano Procida
@ 2020-03-13 15:34 ` Giuliano Procida
2020-03-13 17:02 ` Dodji Seketeli
0 siblings, 1 reply; 9+ messages in thread
From: Giuliano Procida @ 2020-03-13 15:34 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida
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.
* 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>
---
src/abg-comparison.cc | 2 +-
src/abg-default-reporter.cc | 12 +-
src/abg-leaf-reporter.cc | 327 ++++++++++++++++--
tests/data/Makefile.am | 5 +
.../test-abidiff-exit/test-leaf0-report.txt | 9 +
.../test-abidiff-exit/test-leaf1-report.txt | 46 +++
tests/data/test-abidiff-exit/test-leaf1-v0.cc | 16 +
tests/data/test-abidiff-exit/test-leaf1-v0.o | Bin 0 -> 3792 bytes
tests/data/test-abidiff-exit/test-leaf1-v1.cc | 16 +
tests/data/test-abidiff-exit/test-leaf1-v1.o | Bin 0 -> 3808 bytes
tests/test-abidiff-exit.cc | 11 +
11 files changed, 406 insertions(+), 38 deletions(-)
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-report.txt
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v0.cc
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v0.o
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v1.cc
create mode 100644 tests/data/test-abidiff-exit/test-leaf1-v1.o
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index b99cfa7d..6539ed7a 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -9841,7 +9841,7 @@ corpus_diff::priv::emit_diff_stats(const diff_stats& s,
s.net_num_leaf_func_changes() +
s.net_num_vars_removed() +
s.net_num_vars_added() +
- s.net_num_vars_changed() +
+ s.net_num_leaf_var_changes() +
s.net_num_leaf_type_changes();
if (!sonames_equal_)
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index 2ac93d41..3bfdfbc4 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -1761,18 +1761,12 @@ void
default_reporter::report(const corpus_diff& d, ostream& out,
const string& indent) const
{
- size_t total = 0;
const corpus_diff::diff_stats &s =
const_cast<corpus_diff&>(d).
apply_filters_and_suppressions_before_reporting();
const diff_context_sptr& ctxt = d.context();
- /// Report removed/added/changed functions.
- total = s.net_num_func_removed() + s.net_num_func_added() +
- s.net_num_func_changed();
- const unsigned large_num = 100;
-
d.priv_->emit_diff_stats(s, out, indent);
if (ctxt->show_stats_only())
return;
@@ -1790,6 +1784,11 @@ default_reporter::report(const corpus_diff& d, ostream& out,
<< d.first_corpus()->get_architecture_name() << "' to '"
<< d.second_corpus()->get_architecture_name() << "'\n\n";
+ /// Report removed/added/changed functions.
+ size_t total = s.net_num_func_removed() + s.net_num_func_added() +
+ s.net_num_func_changed();
+ const unsigned large_num = 100;
+
if (ctxt->show_deleted_fns())
{
if (s.net_num_func_removed() == 1)
@@ -2249,7 +2248,6 @@ default_reporter::report(const corpus_diff& d, ostream& out,
// Report added/removed/changed types not reacheable from public
// interfaces.
-
maybe_report_unreachable_type_changes(d, s, indent, out);
d.priv_->maybe_dump_diff_tree();
diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
index 4b85d816..cccc1225 100644
--- a/src/abg-leaf-reporter.cc
+++ b/src/abg-leaf-reporter.cc
@@ -1034,16 +1034,15 @@ leaf_reporter::report(const corpus_diff& d,
if (!d.has_changes())
return;
- const diff_context_sptr& ctxt = d.context();
-
const corpus_diff::diff_stats &s =
const_cast<corpus_diff&>(d).
apply_filters_and_suppressions_before_reporting();
+ const diff_context_sptr& ctxt = d.context();
+
d.priv_->emit_diff_stats(s, out, indent);
if (ctxt->show_stats_only())
return;
-
out << "\n";
if (ctxt->show_soname_change()
@@ -1058,6 +1057,7 @@ leaf_reporter::report(const corpus_diff& d,
<< d.first_corpus()->get_architecture_name() << "' to '"
<< d.second_corpus()->get_architecture_name() << "'\n\n";
+ /// Report removed/added/changed functions.
if (ctxt->show_deleted_fns())
{
if (s.net_num_func_removed() == 1)
@@ -1104,6 +1104,55 @@ leaf_reporter::report(const corpus_diff& d,
out << "\n";
}
+ if (ctxt->show_added_fns())
+ {
+ if (s.net_num_func_added() == 1)
+ out << indent << "1 Added function:\n\n";
+ else if (s.net_num_func_added() > 1)
+ out << indent << s.net_num_func_added()
+ << " Added functions:\n\n";
+ bool emitted = false;
+ vector<function_decl*> sorted_added_fns;
+ sort_string_function_ptr_map(d.priv_->added_fns_, sorted_added_fns);
+ for (vector<function_decl*>::const_iterator i = sorted_added_fns.begin();
+ i != sorted_added_fns.end();
+ ++i)
+ {
+ if (d.priv_->added_function_is_suppressed(*i))
+ continue;
+
+ out
+ << indent
+ << " ";
+ out << "[A] ";
+ out << "'"
+ << (*i)->get_pretty_representation()
+ << "'";
+ if (ctxt->show_linkage_names())
+ {
+ out << " {";
+ show_linkage_name_and_aliases
+ (out, "", *(*i)->get_symbol(),
+ d.second_corpus()->get_fun_symbol_map());
+ out << "}";
+ }
+ out << "\n";
+ if (is_member_function(*i) && get_member_function_is_virtual(*i))
+ {
+ class_decl_sptr c =
+ is_class_type(is_method_type((*i)->get_type())->get_class_type());
+ out << indent
+ << " "
+ << "note that this adds a new entry to the vtable of "
+ << c->get_pretty_representation()
+ << "\n";
+ }
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
if (ctxt->show_changed_fns())
{
// Show changed functions.
@@ -1174,55 +1223,273 @@ leaf_reporter::report(const corpus_diff& d,
out << "\n";
}
- if (ctxt->show_added_fns())
+ // Report removed/added/changed variables.
+ if (ctxt->show_deleted_vars())
{
- if (s.net_num_func_added() == 1)
- out << indent << "1 Added function:\n\n";
- else if (s.net_num_func_added() > 1)
- out << indent << s.net_num_func_added()
- << " Added functions:\n\n";
+ if (s.net_num_vars_removed() == 1)
+ out << indent << "1 Removed variable:\n\n";
+ else if (s.net_num_vars_removed() > 1)
+ out << indent << s.net_num_vars_removed()
+ << " Removed variables:\n\n";
+ string n;
bool emitted = false;
- vector<function_decl*> sorted_added_fns;
- sort_string_function_ptr_map(d.priv_->added_fns_, sorted_added_fns);
- for (vector<function_decl*>::const_iterator i = sorted_added_fns.begin();
- i != sorted_added_fns.end();
+ vector<var_decl*> sorted_deleted_vars;
+ sort_string_var_ptr_map(d.priv_->deleted_vars_, sorted_deleted_vars);
+ for (vector<var_decl*>::const_iterator i =
+ sorted_deleted_vars.begin();
+ i != sorted_deleted_vars.end();
++i)
{
- if (d.priv_->added_function_is_suppressed(*i))
+ if (d.priv_->deleted_variable_is_suppressed(*i))
continue;
- out
- << indent
- << " ";
- out << "[A] ";
+ n = (*i)->get_pretty_representation();
+
+ out << indent
+ << " ";
+ out << "[D] ";
out << "'"
- << (*i)->get_pretty_representation()
+ << n
<< "'";
if (ctxt->show_linkage_names())
{
out << " {";
- show_linkage_name_and_aliases
- (out, "", *(*i)->get_symbol(),
- d.second_corpus()->get_fun_symbol_map());
+ show_linkage_name_and_aliases(out, "", *(*i)->get_symbol(),
+ d.first_corpus()->get_var_symbol_map());
out << "}";
}
out << "\n";
- if (is_member_function(*i) && get_member_function_is_virtual(*i))
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ if (ctxt->show_added_vars())
+ {
+ if (s.net_num_vars_added() == 1)
+ out << indent << "1 Added variable:\n\n";
+ else if (s.net_num_vars_added() > 1)
+ out << indent << s.net_num_vars_added()
+ << " Added variables:\n\n";
+ string n;
+ bool emitted = false;
+ vector<var_decl*> sorted_added_vars;
+ sort_string_var_ptr_map(d.priv_->added_vars_, sorted_added_vars);
+ for (vector<var_decl*>::const_iterator i =
+ sorted_added_vars.begin();
+ i != sorted_added_vars.end();
+ ++i)
+ {
+ if (d.priv_->added_variable_is_suppressed(*i))
+ continue;
+
+ n = (*i)->get_pretty_representation();
+
+ out << indent
+ << " ";
+ out << "[A] ";
+ out << "'" << n << "'";
+ if (ctxt->show_linkage_names())
{
- class_decl_sptr c =
- is_class_type(is_method_type((*i)->get_type())->get_class_type());
- out << indent
- << " "
- << "note that this adds a new entry to the vtable of "
- << c->get_pretty_representation()
- << "\n";
+ out << " {";
+ show_linkage_name_and_aliases(out, "", *(*i)->get_symbol(),
+ d.second_corpus()->get_var_symbol_map());
+ out << "}";
}
+ out << "\n";
emitted = true;
}
if (emitted)
out << "\n";
}
+ if (ctxt->show_changed_vars())
+ {
+ size_t num_changed = s.net_num_leaf_var_changes();
+ if (num_changed == 1)
+ out << indent << "1 Changed variable:\n\n";
+ else if (num_changed > 1)
+ out << indent << num_changed
+ << " Changed variables:\n\n";
+ string n1, n2;
+ bool emitted = false;
+ for (var_diff_sptrs_type::const_iterator i =
+ d.priv_->sorted_changed_vars_.begin();
+ i != d.priv_->sorted_changed_vars_.end();
+ ++i)
+ {
+ diff_sptr diff = *i;
+
+ if (!diff)
+ continue;
+
+ if (!diff_to_be_reported(diff.get()))
+ continue;
+
+ n1 = diff->first_subject()->get_pretty_representation();
+ n2 = diff->second_subject()->get_pretty_representation();
+
+ out << indent << " [C] '" << n1 << "' was changed";
+ if (n1 != n2)
+ out << " to '" << n2 << "'";
+ report_loc_info(diff->second_subject(), *ctxt, out);
+ out << ":\n";
+ diff->report(out, indent + " ");
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report removed function symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && d.priv_->deleted_unrefed_fn_syms_.size())
+ {
+ if (s.net_num_removed_func_syms() == 1)
+ out << indent
+ << "1 Removed function symbol not referenced by debug info:\n\n";
+ else if (s.net_num_removed_func_syms() > 0)
+ out << indent
+ << s.net_num_removed_func_syms()
+ << " Removed function symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_deleted_unrefed_fn_syms;
+ sort_string_elf_symbol_map(d.priv_->deleted_unrefed_fn_syms_,
+ sorted_deleted_unrefed_fn_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_deleted_unrefed_fn_syms.begin();
+ i != sorted_deleted_unrefed_fn_syms.end();
+ ++i)
+ {
+ if (d.priv_->deleted_unrefed_fn_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[D] ";
+
+ show_linkage_name_and_aliases(out, "", **i,
+ d.first_corpus()->get_fun_symbol_map());
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report added function symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && ctxt->show_added_symbols_unreferenced_by_debug_info()
+ && d.priv_->added_unrefed_fn_syms_.size())
+ {
+ if (s.net_num_added_func_syms() == 1)
+ out << indent
+ << "1 Added function symbol not referenced by debug info:\n\n";
+ else if (s.net_num_added_func_syms() > 0)
+ out << indent
+ << s.net_num_added_func_syms()
+ << " Added function symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_added_unrefed_fn_syms;
+ sort_string_elf_symbol_map(d.priv_->added_unrefed_fn_syms_,
+ sorted_added_unrefed_fn_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_added_unrefed_fn_syms.begin();
+ i != sorted_added_unrefed_fn_syms.end();
+ ++i)
+ {
+ if (d.priv_->added_unrefed_fn_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[A] ";
+ show_linkage_name_and_aliases(out, "",
+ **i,
+ d.second_corpus()->get_fun_symbol_map());
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report removed variable symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && d.priv_->deleted_unrefed_var_syms_.size())
+ {
+ if (s.net_num_removed_var_syms() == 1)
+ out << indent
+ << "1 Removed variable symbol not referenced by debug info:\n\n";
+ else if (s.net_num_removed_var_syms() > 0)
+ out << indent
+ << s.net_num_removed_var_syms()
+ << " Removed variable symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_deleted_unrefed_var_syms;
+ sort_string_elf_symbol_map(d.priv_->deleted_unrefed_var_syms_,
+ sorted_deleted_unrefed_var_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_deleted_unrefed_var_syms.begin();
+ i != sorted_deleted_unrefed_var_syms.end();
+ ++i)
+ {
+ if (d.priv_->deleted_unrefed_var_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[D] ";
+
+ show_linkage_name_and_aliases
+ (out, "", **i,
+ d.first_corpus()->get_fun_symbol_map());
+
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
+ // Report added variable symbols not referenced by any debug info.
+ if (ctxt->show_symbols_unreferenced_by_debug_info()
+ && ctxt->show_added_symbols_unreferenced_by_debug_info()
+ && d.priv_->added_unrefed_var_syms_.size())
+ {
+ if (s.net_num_added_var_syms() == 1)
+ out << indent
+ << "1 Added variable symbol not referenced by debug info:\n\n";
+ else if (s.net_num_added_var_syms() > 0)
+ out << indent
+ << s.net_num_added_var_syms()
+ << " Added variable symbols not referenced by debug info:\n\n";
+
+ bool emitted = false;
+ vector<elf_symbol_sptr> sorted_added_unrefed_var_syms;
+ sort_string_elf_symbol_map(d.priv_->added_unrefed_var_syms_,
+ sorted_added_unrefed_var_syms);
+ for (vector<elf_symbol_sptr>::const_iterator i =
+ sorted_added_unrefed_var_syms.begin();
+ i != sorted_added_unrefed_var_syms.end();
+ ++i)
+ {
+ if (d.priv_->added_unrefed_var_sym_is_suppressed((*i).get()))
+ continue;
+
+ out << indent << " ";
+ out << "[A] ";
+ show_linkage_name_and_aliases(out, "", **i,
+ d.second_corpus()->get_fun_symbol_map());
+ out << "\n";
+ emitted = true;
+ }
+ if (emitted)
+ out << "\n";
+ }
+
// Now show the changed types.
const diff_maps& leaf_diffs = d.get_leaf_diffs();
report_type_changes_from_diff_maps(*this, leaf_diffs, out, indent);
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 07077608..5dab8685 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -115,6 +115,11 @@ test-abidiff-exit/test-leaf0-v0.o \
test-abidiff-exit/test-leaf0-v1.cc \
test-abidiff-exit/test-leaf0-v1.o \
test-abidiff-exit/test-leaf0-report.txt \
+test-abidiff-exit/test-leaf1-v0.cc \
+test-abidiff-exit/test-leaf1-v0.o \
+test-abidiff-exit/test-leaf1-v1.cc \
+test-abidiff-exit/test-leaf1-v1.o \
+test-abidiff-exit/test-leaf1-report.txt \
\
test-diff-dwarf/test0-v0.cc \
test-diff-dwarf/test0-v0.o \
diff --git a/tests/data/test-abidiff-exit/test-leaf0-report.txt b/tests/data/test-abidiff-exit/test-leaf0-report.txt
index f823789d..7d15e28f 100644
--- a/tests/data/test-abidiff-exit/test-leaf0-report.txt
+++ b/tests/data/test-abidiff-exit/test-leaf0-report.txt
@@ -11,3 +11,12 @@ Removed/Changed/Added variables summary: 0 Removed, 1 Changed, 0 Added variable
type size changed from 32 to 64 (in bits)
+1 Changed variable:
+
+ [C] 'int changed_var' was changed to 'long int changed_var':
+ size of symbol changed from 4 to 8
+ type of variable changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+
+
diff --git a/tests/data/test-abidiff-exit/test-leaf1-report.txt b/tests/data/test-abidiff-exit/test-leaf1-report.txt
new file mode 100644
index 00000000..b51bb4b3
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-leaf1-report.txt
@@ -0,0 +1,46 @@
+Leaf changes summary: 7 artifacts changed
+Changed leaf types summary: 1 leaf type changed
+Removed/Changed/Added functions summary: 1 Removed, 1 Changed, 1 Added function
+Removed/Changed/Added variables summary: 1 Removed, 1 Changed, 1 Added variable
+
+1 Removed function:
+
+ [D] 'function int deleted_fun()' {_Z11deleted_funv}
+
+1 Added function:
+
+ [A] 'function long int added_fun()' {_Z9added_funv}
+
+1 function with some sub-type change:
+
+ [C] 'function int directly_changed_fun()' has some sub-type changes:
+ return type changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+
+
+1 Removed variable:
+
+ [D] 'int deleted_var' {deleted_var}
+
+1 Added variable:
+
+ [A] 'long int added_var' {added_var}
+
+1 Changed variable:
+
+ [C] 'int directly_changed_var' was changed to 'long int directly_changed_var':
+ size of symbol changed from 4 to 8
+ type of variable changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+
+
+'struct changed' changed:
+ type size changed from 32 to 64 (in bits)
+ there are data member changes:
+ type 'int' of 'changed::foo' changed:
+ type name changed from 'int' to 'long int'
+ type size changed from 32 to 64 (in bits)
+ and size changed from 32 to 64 (in bits) (by +32 bits)
+
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v0.cc b/tests/data/test-abidiff-exit/test-leaf1-v0.cc
new file mode 100644
index 00000000..bb6505c8
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-leaf1-v0.cc
@@ -0,0 +1,16 @@
+struct changed {
+ int foo = 0;
+};
+
+int deleted_var = 0;
+int directly_changed_var = 0;
+changed * indirectly_changed_var;
+
+int deleted_fun() {
+ return 0;
+}
+int directly_changed_fun() {
+ return 0;
+}
+void indirectly_changed_fun(changed * x) {
+}
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v0.o b/tests/data/test-abidiff-exit/test-leaf1-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..a8734481238bda41217440b305307ea7c5464782
GIT binary patch
literal 3792
zcmbtW&2Jl35TEtNu~R3G6Z6p|6><?Ml(3sPgtRH7H3ZUxA{3BvXhr0%?R9KnuOoXC
z96rPWgp^w=ApsH+he-Sl+<PE5Zoof)kht{*LVV2Z&e$){n{r?z@67z>^Sw9w)*rw2
z#(K;!K*)e=(ANY7$eioTg+MGo7A9eP<LH;~NZ+}yi5o{B{eE}ne&7p`#H2%kOp?fK
zBz94eV<Zn5MsfqWS#7nzh$ToybSPf;e29~5#Nz~xn4}Pe=x`*ONZmt^zVC3-LOnrY
z#`rKXp%e?o*WaU*?4o^^Vq^DeFhVwCG$<4{L;yfPCiED2hzA%G#?$F+%1qCsCydEN
z=5frOHOzD>bv5;3Y9(c4an)mBCPQ^Idj-=EJq4-MwUn8}WiEl47Tfiy*m;2AakOq?
zT*5Z4Pl?qF@)yp3@yZnCOvaU*6g6Ul>b#kyb^>=?(sTF=4g#>GLh3;&Fl?h@!Yn=w
z<InUMK7+Bs(>B0&JHFL$ooe1XSg_0GATFMTk+1-&)A4gR<HoJ{g?Sd9osS!z#7s<6
zyuG+kskhy--#9FmYmT?;R*Kbq?*NLo^ZAO~aD4#=>I}pkIBnn@Q1>FuN=6uPz`Ahb
z&F$RU++2Psw_-2a`P||{ekGq@Sj<^Xf8TRgcU{kI*UP9n?Q(6kdu7>LUdma!Ijib)
z4!v@%-SS%d9c$lv4_mN&r_^veQQc56T>WB1qiEW?EDM?Un|mN_(+c6n+S=9JGq1X(
zy5ogiTKUVDw0`Ln9tn;D&*=TP!RTg;nbAb@E}q#1#OX&A4EM|F%xAH+vEg-eK#XiJ
zkrC-E`#YgI($1y<hNbbIc<|4F0RbF_C>ZhH7@e}4Xo(_|ZzA1h!l`?pXEwuYqNj^s
zhS!0pJ#O}{gB}zJCXAg1STiAg20l7~Pa-m*tSh`z0dkCsF8m3`<sBCO0^*a(e#!;5
z&ba8p?=b!taw5S8Uu|^`BL<hobKt#w*em|u<4^s_$<b#@WWr?!38$->E^?xYkb&TH
ztUs#ZPx8o3F;1#FZI={(*;|5a1*f`ivA(6@vz&j2aZ=SOscQc6&UF-jb$UM3{BN;-
zr1+1sOzcaRWgmWJ{TmJcoAqPnvJWy3Cz`+X@q>o53O{LiWS)N0{P$RYpyAW3|G`}P
zVYN2{w65S=%!bSv{>x|JImYSC)pIXsxb%NV!)1TK?i@Dprbq6#LtYcfce_5=6~}kL
zE_FI!x7~(gSKQM6Zn5rFTWAYpr&MaY2dB(Vn?8r9LmG9@l@KfecDdDTy7;Hb_FBGc
z<JZgT_)hs<PUO~#)wa`gx$*zWW-y-4LVX7*Wq}?@7em&2lCMf+CrrP&9o0#Z^?3UK
zLGPg+U*i}#e@UUfvxMdGr{^SG;zu>3rUw6-=<Y30kU}TCAsHSb>Vy2GKK>gEXceD&
zPBQomej9l8AG+@rnski3r@8JzKQxeZ-<ya=_5U+BD)H2R0)Kk{6~y$M2*K-IU+yEq
zh;-i~Vo~*zbcrI#K2d#w^!j^<QBLU-#ej~v{_|QvQH6hi-cj*C@<7pisU&g2KSiU+
zs^4GwK(bNy{fgtc-F^z%6r^s*dQVxu0H31}wSPI@zYTVz{iE-O#LK=?KLfQ9>vH`Q
lZkWEG^qtb<sZ7*T3q1d{KPu_#r@T?|HSYgd4Mtgy{}+ciMVbHr
literal 0
HcmV?d00001
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v1.cc b/tests/data/test-abidiff-exit/test-leaf1-v1.cc
new file mode 100644
index 00000000..5df5df9d
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-leaf1-v1.cc
@@ -0,0 +1,16 @@
+struct changed {
+ long foo = 0;
+};
+
+long directly_changed_var = 0;
+changed * indirectly_changed_var;
+long added_var = 0;
+
+long directly_changed_fun() {
+ return 0;
+}
+void indirectly_changed_fun(changed * x) {
+}
+long added_fun() {
+ return 0;
+}
diff --git a/tests/data/test-abidiff-exit/test-leaf1-v1.o b/tests/data/test-abidiff-exit/test-leaf1-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..324180ca92b91045edb4c9d3b9fef1fd314a09ba
GIT binary patch
literal 3808
zcmbtW&2Jl35TEC*zv{$IVm|UgC0BsbQoM~rNSi`hL!eD4pn{Y`D=M?L*Rh4YF80Pa
zAXEtus3(eq1V~66D#5>iIDrrne*zbdNWiTU2M~ucvpeHFKW`8cBkj)2Z$97KdAob#
z+2=3Blma0I=U`tG6d-?iUoHh=334z4+nYCk`I_`=-;=m`^X)%xg(9PYFF*>D9tvcV
zL}nwgW11WzIi*x;6S-CIbU?)d8RJrMG6pvlPmn{}B7`VJ6PdSA-bWLu!qAp9B9auQ
zvI%08ODMyF`aDIFng~3`t-m3Z9-w`ZYQ{clK$tQ8FBw5itB13>jFnx;POF*Z)cqKl
zQ&u*UIh%PRvzAdgT=gVasZia@oxyevJp`G|ISj&Ou7H&l+xfZJEI{H2TDLGRX~X$B
zv3l99991aIe)9Aj?x?MF+t5Pw(s8XGWrRm*L3m+YaF?6@padhnjh6+BcvtH0>?GcU
zvBIaVz;}Coq2)UDVxeEOt5x!c_)$oQ1yGrfAHEb<SK^PHimP|yGwY!mi(#7b)#asH
zv+Gv<)?T^VaJ(J2R<7@QeQ0&OoqW^t$McW*_nj_qEHu4Hvs0^uiDAr))!^cb+xd-?
zCyOijHGA1E=9ia>YsKQya=y^^cRhD~$MxK9vx=(Itv1#NXI2ZVEBV4szEF31dtSBC
z?RcHtUSZdJ0~;v#PNn7cqI#iXxaQ@EHqmtXv@GM4-@XoE)3mzs)wSUY`T!R<HqPdc
zKjT)Kju&=ZD4srH^bT-O@F?&Zz4<bz!IWB<NTzP#GkXAWx`=|q@Kko{{n*B2;sQD#
zMz$x(h;)$smCzh%XVVzN()gaZb9c;ufED>>B78VTBViU=qR8Smk!iDF<dhH1R(Pm~
zx(F7eMI7N)xZ)9v6bmNDun@;X9YGdMjG^?rrnPm+*_>fqbm1o$pW;k{KZg2@wx9CC
zdx3G$g}=`DeVj+|@uL?pwltplpdNId|I5@HV=hmOK2IW3i?VZs(^L9@K&0uAf#46a
zey@Sg(jX$?Ycn8H^(mu$C89jCr=;6-Lz4Vkp$eCn3qH^KWdmPf{Tg$st4~ec@PCQ*
zp60Jl(>sR$QC`=Fn*R}&iG9kl^x0tj3j<%|JYO*``y=!5jp6?@>)#ppAFThtT=K|#
z{ciYoS-)f8tioR`NFKpuHfUYJZvq?ST$ui=2js~!PP3rTL&?CU|5puM_6O|VUK_sw
z$o+1}8zTAczz4hL_zu{WUJvZ9+j8uhTiM+yH@$iXZGr4mDqXifVs^Upc^nOCH9c2C
zumsrEPP^^mSJU=7zH8%mt<dwG>T8_HZItU>r|oj%|C23XJngOQzoaX%a7+N^<f{|e
z2{Yg1j_#z%W<33m(D^juWv=N@M<@L(6JEs2%t^S!PZ&l+4gR9&zqdp|8hy(fa-4^V
z=8*iPzVx+@n3<E(_zZqYibkd{y<bd>+~>LOk`_y%=}YgesQ!QDMkSv5PvT|vUyI0z
z5Io2AuaKgVS-*_7sQR<?Qb&?~qWT1x^{*pFIi*h&1G>%iA2$k$Dx7}nqvC(!fui}+
zN#caRhenZgzq{r@veESYjN`f8{S>q*NQ00K@3MXYK1LyG|CV_FHrbK(kG>lcFZ)XU
tjMYYL!1aIRhUvRX-zhVm%0w--#Pfdx?K+w3r@T?|1MdH}9*nXX{||`LLr4Gs
literal 0
HcmV?d00001
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 5372b3fe..7a334227 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -129,6 +129,17 @@ InOutSpec in_out_specs[] =
"data/test-abidiff-exit/test-leaf0-report.txt",
"output/test-abidiff-exit/test-leaf0-report.txt"
},
+ {
+ "data/test-abidiff-exit/test-leaf1-v0.o",
+ "data/test-abidiff-exit/test-leaf1-v1.o",
+ "",
+ // --redundant - pending a bug fix
+ "--no-show-locs --leaf-changes-only --redundant",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE
+ | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
+ "data/test-abidiff-exit/test-leaf1-report.txt",
+ "output/test-abidiff-exit/test-leaf1-report.txt"
+ },
{0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
};
--
2.25.1.481.gfbce0eb801-goog
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] [abidiff] Add more leaf change reporting.
2020-03-13 15:34 ` [PATCH 2/2 v2] " Giuliano Procida
@ 2020-03-13 17:02 ` Dodji Seketeli
2020-03-13 17:22 ` Giuliano Procida
0 siblings, 1 reply; 9+ messages in thread
From: Dodji Seketeli @ 2020-03-13 17:02 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team
Giuliano Procida <gprocida@google.com> a écrit:
> 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.
>
> * 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.
This looks OK to me. I have just updated the manual of the abi{pkg}diff
tools to say that --leaf-changes-only implies --redudant, just as you
did in the help string of these tools.
The patch was applied to master.
Thanks!
--
Dodji
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] [abidiff] Add more leaf change reporting.
2020-03-13 17:02 ` Dodji Seketeli
@ 2020-03-13 17:22 ` Giuliano Procida
2020-03-13 17:26 ` Giuliano Procida
0 siblings, 1 reply; 9+ messages in thread
From: Giuliano Procida @ 2020-03-13 17:22 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team
I was confused by your comment. Then I looked at the patch.
The usage string change was supposed to be in "Fix interaction of
--redundant and --leaf-changes-only options." not this patch.
Sorry about this. Looks like it slipped through during a rebase to
reorder changes.
On Fri, 13 Mar 2020 at 17:02, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > 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.
> >
> > * 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.
>
> This looks OK to me. I have just updated the manual of the abi{pkg}diff
> tools to say that --leaf-changes-only implies --redudant, just as you
> did in the help string of these tools.
>
> The patch was applied to master.
>
> Thanks!
>
> --
> Dodji
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] [abidiff] Add more leaf change reporting.
2020-03-13 17:22 ` Giuliano Procida
@ 2020-03-13 17:26 ` Giuliano Procida
0 siblings, 0 replies; 9+ messages in thread
From: Giuliano Procida @ 2020-03-13 17:26 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team
Actually, my mistake. You attached that doc change to the wrong patch.
No harm done.
On Fri, 13 Mar 2020 at 17:22, Giuliano Procida <gprocida@google.com> wrote:
>
> I was confused by your comment. Then I looked at the patch.
>
> The usage string change was supposed to be in "Fix interaction of
> --redundant and --leaf-changes-only options." not this patch.
>
> Sorry about this. Looks like it slipped through during a rebase to
> reorder changes.
>
> On Fri, 13 Mar 2020 at 17:02, Dodji Seketeli <dodji@seketeli.org> wrote:
> >
> > Giuliano Procida <gprocida@google.com> a écrit:
> >
> > > 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.
> > >
> > > * 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.
> >
> > This looks OK to me. I have just updated the manual of the abi{pkg}diff
> > tools to say that --leaf-changes-only implies --redudant, just as you
> > did in the help string of these tools.
> >
> > The patch was applied to master.
> >
> > Thanks!
> >
> > --
> > Dodji
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-13 17:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 10:35 [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections Giuliano Procida
2020-03-11 10:35 ` [PATCH 2/2] [abidiff] Add more leaf change reporting Giuliano Procida
2020-03-13 10:28 ` Dodji Seketeli
2020-03-13 14:00 ` Giuliano Procida
2020-03-13 15:34 ` [PATCH 2/2 v2] " Giuliano Procida
2020-03-13 17:02 ` Dodji Seketeli
2020-03-13 17:22 ` Giuliano Procida
2020-03-13 17:26 ` Giuliano Procida
2020-03-12 13:03 ` [PATCH 1/2] [abidiff] Fix spurious new lines after diff sections Dodji Seketeli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).