* [PATCH 1/3] abidiff.cc: tidy using directives and declarations
2020-05-04 16:24 [PATCH 0/3] Add an option to give finer-grained control of offset reporting Giuliano Procida
@ 2020-05-04 16:24 ` Giuliano Procida
2020-05-13 11:27 ` Dodji Seketeli
2020-05-04 16:24 ` [PATCH 2/3] Allow offset changes to be considered harmless Giuliano Procida
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Giuliano Procida @ 2020-05-04 16:24 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
This patch replaces a using directive with a couple of specific using
declarations, moves a couple of globally-scoped using declarations to
the top of the file and alphabetically sorts all the using
declarations.
* tools/abidiff.cc: Eliminate using directive. Move stray
using declarations to top of file and sort all using
declarations.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
tools/abidiff.cc | 44 ++++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/tools/abidiff.cc b/tools/abidiff.cc
index 162d5ebc..cffe5ab5 100644
--- a/tools/abidiff.cc
+++ b/tools/abidiff.cc
@@ -33,36 +33,39 @@
#include "abg-reader.h"
#include "abg-dwarf-reader.h"
-using std::vector;
-using std::string;
-using std::ostream;
-using std::cout;
-using std::cerr;
using abg_compat::shared_ptr;
-using abigail::ir::environment;
-using abigail::ir::environment_sptr;
-using abigail::translation_unit;
-using abigail::translation_unit_sptr;
-using abigail::corpus_sptr;
-using abigail::corpus_group_sptr;
-using abigail::comparison::translation_unit_diff_sptr;
+using abigail::comparison::compute_diff;
using abigail::comparison::corpus_diff;
using abigail::comparison::corpus_diff_sptr;
-using abigail::comparison::compute_diff;
-using abigail::comparison::get_default_harmless_categories_bitmap;
+using abigail::comparison::diff_context;
+using abigail::comparison::diff_context_sptr;
using abigail::comparison::get_default_harmful_categories_bitmap;
+using abigail::comparison::get_default_harmless_categories_bitmap;
+using abigail::comparison::translation_unit_diff_sptr;
+using abigail::corpus_group_sptr;
+using abigail::corpus_sptr;
+using abigail::dwarf_reader::STATUS_ALT_DEBUG_INFO_NOT_FOUND;
+using abigail::dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND;
+using abigail::ir::environment;
+using abigail::ir::environment_sptr;
+using abigail::suppr::read_suppressions;
using abigail::suppr::suppression_sptr;
using abigail::suppr::suppressions_type;
-using abigail::suppr::read_suppressions;
-using namespace abigail::dwarf_reader;
-using abigail::tools_utils::emit_prefix;
+using abigail::tools_utils::abidiff_status;
using abigail::tools_utils::check_file;
-using abigail::tools_utils::guess_file_type;
+using abigail::tools_utils::emit_prefix;
using abigail::tools_utils::gen_suppr_spec_from_headers;
using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelists;
+using abigail::tools_utils::guess_file_type;
using abigail::tools_utils::load_default_system_suppressions;
using abigail::tools_utils::load_default_user_suppressions;
-using abigail::tools_utils::abidiff_status;
+using abigail::translation_unit;
+using abigail::translation_unit_sptr;
+using std::cerr;
+using std::cout;
+using std::ostream;
+using std::string;
+using std::vector;
struct options
{
@@ -625,9 +628,6 @@ display_symtabs(const corpus_sptr c1, const corpus_sptr c2, ostream& o)
o << (*i)->get_pretty_representation() << std::endl;
}
-using abigail::comparison::diff_context_sptr;
-using abigail::comparison::diff_context;
-
/// Check that the suppression specification files supplied are
/// present. If not, emit an error on stderr.
///
--
2.26.2.526.g744177e7f7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/3] abidiff.cc: tidy using directives and declarations
2020-05-04 16:24 ` [PATCH 1/3] abidiff.cc: tidy using directives and declarations Giuliano Procida
@ 2020-05-13 11:27 ` Dodji Seketeli
2020-05-13 15:56 ` Giuliano Procida
0 siblings, 1 reply; 18+ messages in thread
From: Dodji Seketeli @ 2020-05-13 11:27 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich
Giuliano Procida <gprocida@google.com> a écrit:
> This patch replaces a using directive with a couple of specific using
> declarations, moves a couple of globally-scoped using declarations to
> the top of the file and alphabetically sorts all the using
> declarations.
Are these kinds of patches really necessary? I mean, I am not sure why
this kind of sorting is useful. Besides, it adds unnecessary noise to
the history, making "git blame" less useful.
Just like for the #include patch earlier, I am not for enforcing
alphabetical sorting for of the using declarations accross the project.
Or am I missing something?
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] abidiff.cc: tidy using directives and declarations
2020-05-13 11:27 ` Dodji Seketeli
@ 2020-05-13 15:56 ` Giuliano Procida
2020-05-14 8:22 ` Dodji Seketeli
0 siblings, 1 reply; 18+ messages in thread
From: Giuliano Procida @ 2020-05-13 15:56 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team, Matthias Männich
Hi.
On Wed, 13 May 2020 at 12:27, Dodji Seketeli <dodji@seketeli.org> wrote:
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > This patch replaces a using directive with a couple of specific using
> > declarations, moves a couple of globally-scoped using declarations to
> > the top of the file and alphabetically sorts all the using
> > declarations.
>
> Are these kinds of patches really necessary? I mean, I am not sure why
> this kind of sorting is useful. Besides, it adds unnecessary noise to
> the history, making "git blame" less useful.
>
> Just like for the #include patch earlier, I am not for enforcing
> alphabetical sorting for of the using declarations accross the project.
>
> Or am I missing something?
>
Thanks for calling this out, it deserves some discussion. I should have put
more justification into the commit message.
I think this comes from a couple of things.
Firstly, it is part of an internal coding standard that has received
thousands of hours of review and has been tested on very large scale code
bases. We probably follow parts of it by habit. Not all of it makes sense
outside of Google (where there a lot more than the Standard Library is
available and we have linting tools that help enforce compliance), but a
lot does and quite a bit will be non-controversial. The external version is
https://google.github.io/styleguide/cppguide.html and includes rationales.
Secondly, I was asked to do this in a review comment. This is probably just
the point above, by proxy, but it could be Matthias following his own
internal or some other style guide. I think the right thing to do
(initially) is to push back and say "this is not Google" in the cases where
there's no benefit.
You may want to consider adopting or adapting some of the style guide,
where you feel it would be a good match for the project, and brings some
value (such as increased readability, safety or performance).
Do let me know if I should drop this change and fix up the remaining ones.
Thanks!
Regards,
Giuliano.
> Cheers,
>
> --
> Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscribe@android.com.
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] Allow offset changes to be considered harmless.
2020-05-04 16:24 [PATCH 0/3] Add an option to give finer-grained control of offset reporting Giuliano Procida
2020-05-04 16:24 ` [PATCH 1/3] abidiff.cc: tidy using directives and declarations Giuliano Procida
@ 2020-05-04 16:24 ` Giuliano Procida
2020-05-13 11:46 ` Dodji Seketeli
2020-05-04 16:24 ` [PATCH 3/3] Add abidiff --offset-changes-are-harmless tests Giuliano Procida
2020-05-12 14:51 ` [PATCH 0/3] Add an option to give finer-grained control of offset reporting Matthias Maennich
3 siblings, 1 reply; 18+ messages in thread
From: Giuliano Procida @ 2020-05-04 16:24 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
This changes lets the user flip offset changes from harmful to
harmless. This is a simple way of shrinking very verbose diffs where
most changes within structs are offset changes caused by much rarer
member addition, removal and size changes.
* include/abg-comparison.h (diff_category): Mechanically split
SIZE_OR_OFFSET_CHANGE_CATEGORY into SIZE_CHANGE_CATEGORY and
OFFSET_CHANGE_CATEGORY.
* src/abg-comp-filter.cc (categorize_harmful_diff_node): Ditto.
* src/abg-comparison.cc
(get_default_harmless_categories_bitmap): Ditto.
(operator<<): In the diff_category overload, ditto.
* src/abg-reporter-priv.cc (represent): Split
show_size_offset_changes into show_size_changes and
show_offset_changes and split conditional to match.
(report_size_and_alignment_changes): Test SIZE_CHANGE_CATEGORY
instead of old SIZE_OR_OFFSET_CHANGE_CATEGORY. Add a TODO to
check an unexpected test.
* tools/abidiff.cc: (options): Add offset_changes_are_harmless
and plumb this through.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/abg-comparison.h | 35 +++++++++++++++++++++--------------
src/abg-comp-filter.cc | 14 +++++++++-----
src/abg-comparison.cc | 15 ++++++++++++---
src/abg-reporter-priv.cc | 17 +++++++++++------
tools/abidiff.cc | 20 +++++++++++++++++---
5 files changed, 70 insertions(+), 31 deletions(-)
diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index 4f60ff99..8df84404 100644
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -388,48 +388,54 @@ enum diff_category
PRIVATE_TYPE_CATEGORY = 1 << 9,
/// This means the diff node (or at least one of its descendant
- /// nodes) carries a change that modifies the size of a type or an
- /// offset of a type member. Removal or changes of enumerators in a
- /// enum fall in this category too.
- SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 10,
+ /// nodes) carries a change that modifies the size of a type.
+ /// Removal or changes of enumerators in a enum fall in this
+ /// category too.
+ SIZE_CHANGE_CATEGORY = 1 << 10,
+
+ /// This means the diff node (or at least one of its descendant
+ /// nodes) carries a change that modifies the offset of a type
+ /// member.
+ OFFSET_CHANGE_CATEGORY = 1 << 11,
/// This means that a diff node in the sub-tree carries an
/// incompatible change to a vtable.
- VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 11,
+ VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 12,
/// A diff node in this category is redundant. That means it's
/// present as a child of a other nodes in the diff tree.
- REDUNDANT_CATEGORY = 1 << 12,
+ REDUNDANT_CATEGORY = 1 << 13,
/// This means that a diff node in the sub-tree carries a class type
/// that was declaration-only and that is now defined, or vice
/// versa.
- CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 13,
+ CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 14,
/// A diff node in this category is a function parameter type which
/// top cv-qualifiers change.
- FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 14,
+ FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 15,
/// A diff node in this category has a function parameter type with a
/// cv-qualifiers change.
- FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 15,
+ FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 16,
/// A diff node in this category is a function return type with a
/// cv-qualifier change.
- FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 16,
+ FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 17,
/// A diff node in this category is for a variable which type holds
/// a cv-qualifier change.
- VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 17,
+ VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 18,
/// A diff node in this category carries a change from void pointer
/// to non-void pointer.
- VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 18,
+ VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 19,
/// A diff node in this category carries a change in the size of the
/// array type of a global variable, but the ELF size of the
/// variable didn't change.
- BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19,
+ BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 20,
+
/// A special enumerator that is the logical 'or' all the
/// enumerators above.
///
@@ -446,7 +452,8 @@ enum diff_category
| HARMLESS_UNION_CHANGE_CATEGORY
| SUPPRESSED_CATEGORY
| PRIVATE_TYPE_CATEGORY
- | SIZE_OR_OFFSET_CHANGE_CATEGORY
+ | SIZE_CHANGE_CATEGORY
+ | OFFSET_CHANGE_CATEGORY
| VIRTUAL_MEMBER_CHANGE_CATEGORY
| REDUNDANT_CATEGORY
| CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY
diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index 75df901c..4e66e8b3 100644
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -1585,14 +1585,18 @@ categorize_harmful_diff_node(diff *d, bool pre)
// or removal.
//
// TODO: be more specific -- not all size changes are harmful.
- if (!has_class_decl_only_def_change(d)
- && (type_size_changed(f, s)
- || data_member_offset_changed(f, s)
+ if (!has_class_decl_only_def_change(d))
+ {
+ if (type_size_changed(f, s)
|| non_static_data_member_type_size_changed(f, s)
|| non_static_data_member_added_or_removed(d)
|| base_classes_added_or_removed(d)
- || has_harmful_enum_change(d)))
- category |= SIZE_OR_OFFSET_CHANGE_CATEGORY;
+ || has_harmful_enum_change(d))
+ category |= SIZE_CHANGE_CATEGORY;
+
+ if (data_member_offset_changed(f, s))
+ category |= OFFSET_CHANGE_CATEGORY;
+ }
if (has_virtual_mem_fn_change(d))
category |= VIRTUAL_MEMBER_CHANGE_CATEGORY;
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 46bf9e30..32a2cf1b 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -2963,7 +2963,8 @@ get_default_harmless_categories_bitmap()
diff_category
get_default_harmful_categories_bitmap()
{
- return (abigail::comparison::SIZE_OR_OFFSET_CHANGE_CATEGORY
+ return (abigail::comparison::SIZE_CHANGE_CATEGORY
+ | abigail::comparison::OFFSET_CHANGE_CATEGORY
| abigail::comparison::VIRTUAL_MEMBER_CHANGE_CATEGORY);
}
@@ -3065,11 +3066,19 @@ operator<<(ostream& o, diff_category c)
emitted_a_category |= true;
}
- if (c & SIZE_OR_OFFSET_CHANGE_CATEGORY)
+ if (c & SIZE_CHANGE_CATEGORY)
{
if (emitted_a_category)
o << "|";
- o << "SIZE_OR_OFFSET_CHANGE_CATEGORY";
+ o << "SIZE_CHANGE_CATEGORY";
+ emitted_a_category |= true;
+ }
+
+ if (c & OFFSET_CHANGE_CATEGORY)
+ {
+ if (emitted_a_category)
+ o << "|";
+ o << "OFFSET_CHANGE_CATEGORY";
emitted_a_category |= true;
}
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 4ea591aa..1897b448 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -422,8 +422,10 @@ represent(const var_diff_sptr &diff,
const uint64_t n_offset = get_data_member_offset(n);
const string o_pretty_representation = o->get_pretty_representation();
// no n_pretty_representation here as it's only needed in a couple of places
- const bool show_size_offset_changes = ctxt->get_allowed_category()
- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
+ const bool show_size_changes = ctxt->get_allowed_category()
+ & SIZE_CHANGE_CATEGORY;
+ const bool show_offset_changes = ctxt->get_allowed_category()
+ & OFFSET_CHANGE_CATEGORY;
// Has the main diff text been output?
bool emitted = false;
@@ -554,7 +556,7 @@ represent(const var_diff_sptr &diff,
out << "now becomes laid out";
emitted = true;
}
- if (show_size_offset_changes)
+ if (show_offset_changes)
{
if (o_offset != n_offset)
{
@@ -577,7 +579,9 @@ represent(const var_diff_sptr &diff,
maybe_show_relative_offset_change(diff, *ctxt, out);
emitted = true;
}
-
+ }
+ if (show_size_changes)
+ {
if (!size_reported && o_size != n_size)
{
if (begin_with_and)
@@ -728,7 +732,7 @@ report_size_and_alignment_changes(type_or_decl_base_sptr first,
unsigned fdc = first_array ? first_array->get_dimension_count(): 0,
sdc = second_array ? second_array->get_dimension_count(): 0;
- if (ctxt->get_allowed_category() & SIZE_OR_OFFSET_CHANGE_CATEGORY)
+ if (ctxt->get_allowed_category() & SIZE_CHANGE_CATEGORY)
{
if (fs != ss || fdc != sdc)
{
@@ -794,12 +798,13 @@ report_size_and_alignment_changes(type_or_decl_base_sptr first,
}
} // end if (fs != ss || fdc != sdc)
else
+ // TODO: Is this the right check?
if (ctxt->show_relative_offset_changes())
{
out << indent << "type size hasn't changed\n";
}
}
- if ((ctxt->get_allowed_category() & SIZE_OR_OFFSET_CHANGE_CATEGORY)
+ if ((ctxt->get_allowed_category() & SIZE_CHANGE_CATEGORY)
&& (fa != sa))
{
out << indent;
diff --git a/tools/abidiff.cc b/tools/abidiff.cc
index cffe5ab5..0dbd05f3 100644
--- a/tools/abidiff.cc
+++ b/tools/abidiff.cc
@@ -34,9 +34,12 @@
#include "abg-dwarf-reader.h"
using abg_compat::shared_ptr;
+using abigail::comparison::NO_CHANGE_CATEGORY;
+using abigail::comparison::OFFSET_CHANGE_CATEGORY;
using abigail::comparison::compute_diff;
using abigail::comparison::corpus_diff;
using abigail::comparison::corpus_diff_sptr;
+using abigail::comparison::diff_category;
using abigail::comparison::diff_context;
using abigail::comparison::diff_context_sptr;
using abigail::comparison::get_default_harmful_categories_bitmap;
@@ -95,6 +98,7 @@ struct options
bool show_hexadecimal_values;
bool show_offsets_sizes_in_bits;
bool show_relative_offset_changes;
+ bool offset_changes_are_harmless;
bool show_stats_only;
bool show_symtabs;
bool show_deleted_fns;
@@ -136,6 +140,7 @@ struct options
show_hexadecimal_values(),
show_offsets_sizes_in_bits(true),
show_relative_offset_changes(true),
+ offset_changes_are_harmless(false),
show_stats_only(),
show_symtabs(),
show_deleted_fns(),
@@ -224,6 +229,7 @@ display_usage(const string& prog_name, ostream& out)
<< " --show-bits show size and offsets in bits\n"
<< " --show-hex show size and offset in hexadecimal\n"
<< " --show-dec show size and offset in decimal\n"
+ << " --offset-changes-are-harmless"
<< " --no-show-relative-offset-changes do not show relative"
" offset changes\n"
<< " --suppressions|--suppr <path> specify a suppression file\n"
@@ -480,6 +486,8 @@ parse_command_line(int argc, char* argv[], options& opts)
opts.show_hexadecimal_values = false;
else if (!strcmp(argv[i], "--no-show-relative-offset-changes"))
opts.show_relative_offset_changes = false;
+ else if (!strcmp(argv[i], "--offset-changes-are-harmless"))
+ opts.offset_changes_are_harmless = true;
else if (!strcmp(argv[i], "--suppressions")
|| !strcmp(argv[i], "--suppr"))
{
@@ -690,7 +698,7 @@ set_diff_context_from_opts(diff_context_sptr ctxt,
// redundancy analysis pass altogether. That could help save a
// couple of CPU cycle here and there!
ctxt->show_redundant_changes(opts.show_redundant_changes
- || opts.leaf_changes_only);
+ || opts.leaf_changes_only);
ctxt->show_symbols_unreferenced_by_debug_info
(opts.show_symbols_not_referenced_by_debug_info);
ctxt->show_added_symbols_unreferenced_by_debug_info
@@ -698,11 +706,17 @@ set_diff_context_from_opts(diff_context_sptr ctxt,
ctxt->show_unreachable_types(opts.show_all_types);
ctxt->show_impacted_interfaces(opts.show_impacted_interfaces);
+ diff_category extra_harmless = opts.offset_changes_are_harmless
+ ? OFFSET_CHANGE_CATEGORY
+ : NO_CHANGE_CATEGORY;
+
if (!opts.show_harmless_changes)
- ctxt->switch_categories_off(get_default_harmless_categories_bitmap());
+ ctxt->switch_categories_off(get_default_harmless_categories_bitmap()
+ | extra_harmless);
if (!opts.show_harmful_changes)
- ctxt->switch_categories_off(get_default_harmful_categories_bitmap());
+ ctxt->switch_categories_off(get_default_harmful_categories_bitmap()
+ &~ extra_harmless);
suppressions_type supprs;
for (vector<string>::const_iterator i = opts.suppression_paths.begin();
--
2.26.2.526.g744177e7f7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/3] Allow offset changes to be considered harmless.
2020-05-04 16:24 ` [PATCH 2/3] Allow offset changes to be considered harmless Giuliano Procida
@ 2020-05-13 11:46 ` Dodji Seketeli
2020-05-14 13:21 ` Giuliano Procida
0 siblings, 1 reply; 18+ messages in thread
From: Dodji Seketeli @ 2020-05-13 11:46 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich
Giuliano Procida <gprocida@google.com> a écrit:
> This changes lets the user flip offset changes from harmful to
> harmless. This is a simple way of shrinking very verbose diffs where
> most changes within structs are offset changes caused by much rarer
> member addition, removal and size changes.
Hmmh.
I think this is probably too simple.
Offset changes can really signal a problem on their own. So I am not
for categorizing them as harmless.
I understand that the reporting of offset changes that are a consequence
of an addition, removal or change of a data member can be verbose, but
I'd rather keep them as is for now, rather than risking some false
negative because we want to go the easy route.
In other words, if we really want to filter out those /consequential/
offset changes, then we need to properly detect them and flag them as
being filtered.
I understand that that is a bigger undertaking, but I think that would
the right way to do it.
So I'd rather not apply this patch, have an enhancement request filed
for that task and discuss/work on it properly instead.
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] Allow offset changes to be considered harmless.
2020-05-13 11:46 ` Dodji Seketeli
@ 2020-05-14 13:21 ` Giuliano Procida
0 siblings, 0 replies; 18+ messages in thread
From: Giuliano Procida @ 2020-05-14 13:21 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team, Matthias Männich
Hi.
On Wed, 13 May 2020 at 12:46, Dodji Seketeli <dodji@seketeli.org> wrote:
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > This changes lets the user flip offset changes from harmful to
> > harmless. This is a simple way of shrinking very verbose diffs where
> > most changes within structs are offset changes caused by much rarer
> > member addition, removal and size changes.
>
> Hmmh.
>
> I think this is probably too simple.
>
> Offset changes can really signal a problem on their own. So I am not
> for categorizing them as harmless.
>
> I understand that the reporting of offset changes that are a consequence
> of an addition, removal or change of a data member can be verbose, but
> I'd rather keep them as is for now, rather than risking some false
> negative because we want to go the easy route.
>
> In other words, if we really want to filter out those /consequential/
> offset changes, then we need to properly detect them and flag them as
> being filtered.
>
The tests do show them being filtered out. Did you mean in some other way?
+ 1 data member changes (2 filtered):
+ type of 'int S::a[4]' changed:
+ type name changed from 'int[4]' to 'int[8]'
+ array type size changed from 128 to 256
+ array type subrange 1 changed length from 4 to 8
> I understand that that is a bigger undertaking, but I think that would
> the right way to do it.
>
> So I'd rather not apply this patch, have an enhancement request filed
> for that task and discuss/work on it properly instead.
>
I think the wider discussion is mostly covered by the other email thread.
Regards,
Giuliano.
> Cheers,
>
> --
> Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscribe@android.com.
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] Add abidiff --offset-changes-are-harmless tests.
2020-05-04 16:24 [PATCH 0/3] Add an option to give finer-grained control of offset reporting Giuliano Procida
2020-05-04 16:24 ` [PATCH 1/3] abidiff.cc: tidy using directives and declarations Giuliano Procida
2020-05-04 16:24 ` [PATCH 2/3] Allow offset changes to be considered harmless Giuliano Procida
@ 2020-05-04 16:24 ` Giuliano Procida
2020-05-13 11:48 ` Dodji Seketeli
2020-05-12 14:51 ` [PATCH 0/3] Add an option to give finer-grained control of offset reporting Matthias Maennich
3 siblings, 1 reply; 18+ messages in thread
From: Giuliano Procida @ 2020-05-04 16:24 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
The new --offset-changes-are-harmless option changes the default
interpretation of what constitutes a harmless change and impacts the
--harmless and --no-harmful options.
This commit adds tests covering the various possibilities.
The object files have the following changes.
- S::z changes from void* to int* (harmless)
- S::a changes size (harmful)
- S::b changes offset (normally harmful, harmless with new flag)
* tests/data/Makefile.am: Add new test case files.
* tests/data/test-abidiff-exit/test-offset-harm-report0.txt:
New test case.
* tests/data/test-abidiff-exit/test-offset-harm-report1.txt:
Ditto.
* tests/data/test-abidiff-exit/test-offset-harm-report2.txt:
Ditto.
* tests/data/test-abidiff-exit/test-offset-harm-report3.txt:
Ditto.
* tests/data/test-abidiff-exit/test-offset-harm-v0.c: Ditto.
* tests/data/test-abidiff-exit/test-offset-harm-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-offset-harm-v1.c: Ditto.
* tests/data/test-abidiff-exit/test-offset-harm-v1.o: Ditto.
* tests/test-abidiff-exit.cc: Run new test cases.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
tests/data/Makefile.am | 8 ++++
.../test-offset-harm-report0.txt | 15 ++++++++
.../test-offset-harm-report1.txt | 20 ++++++++++
.../test-offset-harm-report2.txt | 3 ++
.../test-offset-harm-report3.txt | 14 +++++++
.../test-abidiff-exit/test-offset-harm-v0.c | 7 ++++
.../test-abidiff-exit/test-offset-harm-v0.o | Bin 0 -> 2872 bytes
.../test-abidiff-exit/test-offset-harm-v1.c | 7 ++++
.../test-abidiff-exit/test-offset-harm-v1.o | Bin 0 -> 2864 bytes
tests/test-abidiff-exit.cc | 36 ++++++++++++++++++
10 files changed, 110 insertions(+)
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report0.txt
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report1.txt
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report2.txt
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report3.txt
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v0.c
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v0.o
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v1.c
create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v1.o
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index a1b9bf64..6b116c55 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -151,6 +151,14 @@ test-abidiff-exit/test-decl-struct-v0.o \
test-abidiff-exit/test-decl-struct-v1.c \
test-abidiff-exit/test-decl-struct-v1.o \
test-abidiff-exit/test-decl-struct-report.txt \
+test-abidiff-exit/test-offset-harm-v0.c \
+test-abidiff-exit/test-offset-harm-v0.o \
+test-abidiff-exit/test-offset-harm-v1.c \
+test-abidiff-exit/test-offset-harm-v1.o \
+test-abidiff-exit/test-offset-harm-report0.txt \
+test-abidiff-exit/test-offset-harm-report1.txt \
+test-abidiff-exit/test-offset-harm-report2.txt \
+test-abidiff-exit/test-offset-harm-report3.txt \
\
test-diff-dwarf/test0-v0.cc \
test-diff-dwarf/test0-v0.o \
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-report0.txt b/tests/data/test-abidiff-exit/test-offset-harm-report0.txt
new file mode 100644
index 00000000..6ac7022d
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-offset-harm-report0.txt
@@ -0,0 +1,15 @@
+Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+1 function with some indirect sub-type change:
+
+ [C] 'function void reg(S*)' at test-offset-harm-v1.c:7:1 has some indirect sub-type changes:
+ parameter 1 of type 'S*' has sub-type changes:
+ in pointed to type 'struct S' at test-offset-harm-v1.c:1:1:
+ type size changed from 256 to 384 (in bits)
+ 1 data member changes (2 filtered):
+ type of 'int S::a[4]' changed:
+ type name changed from 'int[4]' to 'int[8]'
+ array type size changed from 128 to 256
+ array type subrange 1 changed length from 4 to 8
+
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-report1.txt b/tests/data/test-abidiff-exit/test-offset-harm-report1.txt
new file mode 100644
index 00000000..85a64b07
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-offset-harm-report1.txt
@@ -0,0 +1,20 @@
+Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+1 function with some indirect sub-type change:
+
+ [C] 'function void reg(S*)' at test-offset-harm-v1.c:7:1 has some indirect sub-type changes:
+ parameter 1 of type 'S*' has sub-type changes:
+ in pointed to type 'struct S' at test-offset-harm-v1.c:1:1:
+ type size changed from 256 to 384 (in bits)
+ 3 data member changes:
+ type of 'void* S::z' changed:
+ in pointed to type 'void':
+ type name changed from 'void' to 'int'
+ type size changed from 0 to 32 (in bits)
+ type of 'int S::a[4]' changed:
+ type name changed from 'int[4]' to 'int[8]'
+ array type size changed from 128 to 256
+ array type subrange 1 changed length from 4 to 8
+ 'int S::b' offset changed from 192 to 320 (in bits) (by +128 bits)
+
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-report2.txt b/tests/data/test-abidiff-exit/test-offset-harm-report2.txt
new file mode 100644
index 00000000..9666a8fd
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-offset-harm-report2.txt
@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-report3.txt b/tests/data/test-abidiff-exit/test-offset-harm-report3.txt
new file mode 100644
index 00000000..c4eb748f
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-offset-harm-report3.txt
@@ -0,0 +1,14 @@
+Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+1 function with some indirect sub-type change:
+
+ [C] 'function void reg(S*)' at test-offset-harm-v1.c:7:1 has some indirect sub-type changes:
+ parameter 1 of type 'S*' has sub-type changes:
+ in pointed to type 'struct S' at test-offset-harm-v1.c:1:1:
+ 2 data member changes (1 filtered):
+ type of 'void* S::z' changed:
+ in pointed to type 'void':
+ type name changed from 'void' to 'int'
+ 'int S::b' offset changed from 192 to 320 (in bits) (by +128 bits)
+
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-v0.c b/tests/data/test-abidiff-exit/test-offset-harm-v0.c
new file mode 100644
index 00000000..c05e9742
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-offset-harm-v0.c
@@ -0,0 +1,7 @@
+struct S {
+ void* z;
+ int a[4];
+ int b;
+};
+
+void reg(struct S* s) { (void)s; }
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-v0.o b/tests/data/test-abidiff-exit/test-offset-harm-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..b6222d63e7f27c8ea10d8173bbd98419650e657c
GIT binary patch
literal 2872
zcmbtV&ubiY6o22H-Rx|p*(TXa6XIbCN-a1uNtBwlq^?HW1d9}<7g56O?ssPgXJ^99
zY~5BxC?fP0Q4rCCAP61=j~<0yJ$U!*AK<OvLEoGCemCF!c5>0j?!5Ot@8|dTC$HbS
zQDY1wF}MyVnnVGn3ny~75WCQURk*YN@X!5+A3r{R@6nG4*9((bWH?jE%EwaV+S_0=
zX1;@vaUWQ%MG_c3Fr)b{N~X~RR&Qn~u0h>wkPzX2fj$N_%_ed%>y0qZp_TFgSaa5*
zEX2P30i6~zw4EcV_W1}b)1gSV!k%q+ETg^AUSX@v<@0FmFvGO0tJXE^ip4ruW(f?l
zR1Z2kn6!QtEMyJF4m#wpZ~+cF9wN<W&~P2IVM{_Z0a`A5d0l3>xK4GtYhS&(PN9np
zC3kHNz@pp-Xo2UcQ|b{gheC2x3RH;u_%(_s#{O<E0YgFv{|s`T<xVmjW;}OBemZvU
zZo2&<s&O7#WffG{8Y>TK21Z6n9N3dM3xk*sY?|AfZ{D%@wqCNYxZCcQy}h};iN6<Y
zXPi%Bz8mnEr(qveKkbipr#qLO%P-naU^_!UyBGIIX%Z)s%$dX=gz>=1{a(bgQ@Zjd
zS?Wbe-;cZ?NrH%bqh!pz;3!S{;lTI&c#tOHz>C73-wOjjj651%<_-MZFGL6NK{y;b
zd>ZBeH~03g+Rweld!Zkf{X5$`m$Z=-lL2r}aCZMKVAEx`ai(b=;}kxPIQ@x=1$d>s
z{8?>pY2gMslyK#jR-g`l{ZnPciLQci5oZfBXE)GNMTR(Ty3K&uNtiR?v6<7QVn9oZ
zbKEFLJcp5Dz|s>~iR1pSAOoJb3INX=-X16OFx%YKJC+W3L2_h^s7aX&vI5~WnpFak
zR!RmE9}4{`4gW~!>w;5f>S|n4{MB_&6kMIf2dCtbJySmo;X`a$$a>vtr{K20sYkU>
zaI<@3JZ8xAwB#cx<$RihOQ!|gUY3EI^2m1wyf+CB!+4mWt&sg*FXeaV%zo;}0iIkn
zBno3LL$Cz6{bW4mcxAY8l5-a?4kyd~{`(?`j}C{aKjxzG|H(Emp5{u<r%cxs!fZbY
z{bZ+=zy)N4=_l(A;jc42{vtZiKKQTkTNtb7q?)q+PoyZM`_dypk8M}+A@S$(1%_V*
zPWy@=xo7dEDQFkJdQLJtF026G2rilKdm9ZpM!sJNa8m{fuKUtBtNK3@heF0v|8#5V
z{SOe+b0P$f1bB-Sg>>ITv{lvrPSlt4MD^)h>-CQiqnxr&6bm{l4&!eU5M0)m{6oa6
z;{Ooww7*o6agu+EMk%X)_=;DO?)$ZfpSwE{Ge!K4mPL+V!e63LHGjWU%pbi9GG5Lr
w^`qCO++9)swrH5{Z+dO?cv_RHp&nCHNOV7_q>rESR>l7=`u|=HMp=*l7e>q3{{R30
literal 0
HcmV?d00001
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-v1.c b/tests/data/test-abidiff-exit/test-offset-harm-v1.c
new file mode 100644
index 00000000..025e8462
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-offset-harm-v1.c
@@ -0,0 +1,7 @@
+struct S {
+ int* z;
+ int a[8];
+ int b;
+};
+
+void reg(struct S* s) { (void)s; }
diff --git a/tests/data/test-abidiff-exit/test-offset-harm-v1.o b/tests/data/test-abidiff-exit/test-offset-harm-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..0b2ee10ccb2c4512f1671b5455057fb33de2b914
GIT binary patch
literal 2864
zcmbtVUuzsy6hC)oH#^BBYm%)rAwEn&X$zg1Bud@3q^?HW1d9}<FG7iTckj*)?#_gn
z**1-eAX4mG1wjShd=Wv=Z&2{X2S0#czz^V?V4<EfbMNl#&E!Q7yZ4;?JAdz)`{&cQ
zZr!Lb28tM5hv`hA0Ecr^xt)n^Xu>Mo*?aWo-lI?ceth@WpAfEQDl?0)n8}5~D#(@h
zV3RTPeT0l-V3kY)g99__K7yvv0amSlfZ_$Hn#60>S`el%NAi9A*RugkvyKy(kA)Ac
zJ}FZrW8eLV+JyvlXGp4iIly=hxv&-XQln`Zjg7_%TdgmjMQxKAre$5Vu31+s*2LnL
zz%cXaezS!^tIvUD?W3mlA_~_n!vvUPY`;m?^>eV_^w7$b$_5tZve(vCjPvW1slE2~
z8|!4hP%A{QtYJ-;s6;9bFm8d=atcr;?%`-;UX1<ISOSKG5dImYB1xU7-%mv940t?p
z?rpi<%&T@5=JPB#U8}8pRx!|X7=^w)4wJwSMbD;Q-+bqey}R|QeZ}2&x9siB?M?i>
zY&)ZL9Eu%Zgdz^QIOTD7uyfS9>|B1uc6{6E^W;I;9mG)>jT2`a-Vef_lk(0`B&Ss6
zjg#0LMqNJi{3!B=!W%>*;rWMg)D3#v<6$q3f}S@FI=mD3JQ#Ykyu|DAlxL!YcrWPp
z9dQ(-05^Acui6*h6rF&FdHv3I>!P+2fHPS?uv^FP0y|n}8;f=GF?R0-#OY6z_5St7
z^25sR(%cO+$l=nFnqP&#o|P%F$7L{nzsa|l(Hp2KBSRiDU1z{#_s^&d$WF)%MiT?(
zPhdG-zS|5~I>jEhc?lVC;tBvfV|Z&!`!LB=zFFx|XQWoPfHNhNLS-PFR<lYV(n`)i
z;e9!OUc*0@^L5Fovf|gcSg<dC_u~Ry?Bc^y;;5Rbo|?2Fwk%~m@3m8KTjEq>u}*N4
z2O~UT$kRCI10|*6C<T`e3b>sl0XG&y?)F4y>>mVSKSEt5^G+ug_h!^Q=An;=w&*ep
zLZMtR1-RX4G!l4QxM7qE7w-%wNqP4}=_CdR{g{u0Ec}164fLm8seY<(Z7EFllbla?
zS`M5;MwouG-j?<{)BVq*0qukT>c54)dQ8fx^8Z4U0_nE&KF}lEF4&OxL;mu@6N%Hl
zB1r97dRvHg@UO=t{3#7cU*&8v>bAF0qhn-DcP7zh&L*R7OY1DFe;^Nq@~8Ug*3#?m
zA*RPf2%bpr7EKDI+a92<EdMt$zv>g^r*p06KSYdTsyb23Xi*-<-xVOa%CGoGh?n{Q
zA^mB86-oIi{y8d@T(o;Oa~VuEy6v~p|Ae{R(5<O!CaLu+nD$3m|9&p%AH537U-gyh
xIguN&mdt-!7EJdyy*9c(eG~ocAp~DgQAl(@6iHt{#VzyyP1gT?(HUjk|6c^Q+YJB!
literal 0
HcmV?d00001
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 5afc15bc..372cafcf 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -203,6 +203,42 @@ InOutSpec in_out_specs[] =
"data/test-abidiff-exit/test-decl-struct-report.txt",
"output/test-abidiff-exit/test-decl-struct-report.txt"
},
+ {
+ "data/test-abidiff-exit/test-offset-harm-v0.o",
+ "data/test-abidiff-exit/test-offset-harm-v1.o",
+ "",
+ "--offset-changes-are-harmless",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+ "data/test-abidiff-exit/test-offset-harm-report0.txt",
+ "output/test-abidiff-exit/test-offset-harm-report0.txt"
+ },
+ {
+ "data/test-abidiff-exit/test-offset-harm-v0.o",
+ "data/test-abidiff-exit/test-offset-harm-v1.o",
+ "",
+ "--offset-changes-are-harmless --harmless",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+ "data/test-abidiff-exit/test-offset-harm-report1.txt",
+ "output/test-abidiff-exit/test-offset-harm-report1.txt"
+ },
+ {
+ "data/test-abidiff-exit/test-offset-harm-v0.o",
+ "data/test-abidiff-exit/test-offset-harm-v1.o",
+ "",
+ "--offset-changes-are-harmless --no-harmful",
+ abigail::tools_utils::ABIDIFF_OK,
+ "data/test-abidiff-exit/test-offset-harm-report2.txt",
+ "output/test-abidiff-exit/test-offset-harm-report2.txt"
+ },
+ {
+ "data/test-abidiff-exit/test-offset-harm-v0.o",
+ "data/test-abidiff-exit/test-offset-harm-v1.o",
+ "",
+ "--offset-changes-are-harmless --harmless --no-harmful",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+ "data/test-abidiff-exit/test-offset-harm-report3.txt",
+ "output/test-abidiff-exit/test-offset-harm-report3.txt"
+ },
{0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
};
--
2.26.2.526.g744177e7f7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-04 16:24 [PATCH 0/3] Add an option to give finer-grained control of offset reporting Giuliano Procida
` (2 preceding siblings ...)
2020-05-04 16:24 ` [PATCH 3/3] Add abidiff --offset-changes-are-harmless tests Giuliano Procida
@ 2020-05-12 14:51 ` Matthias Maennich
2020-05-13 12:06 ` Dodji Seketeli
3 siblings, 1 reply; 18+ messages in thread
From: Matthias Maennich @ 2020-05-12 14:51 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team
On Mon, May 04, 2020 at 05:24:36PM +0100, Giuliano Procida wrote:
>This is from an internal request.
>
>I'm pretty happy with the core changes. The new command line option
>and plumbing are what I think deserve more scrutiny. Do we want a
>more generic way of manipulating the change categories? This change
>takes us up to 3 flags.
I think I would like to take a step back and clarify what we actually
want to achieve. The issue raised was that a data member insertion often
leads to a long list of offset changes for the following data members.
So, the reporting of the offset changes feels overly obvious and noisy
and distracts from the actual issue (i.e. the root cause) that needs
addressing.
As such this is a reporting issue as each single member offset change is
in fact an ABI breakage, and actually never harmless.
What appears to me as a more effective way of addressing this would be
to collapse subsequent offset changes. Instead of dropping the reports
of those completely, how about we add an option that changes the report
from
'struct task_struct at sched.h:635:1' changed:
type size hasn't changed
1 data member insertion:
'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
there are data member changes:
'void* task_struct::journal_info' offset changed from 16704 to 16768 (in bits) (by +64 bits)
'bio_list* task_struct::bio_list' offset changed from 16768 to 16832 (in bits) (by +64 bits)
'blk_plug* task_struct::plug' offset changed from 16832 to 16896 (in bits) (by +64 bits)
'reclaim_state* task_struct::reclaim_state' offset changed from 16896 to 16960 (in bits) (by +64 bits)
'backing_dev_info* task_struct::backing_dev_info' offset changed from 16960 to 17024 (in bits) (by +64 bits)
'io_context* task_struct::io_context' offset changed from 17024 to 17088 (in bits) (by +64 bits)
'capture_control* task_struct::capture_control' offset changed from 17088 to 17152 (in bits) (by +64 bits)
'unsigned long int task_struct::ptrace_message' offset changed from 17152 to 17216 (in bits) (by +64 bits)
'kernel_siginfo_t* task_struct::last_siginfo' offset changed from 17216 to 17280 (in bits) (by +64 bits)
to
'struct task_struct at sched.h:635:1' changed:
type size hasn't changed
1 data member insertion:
'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
offset of 9 consecutive data members changed by +64bits (journal_info .. last_siginfo)
While looking at that, we likely can also reduce the name we print for
each data member:
'struct task_struct at sched.h:635:1' changed:
type size hasn't changed
1 data member insertion:
'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
there are data member changes:
'journal_info' offset changed from 16704 to 16768 (in bits) (by +64 bits)
'bio_list' offset changed from 16768 to 16832 (in bits) (by +64 bits)
'plug' offset changed from 16832 to 16896 (in bits) (by +64 bits)
'reclaim_state' offset changed from 16896 to 16960 (in bits) (by +64 bits)
'backing_dev_info' offset changed from 16960 to 17024 (in bits) (by +64 bits)
'io_context' offset changed from 17024 to 17088 (in bits) (by +64 bits)
'capture_control' offset changed from 17088 to 17152 (in bits) (by +64 bits)
'ptrace_message' offset changed from 17152 to 17216 (in bits) (by +64 bits)
'last_siginfo' offset changed from 17216 to 17280 (in bits) (by +64 bits)
Let me know what you think.
Cheers,
Matthias
>
>Regards,
>Giuliano.
>
>Giuliano Procida (3):
> abidiff.cc: tidy using directives and declarations
> Allow offset changes to be considered harmless.
> Add abidiff --offset-changes-are-harmless tests.
>
> include/abg-comparison.h | 35 ++++++----
> src/abg-comp-filter.cc | 14 ++--
> src/abg-comparison.cc | 15 +++-
> src/abg-reporter-priv.cc | 17 +++--
> tests/data/Makefile.am | 8 +++
> .../test-offset-harm-report0.txt | 15 ++++
> .../test-offset-harm-report1.txt | 20 ++++++
> .../test-offset-harm-report2.txt | 3 +
> .../test-offset-harm-report3.txt | 14 ++++
> .../test-abidiff-exit/test-offset-harm-v0.c | 7 ++
> .../test-abidiff-exit/test-offset-harm-v0.o | Bin 0 -> 2872 bytes
> .../test-abidiff-exit/test-offset-harm-v1.c | 7 ++
> .../test-abidiff-exit/test-offset-harm-v1.o | Bin 0 -> 2864 bytes
> tests/test-abidiff-exit.cc | 36 ++++++++++
> tools/abidiff.cc | 64 +++++++++++-------
> 15 files changed, 202 insertions(+), 53 deletions(-)
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report0.txt
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report1.txt
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report2.txt
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-report3.txt
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v0.c
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v0.o
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v1.c
> create mode 100644 tests/data/test-abidiff-exit/test-offset-harm-v1.o
>
>--
>2.26.2.526.g744177e7f7-goog
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-12 14:51 ` [PATCH 0/3] Add an option to give finer-grained control of offset reporting Matthias Maennich
@ 2020-05-13 12:06 ` Dodji Seketeli
2020-05-13 19:38 ` Matthias Maennich
0 siblings, 1 reply; 18+ messages in thread
From: Dodji Seketeli @ 2020-05-13 12:06 UTC (permalink / raw)
To: Matthias Maennich; +Cc: Giuliano Procida, libabigail, kernel-team
Matthias Maennich <maennich@google.com> a écrit:
> As such this is a reporting issue as each single member offset change is
> in fact an ABI breakage, and actually never harmless.
Aggreed. Though, as I was saying in
https://sourceware.org/pipermail/libabigail/2020q2/002229.html, we could
avoid emitting them only when they are a consequence of an ABI change
that we've already emitted, just like ...
> What appears to me as a more effective way of addressing this would be
> to collapse subsequent offset changes. Instead of dropping the reports
> of those completely, how about we add an option that changes the report
> from
>
> 'struct task_struct at sched.h:635:1' changed:
> type size hasn't changed
> 1 data member insertion:
> 'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
> there are data member changes:
> 'void* task_struct::journal_info' offset changed from 16704 to 16768 (in bits) (by +64 bits)
> 'bio_list* task_struct::bio_list' offset changed from 16768 to 16832 (in bits) (by +64 bits)
> 'blk_plug* task_struct::plug' offset changed from 16832 to 16896 (in bits) (by +64 bits)
> 'reclaim_state* task_struct::reclaim_state' offset changed from 16896 to 16960 (in bits) (by +64 bits)
> 'backing_dev_info* task_struct::backing_dev_info' offset changed from 16960 to 17024 (in bits) (by +64 bits)
> 'io_context* task_struct::io_context' offset changed from 17024 to 17088 (in bits) (by +64 bits)
> 'capture_control* task_struct::capture_control' offset changed from 17088 to 17152 (in bits) (by +64 bits)
> 'unsigned long int task_struct::ptrace_message' offset changed from 17152 to 17216 (in bits) (by +64 bits)
> 'kernel_siginfo_t* task_struct::last_siginfo' offset changed from 17216 to 17280 (in bits) (by +64 bits)
>
> to
>
> 'struct task_struct at sched.h:635:1' changed:
> type size hasn't changed
> 1 data member insertion:
> 'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
>
> offset of 9 consecutive data members changed by +64bits (journal_info .. last_siginfo)
... here!
And here, I totally agree with you that this would be an interesting way
to report it. But we could also completely do away with reporting about
them because they are redundant, in a sense. Note that there is already
a pass that flags redundant changes and avoid emitting them, so in a
sense, this new particular case would not be unheard of, so to speak.
> While looking at that, we likely can also reduce the name we print for
> each data member:
>
> 'struct task_struct at sched.h:635:1' changed:
> type size hasn't changed
> 1 data member insertion:
> 'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
So you would emit a qualified data member name here ...
> there are data member changes:
> 'journal_info' offset changed from 16704 to 16768 (in bits) (by +64 bits)
> 'bio_list' offset changed from 16768 to 16832 (in bits) (by +64 bits)
> 'plug' offset changed from 16832 to 16896 (in bits) (by +64 bits)
> 'reclaim_state' offset changed from 16896 to 16960 (in bits) (by +64 bits)
> 'backing_dev_info' offset changed from 16960 to 17024 (in bits) (by +64 bits)
> 'io_context' offset changed from 17024 to 17088 (in bits) (by +64 bits)
> 'capture_control' offset changed from 17088 to 17152 (in bits) (by +64 bits)
> 'ptrace_message' offset changed from 17152 to 17216 (in bits) (by +64 bits)
> 'last_siginfo' offset changed from 17216 to 17280 (in bits) (by +64 bits)
but not here?
Also, would it make sense to emit those qualified data member names when
we are analyzying a C++ program? Or do you think it's more legible to
emit always emit non-qualified data member names?
I know I repeat myself, but if we are to work on "tasks", it'd be nice
to file them as enhancement reports so that we discuss them at "one
place" and so that we can track them. I can go ahead and file these if
you guys want :-)
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-13 12:06 ` Dodji Seketeli
@ 2020-05-13 19:38 ` Matthias Maennich
2020-05-14 8:35 ` Dodji Seketeli
0 siblings, 1 reply; 18+ messages in thread
From: Matthias Maennich @ 2020-05-13 19:38 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Giuliano Procida, libabigail, kernel-team
Hi Dodji!
On Wed, May 13, 2020 at 02:06:32PM +0200, Dodji Seketeli wrote:
>Matthias Maennich <maennich@google.com> a écrit:
>
>> As such this is a reporting issue as each single member offset change is
>> in fact an ABI breakage, and actually never harmless.
>
>Aggreed. Though, as I was saying in
>https://sourceware.org/pipermail/libabigail/2020q2/002229.html, we could
>avoid emitting them only when they are a consequence of an ABI change
>that we've already emitted, just like ...
>
>
>> What appears to me as a more effective way of addressing this would be
>> to collapse subsequent offset changes. Instead of dropping the reports
>> of those completely, how about we add an option that changes the report
>> from
>>
>> 'struct task_struct at sched.h:635:1' changed:
>> type size hasn't changed
>> 1 data member insertion:
>> 'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
>> there are data member changes:
>> 'void* task_struct::journal_info' offset changed from 16704 to 16768 (in bits) (by +64 bits)
>> 'bio_list* task_struct::bio_list' offset changed from 16768 to 16832 (in bits) (by +64 bits)
>> 'blk_plug* task_struct::plug' offset changed from 16832 to 16896 (in bits) (by +64 bits)
>> 'reclaim_state* task_struct::reclaim_state' offset changed from 16896 to 16960 (in bits) (by +64 bits)
>> 'backing_dev_info* task_struct::backing_dev_info' offset changed from 16960 to 17024 (in bits) (by +64 bits)
>> 'io_context* task_struct::io_context' offset changed from 17024 to 17088 (in bits) (by +64 bits)
>> 'capture_control* task_struct::capture_control' offset changed from 17088 to 17152 (in bits) (by +64 bits)
>> 'unsigned long int task_struct::ptrace_message' offset changed from 17152 to 17216 (in bits) (by +64 bits)
>> 'kernel_siginfo_t* task_struct::last_siginfo' offset changed from 17216 to 17280 (in bits) (by +64 bits)
>>
>> to
>>
>> 'struct task_struct at sched.h:635:1' changed:
>> type size hasn't changed
>> 1 data member insertion:
>> 'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
>>
>> offset of 9 consecutive data members changed by +64bits (journal_info .. last_siginfo)
>
>... here!
>
>And here, I totally agree with you that this would be an interesting way
>to report it. But we could also completely do away with reporting about
>them because they are redundant, in a sense. Note that there is already
>a pass that flags redundant changes and avoid emitting them, so in a
>sense, this new particular case would not be unheard of, so to speak.
I think I would like the one line of mention as I suggested. Omitting it
conditionally introduces more logic and likely we confuse users. I also
do not see them as redundant, just obvious and therefore a short form is
enough to transmit this information. Does this make sense?
>
>
>> While looking at that, we likely can also reduce the name we print for
>> each data member:
>>
>> 'struct task_struct at sched.h:635:1' changed:
>> type size hasn't changed
>> 1 data member insertion:
>> 'unsigned int task_struct::in_ubsan', at offset 16704 (in bits) at sched.h:1006:1
>
>So you would emit a qualified data member name here ...
>
>> there are data member changes:
>> 'journal_info' offset changed from 16704 to 16768 (in bits) (by +64 bits)
>> 'bio_list' offset changed from 16768 to 16832 (in bits) (by +64 bits)
>> 'plug' offset changed from 16832 to 16896 (in bits) (by +64 bits)
>> 'reclaim_state' offset changed from 16896 to 16960 (in bits) (by +64 bits)
>> 'backing_dev_info' offset changed from 16960 to 17024 (in bits) (by +64 bits)
>> 'io_context' offset changed from 17024 to 17088 (in bits) (by +64 bits)
>> 'capture_control' offset changed from 17088 to 17152 (in bits) (by +64 bits)
>> 'ptrace_message' offset changed from 17152 to 17216 (in bits) (by +64 bits)
>> 'last_siginfo' offset changed from 17216 to 17280 (in bits) (by +64 bits)
>
>but not here?
Oh, that was an oversight. I meant to apply the simplification in all
places. I.e. 'in_ubsan' only.
>
>Also, would it make sense to emit those qualified data member names when
>we are analyzying a C++ program? Or do you think it's more legible to
>emit always emit non-qualified data member names?
I think this very much applies to C structs as well. In fact, the above
is a C example. I think we should use the non-qualified version if we
are in a nested block where the rest of the qualified name is already
emitted. In the above example, we are in the block of 'task_struct'
differences, hence we can strip that part away from
task_struct::in_ubsan'.
>
>I know I repeat myself, but if we are to work on "tasks", it'd be nice
>to file them as enhancement reports so that we discuss them at "one
>place" and so that we can track them. I can go ahead and file these if
>you guys want :-)
Understood. And we will happily file them. I guess, what started with
fixes here and there, became more frequent and more involved
contributions without adjusting the process properly. Sorry about that.
Cheers,
Matthias
>
>Cheers,
>
>--
> Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-13 19:38 ` Matthias Maennich
@ 2020-05-14 8:35 ` Dodji Seketeli
2020-05-14 12:39 ` Giuliano Procida
2020-05-18 20:09 ` Matthias Maennich
0 siblings, 2 replies; 18+ messages in thread
From: Dodji Seketeli @ 2020-05-14 8:35 UTC (permalink / raw)
To: Matthias Maennich; +Cc: Giuliano Procida, libabigail, kernel-team
Matthias Maennich <maennich@google.com> a écrit:
[...]
> I think I would like the one line of mention as I suggested. Omitting it
> conditionally introduces more logic and likely we confuse users.
Either way, we'll have to introdroce more logic anyway, so I am not
concerned about that, if, of course that is useful.
I guess what I wanted to say is that the one line mention seems like a
good idea because that example is simple. If you have more than one
non-contiguous addition/removal/changes of data members in the struct,
then I am not sure the one liner mention is still that "relevant". Or
maybe in that case we can avoir the one liner mention and emit the
in-extenso message as we have today? What do you think?
> I also do not see them as redundant, just obvious and therefore a
> short form is enough to transmit this information.
Hmmh, yes, I think you are right. The offset changes are not redundant
per se. They just happen to be caused by the change, but they don't
necessary have to be. Point taken.
[...]
>>Also, would it make sense to emit those qualified data member names when
>>we are analyzying a C++ program? Or do you think it's more legible to
>>emit always emit non-qualified data member names?
>
> I think this very much applies to C structs as well. In fact, the above
> is a C example.
Yeah, I got that. I really meant my question. I'll re-phrase it
differently. Do you think we should avoid removing the
full-qualification when we are analyzing c++ programs? Or do do you
think what you are proposing for C should apply for C++ programs
equally? I would think yes, but I am not sure.
> I think we should use the non-qualified version if we
> are in a nested block where the rest of the qualified name is already
> emitted. In the above example, we are in the block of 'task_struct'
> differences, hence we can strip that part away from
> task_struct::in_ubsan'.
This does make sense to me.
If we agree that we should change this behaviour, then maybe we should
create an enhance request for this. Shall we?
Thanks,
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-14 8:35 ` Dodji Seketeli
@ 2020-05-14 12:39 ` Giuliano Procida
2020-05-18 20:16 ` Matthias Maennich
2020-05-20 7:57 ` Dodji Seketeli
2020-05-18 20:09 ` Matthias Maennich
1 sibling, 2 replies; 18+ messages in thread
From: Giuliano Procida @ 2020-05-14 12:39 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Matthias Maennich, libabigail, kernel-team
I'll just chime in...
I concur that the initial change I proposed is dangerous in the sense that
it allows abidiff to be used in a fashion that will not find certain ABI
differences. Even if the mechanism makes sense, perhaps the policy does
not. However, it's already possible to say --harmless --no-harmful.
The splitting of SIZE/OFFSET categories may have some value for libabigail
itself. I think SIZE would still be overloaded, including alignment and
some other miscellaneous things.
As for a concrete proposal, if the two of you feel you have reached a
consensus, could one of you paste it into the issue tracker?
Now, taking an even bigger step back, the source of this request is to
further compress reports for human consumption. This means removing
redundant information but it also means finding smaller diff descriptions
(while still being legible and having meaningful context). We're
considering ways of presenting different levels of details to our users,
you might consider these abidiff options as making a hierarchy:
--redundant
--no-redundant
--leaf-changes-only --impacted-interfaces
--leaf-changes-only --impacted-interface-counts (this doesn't exist)
--leaf-changes-only
--leaf-changes-only --summarise-offset-changes (this doesn't exist)
--leaf-changes-only --offset-changes-are-harmless (was this request)
At the lowest level here, we'd only see root causes of ABI differences -
useful for people who know there's some difference and just want to track
it down.
At the top level we get the full impact, with the change graph expanded
into a tree as far as possible.
It's quite expensive (in terms of run-time) to generate multiple reports
(Matthias, we should fork and wait if we need to do this) with different
variations. It's also (as the present example demonstrates) not ideal to
add every variation to abidiff itself. So we are doing some things with
post-processing. It might be straightforward to do this for offsets, for
example. If there were an XML format for the diffs as well... :-)
I hope this helps fill in a bit more of the background.
Regards,
Giuliano.
On Thu, 14 May 2020 at 09:35, Dodji Seketeli <dodji@seketeli.org> wrote:
> Matthias Maennich <maennich@google.com> a écrit:
>
> [...]
>
> > I think I would like the one line of mention as I suggested. Omitting it
> > conditionally introduces more logic and likely we confuse users.
>
> Either way, we'll have to introdroce more logic anyway, so I am not
> concerned about that, if, of course that is useful.
>
> I guess what I wanted to say is that the one line mention seems like a
> good idea because that example is simple. If you have more than one
> non-contiguous addition/removal/changes of data members in the struct,
> then I am not sure the one liner mention is still that "relevant". Or
> maybe in that case we can avoir the one liner mention and emit the
> in-extenso message as we have today? What do you think?
>
> > I also do not see them as redundant, just obvious and therefore a
> > short form is enough to transmit this information.
>
> Hmmh, yes, I think you are right. The offset changes are not redundant
> per se. They just happen to be caused by the change, but they don't
> necessary have to be. Point taken.
>
> [...]
>
>
> >>Also, would it make sense to emit those qualified data member names when
> >>we are analyzying a C++ program? Or do you think it's more legible to
> >>emit always emit non-qualified data member names?
> >
> > I think this very much applies to C structs as well. In fact, the above
> > is a C example.
>
> Yeah, I got that. I really meant my question. I'll re-phrase it
> differently. Do you think we should avoid removing the
> full-qualification when we are analyzing c++ programs? Or do do you
> think what you are proposing for C should apply for C++ programs
> equally? I would think yes, but I am not sure.
>
> > I think we should use the non-qualified version if we
> > are in a nested block where the rest of the qualified name is already
> > emitted. In the above example, we are in the block of 'task_struct'
> > differences, hence we can strip that part away from
> > task_struct::in_ubsan'.
>
> This does make sense to me.
>
> If we agree that we should change this behaviour, then maybe we should
> create an enhance request for this. Shall we?
>
> Thanks,
>
> Cheers,
>
> --
> Dodji
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-14 12:39 ` Giuliano Procida
@ 2020-05-18 20:16 ` Matthias Maennich
2020-05-20 7:57 ` Dodji Seketeli
1 sibling, 0 replies; 18+ messages in thread
From: Matthias Maennich @ 2020-05-18 20:16 UTC (permalink / raw)
To: Giuliano Procida; +Cc: Dodji Seketeli, libabigail, kernel-team
On Thu, May 14, 2020 at 01:39:48PM +0100, Giuliano Procida wrote:
>I'll just chime in...
>
>I concur that the initial change I proposed is dangerous in the sense that
>it allows abidiff to be used in a fashion that will not find certain ABI
>differences. Even if the mechanism makes sense, perhaps the policy does
>not. However, it's already possible to say --harmless --no-harmful.
>
>The splitting of SIZE/OFFSET categories may have some value for libabigail
>itself. I think SIZE would still be overloaded, including alignment and
>some other miscellaneous things.
>
>As for a concrete proposal, if the two of you feel you have reached a
>consensus, could one of you paste it into the issue tracker?
>
>Now, taking an even bigger step back, the source of this request is to
>further compress reports for human consumption. This means removing
>redundant information but it also means finding smaller diff descriptions
>(while still being legible and having meaningful context). We're
>considering ways of presenting different levels of details to our users,
>you might consider these abidiff options as making a hierarchy:
>
>--redundant
>--no-redundant
>--leaf-changes-only --impacted-interfaces
>--leaf-changes-only --impacted-interface-counts (this doesn't exist)
>--leaf-changes-only
>--leaf-changes-only --summarise-offset-changes (this doesn't exist)
>--leaf-changes-only --offset-changes-are-harmless (was this request)
>
>At the lowest level here, we'd only see root causes of ABI differences -
>useful for people who know there's some difference and just want to track
>it down.
>At the top level we get the full impact, with the change graph expanded
>into a tree as far as possible.
>
>It's quite expensive (in terms of run-time) to generate multiple reports
>(Matthias, we should fork and wait if we need to do this) with different
>variations. It's also (as the present example demonstrates) not ideal to
>add every variation to abidiff itself. So we are doing some things with
>post-processing. It might be straightforward to do this for offsets, for
>example. If there were an XML format for the diffs as well... :-)
I agree that we can solve the problem either in abigail and use
downstream more CPU to generate multiple reports at the same time or do
some postprocessing of the most verbose report. I like the idea of an
intermediate (XML?) format for the diff, actually. That would allow all
sorts of reports and different flags without touching libabigail's core
diffing.
Yet, my main motivation here to chime in was to point out that we should
improve the default for lossless compression and for human consumption.
For more detail, we can add verbosity flags. Maybe even like you
suggested above. But the first target group should be developers that
need to fix an actual breakage.
Cheers,
Matthias
>
>I hope this helps fill in a bit more of the background.
>
>Regards,
>Giuliano.
>
>On Thu, 14 May 2020 at 09:35, Dodji Seketeli <dodji@seketeli.org> wrote:
>
>> Matthias Maennich <maennich@google.com> a écrit:
>>
>> [...]
>>
>> > I think I would like the one line of mention as I suggested. Omitting it
>> > conditionally introduces more logic and likely we confuse users.
>>
>> Either way, we'll have to introdroce more logic anyway, so I am not
>> concerned about that, if, of course that is useful.
>>
>> I guess what I wanted to say is that the one line mention seems like a
>> good idea because that example is simple. If you have more than one
>> non-contiguous addition/removal/changes of data members in the struct,
>> then I am not sure the one liner mention is still that "relevant". Or
>> maybe in that case we can avoir the one liner mention and emit the
>> in-extenso message as we have today? What do you think?
>>
>> > I also do not see them as redundant, just obvious and therefore a
>> > short form is enough to transmit this information.
>>
>> Hmmh, yes, I think you are right. The offset changes are not redundant
>> per se. They just happen to be caused by the change, but they don't
>> necessary have to be. Point taken.
>>
>> [...]
>>
>>
>> >>Also, would it make sense to emit those qualified data member names when
>> >>we are analyzying a C++ program? Or do you think it's more legible to
>> >>emit always emit non-qualified data member names?
>> >
>> > I think this very much applies to C structs as well. In fact, the above
>> > is a C example.
>>
>> Yeah, I got that. I really meant my question. I'll re-phrase it
>> differently. Do you think we should avoid removing the
>> full-qualification when we are analyzing c++ programs? Or do do you
>> think what you are proposing for C should apply for C++ programs
>> equally? I would think yes, but I am not sure.
>>
>> > I think we should use the non-qualified version if we
>> > are in a nested block where the rest of the qualified name is already
>> > emitted. In the above example, we are in the block of 'task_struct'
>> > differences, hence we can strip that part away from
>> > task_struct::in_ubsan'.
>>
>> This does make sense to me.
>>
>> If we agree that we should change this behaviour, then maybe we should
>> create an enhance request for this. Shall we?
>>
>> Thanks,
>>
>> Cheers,
>>
>> --
>> Dodji
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-14 12:39 ` Giuliano Procida
2020-05-18 20:16 ` Matthias Maennich
@ 2020-05-20 7:57 ` Dodji Seketeli
1 sibling, 0 replies; 18+ messages in thread
From: Dodji Seketeli @ 2020-05-20 7:57 UTC (permalink / raw)
To: Giuliano Procida; +Cc: Matthias Maennich, libabigail, kernel-team
Giuliano Procida <gprocida@google.com> a écrit:
[...]
> It's quite expensive (in terms of run-time) to generate multiple reports
> (Matthias, we should fork and wait if we need to do this) with different
> variations. It's also (as the present example demonstrates) not ideal to
> add every variation to abidiff itself. So we are doing some things with
> post-processing.
[...]
> If there were an XML format for the diffs as well... :-)
Actually, I have been thinking about that at some point. My thinking
was more around a textual serialization format for the diff reports. I
think it's roughly equals what you mean by "XML format for the diffs".
I went ahead and started to design an XML schema to define the format.
The schema is in the relax-ng format and can be read at
https://sourceware.org/git/?p=libabigail.git;a=blob;f=doc/manuals/ifac-schema.rng;h=e0676f6a233770ab09f296db6cfd2fb5421ed60f;hb=9105467c9020ebbbea689c056e681b54287db201.
The (very light) introduction to that schema can be read at
https://sourceware.org/git/?p=libabigail.git;a=blob;f=doc/manuals/libabigail-xml-report.rst;h=0e690167b6bbe8b8629c33baece62f4e43810af2;hb=9105467c9020ebbbea689c056e681b54287db201.
The idea was to be able to emit a report in an abstract format so that
other tools could come up with html (or whatever interactive format)
reports as they'd see fit. I wanted to be able to share that abstract
change reporting format with other tools like e.g, the
"abi-compliance-checker" tool, but the author didn't seem interested at
the time.
That was ~ 5 years ago. I moved to other more pressing matters since
then.
> It might be straightforward to do this for offsets, for example.
We can do whatever we want with post-processing, granted, so you could
do the offsets change report formating there. Nevertheless, I think
this is such a fundamental thing that abidiff ought to be able to do it
anyway.
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] Add an option to give finer-grained control of offset reporting.
2020-05-14 8:35 ` Dodji Seketeli
2020-05-14 12:39 ` Giuliano Procida
@ 2020-05-18 20:09 ` Matthias Maennich
1 sibling, 0 replies; 18+ messages in thread
From: Matthias Maennich @ 2020-05-18 20:09 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Giuliano Procida, libabigail, kernel-team
On Thu, May 14, 2020 at 10:35:22AM +0200, Dodji Seketeli wrote:
>Matthias Maennich <maennich@google.com> a écrit:
>
>[...]
>
>> I think I would like the one line of mention as I suggested. Omitting it
>> conditionally introduces more logic and likely we confuse users.
>
>Either way, we'll have to introdroce more logic anyway, so I am not
>concerned about that, if, of course that is useful.
>
>I guess what I wanted to say is that the one line mention seems like a
>good idea because that example is simple. If you have more than one
>non-contiguous addition/removal/changes of data members in the struct,
>then I am not sure the one liner mention is still that "relevant". Or
>maybe in that case we can avoir the one liner mention and emit the
>in-extenso message as we have today? What do you think?
>
>> I also do not see them as redundant, just obvious and therefore a
>> short form is enough to transmit this information.
>
>Hmmh, yes, I think you are right. The offset changes are not redundant
>per se. They just happen to be caused by the change, but they don't
>necessary have to be. Point taken.
>
>[...]
>
>
>>>Also, would it make sense to emit those qualified data member names when
>>>we are analyzying a C++ program? Or do you think it's more legible to
>>>emit always emit non-qualified data member names?
>>
>> I think this very much applies to C structs as well. In fact, the above
>> is a C example.
>
>Yeah, I got that. I really meant my question. I'll re-phrase it
>differently. Do you think we should avoid removing the
>full-qualification when we are analyzing c++ programs? Or do do you
>think what you are proposing for C should apply for C++ programs
>equally? I would think yes, but I am not sure.
>
>> I think we should use the non-qualified version if we
>> are in a nested block where the rest of the qualified name is already
>> emitted. In the above example, we are in the block of 'task_struct'
>> differences, hence we can strip that part away from
>> task_struct::in_ubsan'.
>
>This does make sense to me.
>
>If we agree that we should change this behaviour, then maybe we should
>create an enhance request for this. Shall we?
I think we do agree :-)
I opened Bug 26012 and 26013 to track those.
Cheers,
Matthias
>
>Thanks,
>
>Cheers,
>
>--
> Dodji
^ permalink raw reply [flat|nested] 18+ messages in thread