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.
>
>
@@ -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
@@ -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;
@@ -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;
}
@@ -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;
@@ -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();