* [PATCH 0/3] Type equality refactor and instrumentation @ 2020-07-08 9:53 Giuliano Procida 2020-07-08 9:53 ` [PATCH 1/3] abg-ir.cc: Tidy some operator== definitions Giuliano Procida ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Giuliano Procida @ 2020-07-08 9:53 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich Hi Dodji. This series refactors various operator== methods making their common functionality evident. The first patch is just a prelude to make the second smaller. The second patch does the refactoring. I'm not attached to the name 'equality_helper'. The third patch is not intended for direct inclusion in libabigail but builds on the refactoring to investigate how equality and canonical types work in practice. It identifies some potential discrepancies, but they may be entirely expected. In general, it can be risky to define operator== in a way where reflexivity, symmetry and transitivity do not obviously hold or can be sensitive to small code changes or in way where equality, say for class_decl_sptr, can be affected by something like canonicalisation. More instrumentation could be added to check behaviour. Giuliano Procida (3): abg-ir.cc: Tidy some operator== definitions abg-ir.cc: Refactor operator== methods with helper Add some type equality paranoia. src/abg-ir.cc | 185 +++++++++++++++++++++++++++++--------------------- 1 file changed, 106 insertions(+), 79 deletions(-) -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] abg-ir.cc: Tidy some operator== definitions 2020-07-08 9:53 [PATCH 0/3] Type equality refactor and instrumentation Giuliano Procida @ 2020-07-08 9:53 ` Giuliano Procida 2020-07-08 9:53 ` [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper Giuliano Procida ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Giuliano Procida @ 2020-07-08 9:53 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich Many of the operator== definitions in this source file follow the same pattern: - the address of the argument is dynamic_cast to type of 'this' - naked canonical type pointers are compared, if both present - the types are compared structurally with 'equals' In a couple of cases extra work is done to fetch the canonical type of the definition of a declaration. This commit adjusts a few cases so they more closely follow the common form. This is to make the next refactoring trivial. There are no behavioural changes. * src/abg-irc.cc (scope_type_decl::operator==): Compare naked canonical type pointers instead of the shared pointers. (qualified_type_def::operator==): Remove excess blank line. (function_type::operator==): Do dynamic_cast and check of argument before comparing naked canonical type pointers. (class_or_union::operator==): Eliminate temporary reference. (class_decl::operator==): Likewise. (union_decl::operator==): Likewise. Signed-off-by: Giuliano Procida <gprocida@google.com> --- src/abg-ir.cc | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 37f6bbdf..41e2f00e 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -12872,8 +12872,8 @@ scope_type_decl::operator==(const decl_base& o) const if (!other) return false; - if (get_canonical_type() && other->get_canonical_type()) - return get_canonical_type().get() == other->get_canonical_type().get(); + if (get_naked_canonical_type() && other->get_naked_canonical_type()) + return get_naked_canonical_type() == other->get_naked_canonical_type(); return equals(*this, *other, 0); } @@ -13243,7 +13243,6 @@ qualified_type_def::operator==(const decl_base& o) const if (get_naked_canonical_type() && other->get_naked_canonical_type()) return get_naked_canonical_type() == other->get_naked_canonical_type(); - return equals(*this, *other, 0); } @@ -16723,16 +16722,16 @@ function_type::get_cached_name(bool internal) const bool function_type::operator==(const type_base& other) const { + const function_type* o = dynamic_cast<const function_type*>(&other); + if (!o) + return false; + type_base* canonical_type = get_naked_canonical_type(); type_base* other_canonical_type = other.get_naked_canonical_type(); if (canonical_type && other_canonical_type) return canonical_type == other_canonical_type; - const function_type* o = dynamic_cast<const function_type*>(&other); - if (!o) - return false; - return equals(*this, *o, 0); } @@ -19111,8 +19110,7 @@ class_or_union::operator==(const decl_base& other) const if (canonical_type && other_canonical_type) return canonical_type == other_canonical_type; - const class_or_union& o = *op; - return equals(*this, o, 0); + return equals(*this, *op, 0); } /// Equality operator. @@ -20966,8 +20964,7 @@ class_decl::operator==(const decl_base& other) const if (canonical_type && other_canonical_type) return canonical_type == other_canonical_type; - const class_decl& o = *op; - return equals(*this, o, 0); + return equals(*this, *op, 0); } /// Equality operator for class_decl. @@ -21755,8 +21752,7 @@ union_decl::operator==(const decl_base& other) const if (canonical_type && other_canonical_type) return canonical_type == other_canonical_type; - const union_decl &o = *op; - return equals(*this, o, 0); + return equals(*this, *op, 0); } /// Equality operator for union_decl. -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper 2020-07-08 9:53 [PATCH 0/3] Type equality refactor and instrumentation Giuliano Procida 2020-07-08 9:53 ` [PATCH 1/3] abg-ir.cc: Tidy some operator== definitions Giuliano Procida @ 2020-07-08 9:53 ` Giuliano Procida 2020-07-27 7:56 ` Dodji Seketeli 2020-07-08 9:53 ` [PATCH 3/3] Add some type equality paranoia Giuliano Procida 2020-07-27 7:32 ` [PATCH 0/3] Type equality refactor and instrumentation Dodji Seketeli 3 siblings, 1 reply; 9+ messages in thread From: Giuliano Procida @ 2020-07-08 9:53 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich Many of the operator== definitions in this source file follow the same pattern: - the address of the argument is dynamic_cast to type of 'this' - naked canonical type pointers are compared, if both present - the types are compared structurally with 'equals' In a couple of cases extra work is done to fetch the canonical type of the definition of a declaration. This commit refactors all the common logic into a couple of templated helper functions. There are no behavioural changes. * src/abg-ir.cc (equality_helper): Add an overloaded function to perform the common actions needed for operator==. The first overload takes two extra canonical type pointer arguments while the second obtains these from the types being compared. (type_decl::operator==): Call equality_helper to perform canonical type pointer and 'equals' comparisons. (scope_type_decl::operator==): Likewise. (qualified_type_def::operator==): Likewise. (pointer_type_def::operator==): Likewise. (reference_type_def::operator==): Likewise. (array_type_def::subrange_type::operator==): Likewise. (array_type_def::operator==): Likewise. (enum_type_decl::operator==): Likewise. (typedef_decl::operator==): Likewise. (function_type::operator==): Likewise. (class_or_union::operator==): Likewise. (class_decl::operator==): Likewise. (union_decl::operator==): Likewise. Signed-off-by: Giuliano Procida <gprocida@google.com> --- src/abg-ir.cc | 104 ++++++++++++++------------------------------------ 1 file changed, 29 insertions(+), 75 deletions(-) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 41e2f00e..4b7e180d 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -651,6 +651,22 @@ struct type_name_comp {return operator()(type_base_sptr(l), type_base_sptr(r));} }; // end struct type_name_comp +template<typename T> +bool equality_helper(const T* lptr, const T* rptr, + const type_base* lcanon, + const type_base* rcanon) +{ + return lcanon && rcanon ? lcanon == rcanon : equals(*lptr, *rptr, 0); +} + +template<typename T> +bool equality_helper(const T* lptr, const T* rptr) +{ + return equality_helper(lptr, rptr, + lptr->get_naked_canonical_type(), + rptr->get_naked_canonical_type()); +} + /// Getter of all types types sorted by their pretty representation. /// /// @return a sorted vector of all types sorted by their pretty @@ -12690,11 +12706,7 @@ type_decl::operator==(const decl_base& o) const const type_decl* other = dynamic_cast<const type_decl*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Return true if both types equals. @@ -12871,11 +12883,7 @@ scope_type_decl::operator==(const decl_base& o) const const scope_type_decl* other = dynamic_cast<const scope_type_decl*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Equality operator between two scope_type_decl. @@ -13239,11 +13247,7 @@ qualified_type_def::operator==(const decl_base& o) const dynamic_cast<const qualified_type_def*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Equality operator for qualified types. @@ -13610,14 +13614,7 @@ pointer_type_def::operator==(const decl_base& o) const const pointer_type_def* other = is_pointer_type(&o); if (!other) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Return true iff both instances of pointer_type_def are equal. @@ -13921,14 +13918,7 @@ reference_type_def::operator==(const decl_base& o) const dynamic_cast<const reference_type_def*>(&o); if (!other) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Equality operator of the @ref reference_type_def type. @@ -14470,11 +14460,7 @@ array_type_def::subrange_type::operator==(const decl_base& o) const dynamic_cast<const subrange_type*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Equality operator. @@ -14792,11 +14778,7 @@ array_type_def::operator==(const decl_base& o) const dynamic_cast<const array_type_def*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return equality_helper(this, other); } bool @@ -15272,11 +15254,7 @@ enum_type_decl::operator==(const decl_base& o) const const enum_type_decl* op = dynamic_cast<const enum_type_decl*>(&o); if (!op) return false; - - if (get_naked_canonical_type() && op->get_naked_canonical_type()) - return get_naked_canonical_type() == op->get_naked_canonical_type(); - - return equals(*this, *op, 0); + return equality_helper(this, op); } /// Equality operator. @@ -15626,11 +15604,7 @@ typedef_decl::operator==(const decl_base& o) const const typedef_decl* other = dynamic_cast<const typedef_decl*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return equality_helper(this, other); } /// Equality operator @@ -16725,14 +16699,7 @@ function_type::operator==(const type_base& other) const const function_type* o = dynamic_cast<const function_type*>(&other); if (!o) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other.get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *o, 0); + return equality_helper(this, o); } /// Return a copy of the pretty representation of the current @ref @@ -19107,10 +19074,7 @@ class_or_union::operator==(const decl_base& other) const other_canonical_type = op->get_naked_definition_of_declaration()->get_naked_canonical_type(); - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return equality_helper(this, op, canonical_type, other_canonical_type); } /// Equality operator. @@ -20961,10 +20925,7 @@ class_decl::operator==(const decl_base& other) const other_canonical_type = op->get_naked_definition_of_declaration()->get_naked_canonical_type(); - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return equality_helper(this, op, canonical_type, other_canonical_type); } /// Equality operator for class_decl. @@ -21745,14 +21706,7 @@ union_decl::operator==(const decl_base& other) const const union_decl* op = dynamic_cast<const union_decl*>(&other); if (!op) return false; - - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return equality_helper(this, op); } /// Equality operator for union_decl. -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper 2020-07-08 9:53 ` [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper Giuliano Procida @ 2020-07-27 7:56 ` Dodji Seketeli 2020-07-27 10:36 ` Giuliano Procida 0 siblings, 1 reply; 9+ messages in thread From: Dodji Seketeli @ 2020-07-27 7:56 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich [-- Attachment #1: Type: text/plain, Size: 6193 bytes --] Giuliano Procida <gprocida@google.com> a écrit: > Many of the operator== definitions in this source file follow the same > pattern: > > - the address of the argument is dynamic_cast to type of 'this' > - naked canonical type pointers are compared, if both present > - the types are compared structurally with 'equals' > > In a couple of cases extra work is done to fetch the canonical type > of the definition of a declaration. > > This commit refactors all the common logic into a couple of templated > helper functions. > > There are no behavioural changes. > > * src/abg-ir.cc (equality_helper): Add an overloaded function > to perform the common actions needed for operator==. The first > overload takes two extra canonical type pointer arguments > while the second obtains these from the types being compared. > (type_decl::operator==): Call equality_helper to perform > canonical type pointer and 'equals' comparisons. > (scope_type_decl::operator==): Likewise. > (qualified_type_def::operator==): Likewise. > (pointer_type_def::operator==): Likewise. > (reference_type_def::operator==): Likewise. > (array_type_def::subrange_type::operator==): Likewise. > (array_type_def::operator==): Likewise. > (enum_type_decl::operator==): Likewise. > (typedef_decl::operator==): Likewise. > (function_type::operator==): Likewise. > (class_or_union::operator==): Likewise. > (class_decl::operator==): Likewise. > (union_decl::operator==): Likewise. > > Signed-off-by: Giuliano Procida <gprocida@google.com> > --- > src/abg-ir.cc | 104 ++++++++++++++------------------------------------ > 1 file changed, 29 insertions(+), 75 deletions(-) > > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > index 41e2f00e..4b7e180d 100644 > --- a/src/abg-ir.cc > +++ b/src/abg-ir.cc > @@ -651,6 +651,22 @@ struct type_name_comp > {return operator()(type_base_sptr(l), type_base_sptr(r));} > }; // end struct type_name_comp > > +template<typename T> > +bool equality_helper(const T* lptr, const T* rptr, > + const type_base* lcanon, > + const type_base* rcanon) > +{ > + return lcanon && rcanon ? lcanon == rcanon : equals(*lptr, *rptr, 0); > +} > + > +template<typename T> > +bool equality_helper(const T* lptr, const T* rptr) > +{ As done already throughout the code, the return type of functions should be on their own line, please. The name of the function should start on its own line as well. This makes searching for the definition of the function easy by typing a regular expression like "^equality_helper". Also, to make the code somewhat self documented, I try to have all function names contain a "verb". That forces us to give a name that tells what the function /does/. equality_helper is not quite useful in that respect. Essentially, what the function does is that it tries to compare the types canonically (i.e, using their canonical types) if possible. Otherwise, it falls back to structural comparison. So I'd rather call this something like try_canonical_compare. Last but not least, all function definitions should come documented, please. I have adjusted this accordingly and a patch that you'll find at the end of this message. > + return equality_helper(lptr, rptr, > + lptr->get_naked_canonical_type(), > + rptr->get_naked_canonical_type()); > +} > + > /// Getter of all types types sorted by their pretty representation. > /// > /// @return a sorted vector of all types sorted by their pretty > @@ -12690,11 +12706,7 @@ type_decl::operator==(const decl_base& o) const > const type_decl* other = dynamic_cast<const type_decl*>(&o); > if (!other) > return false; > - > - if (get_naked_canonical_type() && other->get_naked_canonical_type()) Something to keep in mind is that there are times when we need to debug some possibly tricky issues related to type comparison. Especially, we want to see why two types are (for instance) deemed different by libabigail. That is, we want to know why the types have different canonical types. A simple way to know this is to step in the debugger at this point and then make the debugger jump to the "return equals()" line below. That way, we force the code to take structural equality path in the debugger. We can thus step and see why the structural equality code decided that the two types are different (and thus that their canonical types are different). Yours changes in equality_helper are making this important debubbing much more difficult. So I have taken that into account in how I adjusted the try_canonical_compare function. > - return get_naked_canonical_type() == other->get_naked_canonical_type(); > - > - return equals(*this, *other, 0); > + return equality_helper(this, other); > } [...] > /// Return a copy of the pretty representation of the current @ref > @@ -19107,10 +19074,7 @@ class_or_union::operator==(const decl_base& other) const > other_canonical_type = > op->get_naked_definition_of_declaration()->get_naked_canonical_type(); > > - if (canonical_type && other_canonical_type) > - return canonical_type == other_canonical_type; > - > - return equals(*this, *op, 0); > + return equality_helper(this, op, canonical_type, other_canonical_type); By massaging the code above this line, it's possible to call the second overload of equality_helper (just like what all the other spots are doing) and thus do away with the first overload of equality_helper. My amended patch does this. > } > > /// Equality operator. > @@ -20961,10 +20925,7 @@ class_decl::operator==(const decl_base& other) const > other_canonical_type = > op->get_naked_definition_of_declaration()->get_naked_canonical_type(); > > - if (canonical_type && other_canonical_type) > - return canonical_type == other_canonical_type; > - > - return equals(*this, *op, 0); > + return equality_helper(this, op, canonical_type, other_canonical_type); Likewise. > } [...] Here is the amended patch that I'd be for applying. I have also adjusted its commit log. Thanks. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Amended patch --] [-- Type: text/x-patch, Size: 11032 bytes --] From cd06d651b8fca32a8385ff152bae972763e95b26 Mon Sep 17 00:00:00 2001 From: Giuliano Procida <gprocida@google.com> Date: Wed, 8 Jul 2020 10:53:14 +0100 Subject: [PATCH] abg-ir.cc: Refactor operator== methods with helper function Many of the operator== definitions in this source file follow the same pattern: - First, canonical comparison is attempted if canonical types are present. - Otherwise, the comparison is performed structurally using the 'equals' function. This commit refactors the common logic into a templated helper function named "try_canonical_compare". There are no behavioural changes. * src/abg-ir.cc (try_canonical_compare): New template function. (type_decl::operator==): Use it here. (scope_type_decl::operator==): Likewise. (qualified_type_def::operator==): Likewise. (pointer_type_def::operator==): Likewise. (reference_type_def::operator==): Likewise. (array_type_def::subrange_type::operator==): Likewise. (array_type_def::operator==): Likewise. (enum_type_decl::operator==): Likewise. (typedef_decl::operator==): Likewise. (function_type::operator==): Likewise. (class_or_union::operator==): Likewise. (class_decl::operator==): Likewise. (union_decl::operator==): Likewise. Signed-off-by: Giuliano Procida <gprocida@google.com> Signed-off-by: Dodji Seketeli <dodji@redhat.com> --- src/abg-ir.cc | 166 +++++++++++++++++++++------------------------------------- 1 file changed, 59 insertions(+), 107 deletions(-) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index f6eb285..ac837da 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -651,6 +651,25 @@ struct type_name_comp {return operator()(type_base_sptr(l), type_base_sptr(r));} }; // end struct type_name_comp +/// Compare two types by comparing their canonical types if present. +/// +/// If the canonical types are not present (because the types have not +/// yet been canonicalized, for instance) then the types are compared +/// structurally. +/// +/// @param l the first type to take into account in the comparison. +/// +/// @param r the second type to take into account in the comparison. +template<typename T> +bool +try_canonical_compare(const T *l, const T *r) +{ + if (const type_base *lc = l->get_naked_canonical_type()) + if (const type_base *rc = r->get_naked_canonical_type()) + return lc == rc; + return equals(*l, *r, 0); +} + /// Getter of all types types sorted by their pretty representation. /// /// @return a sorted vector of all types sorted by their pretty @@ -12904,11 +12923,7 @@ type_decl::operator==(const decl_base& o) const const type_decl* other = dynamic_cast<const type_decl*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Return true if both types equals. @@ -13085,11 +13100,7 @@ scope_type_decl::operator==(const decl_base& o) const const scope_type_decl* other = dynamic_cast<const scope_type_decl*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator between two scope_type_decl. @@ -13453,11 +13464,7 @@ qualified_type_def::operator==(const decl_base& o) const dynamic_cast<const qualified_type_def*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator for qualified types. @@ -13824,14 +13831,7 @@ pointer_type_def::operator==(const decl_base& o) const const pointer_type_def* other = is_pointer_type(&o); if (!other) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Return true iff both instances of pointer_type_def are equal. @@ -14135,14 +14135,7 @@ reference_type_def::operator==(const decl_base& o) const dynamic_cast<const reference_type_def*>(&o); if (!other) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator of the @ref reference_type_def type. @@ -14684,11 +14677,7 @@ array_type_def::subrange_type::operator==(const decl_base& o) const dynamic_cast<const subrange_type*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator. @@ -15006,11 +14995,7 @@ array_type_def::operator==(const decl_base& o) const dynamic_cast<const array_type_def*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } bool @@ -15485,11 +15470,7 @@ enum_type_decl::operator==(const decl_base& o) const const enum_type_decl* op = dynamic_cast<const enum_type_decl*>(&o); if (!op) return false; - - if (get_naked_canonical_type() && op->get_naked_canonical_type()) - return get_naked_canonical_type() == op->get_naked_canonical_type(); - - return equals(*this, *op, 0); + return try_canonical_compare(this, op); } /// Equality operator. @@ -15839,11 +15820,7 @@ typedef_decl::operator==(const decl_base& o) const const typedef_decl* other = dynamic_cast<const typedef_decl*>(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator @@ -16934,14 +16911,7 @@ function_type::operator==(const type_base& other) const const function_type* o = dynamic_cast<const function_type*>(&other); if (!o) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other.get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *o, 0); + return try_canonical_compare(this, o); } /// Return a copy of the pretty representation of the current @ref @@ -19201,29 +19171,22 @@ class_or_union::operator==(const decl_base& other) const if (!op) return false; - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); - - // If this is a declaration only class with no canonical class, use - // the canonical type of the definition, if any. - if (!canonical_type - && get_is_declaration_only() - && get_naked_definition_of_declaration()) - canonical_type = is_class_or_union_type - (get_naked_definition_of_declaration())->get_naked_canonical_type(); + // If this is a decl-only type (and thus with no canonical type), + // use the canonical type of the definition, if any. + const class_or_union *l = 0; + if (get_is_declaration_only()) + l = dynamic_cast<const class_or_union*>(get_naked_definition_of_declaration()); + if (l == 0) + l = this; // Likewise for the other class. - if (!other_canonical_type - && op->get_is_declaration_only() - && op->get_naked_definition_of_declaration()) - other_canonical_type = - is_class_or_union_type - (op->get_naked_definition_of_declaration())->get_naked_canonical_type(); + const class_or_union *r = 0; + if (op->get_is_declaration_only()) + r = dynamic_cast<const class_or_union*>(op->get_naked_definition_of_declaration()); + if (r == 0) + r = op; - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return try_canonical_compare(l, r); } /// Equality operator. @@ -21032,30 +20995,26 @@ class_decl::operator==(const decl_base& other) const if (!op) return false; - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); + // If this is a decl-only type (and thus with no canonical type), + // use the canonical type of the definition, if any. + const class_decl *l = 0; + if (get_is_declaration_only()) + l = dynamic_cast<const class_decl*>(get_naked_definition_of_declaration()); + if (l == 0) + l = this; - // If this is a declaration only class with no canonical class, use - // the canonical type of the definition, if any. - if (!canonical_type - && get_is_declaration_only() - && get_naked_definition_of_declaration()) - canonical_type = - is_class_type - (get_naked_definition_of_declaration())->get_naked_canonical_type(); + ABG_ASSERT(l); - // Likewise for the other class. - if (!other_canonical_type - && op->get_is_declaration_only() - && op->get_naked_definition_of_declaration()) - other_canonical_type = - is_class_type - (op->get_naked_definition_of_declaration())->get_naked_canonical_type(); + // Likewise for the other type. + const class_decl *r = 0; + if (op->get_is_declaration_only()) + r = dynamic_cast<const class_decl*>(op->get_naked_definition_of_declaration()); + if (r == 0) + r = op; - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; + ABG_ASSERT(r); - return equals(*this, *op, 0); + return try_canonical_compare(l, r); } /// Equality operator for class_decl. @@ -21836,14 +21795,7 @@ union_decl::operator==(const decl_base& other) const const union_decl* op = dynamic_cast<const union_decl*>(&other); if (!op) return false; - - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return try_canonical_compare(this, op); } /// Equality operator for union_decl. -- 1.8.3.1 [-- Attachment #3: Type: text/plain, Size: 13 bytes --] -- Dodji ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper 2020-07-27 7:56 ` Dodji Seketeli @ 2020-07-27 10:36 ` Giuliano Procida 2020-07-27 16:12 ` Dodji Seketeli 0 siblings, 1 reply; 9+ messages in thread From: Giuliano Procida @ 2020-07-27 10:36 UTC (permalink / raw) To: Dodji Seketeli; +Cc: libabigail, kernel-team, Matthias Männich Hi. On Mon, 27 Jul 2020 at 08:56, Dodji Seketeli <dodji@seketeli.org> wrote: > > Giuliano Procida <gprocida@google.com> a écrit: > > > Many of the operator== definitions in this source file follow the same > > pattern: > > > > - the address of the argument is dynamic_cast to type of 'this' > > - naked canonical type pointers are compared, if both present > > - the types are compared structurally with 'equals' > > > > In a couple of cases extra work is done to fetch the canonical type > > of the definition of a declaration. > > > > This commit refactors all the common logic into a couple of templated > > helper functions. > > > > There are no behavioural changes. > > > > * src/abg-ir.cc (equality_helper): Add an overloaded function > > to perform the common actions needed for operator==. The first > > overload takes two extra canonical type pointer arguments > > while the second obtains these from the types being compared. > > (type_decl::operator==): Call equality_helper to perform > > canonical type pointer and 'equals' comparisons. > > (scope_type_decl::operator==): Likewise. > > (qualified_type_def::operator==): Likewise. > > (pointer_type_def::operator==): Likewise. > > (reference_type_def::operator==): Likewise. > > (array_type_def::subrange_type::operator==): Likewise. > > (array_type_def::operator==): Likewise. > > (enum_type_decl::operator==): Likewise. > > (typedef_decl::operator==): Likewise. > > (function_type::operator==): Likewise. > > (class_or_union::operator==): Likewise. > > (class_decl::operator==): Likewise. > > (union_decl::operator==): Likewise. > > > > Signed-off-by: Giuliano Procida <gprocida@google.com> > > --- > > src/abg-ir.cc | 104 ++++++++++++++------------------------------------ > > 1 file changed, 29 insertions(+), 75 deletions(-) > > > > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > > index 41e2f00e..4b7e180d 100644 > > --- a/src/abg-ir.cc > > +++ b/src/abg-ir.cc > > @@ -651,6 +651,22 @@ struct type_name_comp > > {return operator()(type_base_sptr(l), type_base_sptr(r));} > > }; // end struct type_name_comp > > > > +template<typename T> > > +bool equality_helper(const T* lptr, const T* rptr, > > + const type_base* lcanon, > > + const type_base* rcanon) > > +{ > > + return lcanon && rcanon ? lcanon == rcanon : equals(*lptr, *rptr, 0); > > +} > > + > > +template<typename T> > > +bool equality_helper(const T* lptr, const T* rptr) > > +{ > > As done already throughout the code, the return type of functions should > be on their own line, please. Of course. Sorry. > The name of the function should start on its own line as well. This > makes searching for the definition of the function easy by typing a > regular expression like "^equality_helper". > > Also, to make the code somewhat self documented, I try to have all > function names contain a "verb". That forces us to give a name that > tells what the function /does/. equality_helper is not quite useful in > that respect. Essentially, what the function does is that it tries to > compare the types canonically (i.e, using their canonical types) if > possible. Otherwise, it falls back to structural comparison. > > So I'd rather call this something like try_canonical_compare. Yes, the name wasn't ideal. > Last but not least, all function definitions should come documented, > please. Oops. > I have adjusted this accordingly and a patch that you'll find at the end > of this message. > > > + return equality_helper(lptr, rptr, > > + lptr->get_naked_canonical_type(), > > + rptr->get_naked_canonical_type()); > > +} > > + > > /// Getter of all types types sorted by their pretty representation. > > /// > > /// @return a sorted vector of all types sorted by their pretty > > @@ -12690,11 +12706,7 @@ type_decl::operator==(const decl_base& o) const > > const type_decl* other = dynamic_cast<const type_decl*>(&o); > > if (!other) > > return false; > > - > > - if (get_naked_canonical_type() && other->get_naked_canonical_type()) > > Something to keep in mind is that there are times when we need to debug > some possibly tricky issues related to type comparison. Especially, we > want to see why two types are (for instance) deemed different by > libabigail. That is, we want to know why the types have different > canonical types. > > A simple way to know this is to step in the debugger at this point and > then make the debugger jump to the "return equals()" line below. That > way, we force the code to take structural equality path in the > debugger. We can thus step and see why the structural equality code > decided that the two types are different (and thus that their canonical > types are different). > > Yours changes in equality_helper are making this important debubbing > much more difficult. So I have taken that into account in how I > adjusted the try_canonical_compare function. That's a shame. One motivation was to make it easier to perform bulk checks against canonical and structural equality logic. I see your tweaks to a couple of the functions enable a uniform calling path into the helper, allowing the removal of the second overload. > > - return get_naked_canonical_type() == other->get_naked_canonical_type(); > > - > > - return equals(*this, *other, 0); > > + return equality_helper(this, other); > > } > > [...] > > > > /// Return a copy of the pretty representation of the current @ref > > @@ -19107,10 +19074,7 @@ class_or_union::operator==(const decl_base& other) const > > other_canonical_type = > > op->get_naked_definition_of_declaration()->get_naked_canonical_type(); > > > > - if (canonical_type && other_canonical_type) > > - return canonical_type == other_canonical_type; > > - > > - return equals(*this, *op, 0); > > + return equality_helper(this, op, canonical_type, other_canonical_type); > > By massaging the code above this line, it's possible to call the second > overload of equality_helper (just like what all the other spots are > doing) and thus do away with the first overload of equality_helper. My > amended patch does this. > > > } > > > > /// Equality operator. > > @@ -20961,10 +20925,7 @@ class_decl::operator==(const decl_base& other) const > > other_canonical_type = > > op->get_naked_definition_of_declaration()->get_naked_canonical_type(); > > > > - if (canonical_type && other_canonical_type) > > - return canonical_type == other_canonical_type; > > - > > - return equals(*this, *op, 0); > > + return equality_helper(this, op, canonical_type, other_canonical_type); > > Likewise. > > > } > > [...] > > Here is the amended patch that I'd be for applying. I have also > adjusted its commit log. Thanks. > It looks good to me. Giuliano. > > -- > Dodji ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper 2020-07-27 10:36 ` Giuliano Procida @ 2020-07-27 16:12 ` Dodji Seketeli 0 siblings, 0 replies; 9+ messages in thread From: Dodji Seketeli @ 2020-07-27 16:12 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, Matthias Männich Giuliano Procida <gprocida@google.com> a écrit: [...] > It looks good to me. Applied to master, thanks! -- Dodji ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] Add some type equality paranoia. 2020-07-08 9:53 [PATCH 0/3] Type equality refactor and instrumentation Giuliano Procida 2020-07-08 9:53 ` [PATCH 1/3] abg-ir.cc: Tidy some operator== definitions Giuliano Procida 2020-07-08 9:53 ` [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper Giuliano Procida @ 2020-07-08 9:53 ` Giuliano Procida 2020-07-27 7:32 ` [PATCH 0/3] Type equality refactor and instrumentation Dodji Seketeli 3 siblings, 0 replies; 9+ messages in thread From: Giuliano Procida @ 2020-07-08 9:53 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich This commit is an attempt to answer the question "how do canonical types relate to type equality?" and in particular "are they just an optimisation?". It adds some instrumentation to the equality_helper methods. - they take an extra line (__LINE__) argument - both canonical type pointer and plain comparisons are done - comparisons are skipped if the pointers are identical - in-progress comparisons are recorded to avoid infinite recursion - discrepancies are logged The commit also adds some comments to reflect behaviour seen in practice with libabigail's test suite. Note that this is not quite cross-checking canonical pointer comparison with deep equality as the (recursive) equality tests are themselves influenced by canonical pointer comparisons in the direction of considering more things to be equal. Note that thread-local storage is used to avoid corruption of the comparison stack. This is not portable to older compilers in its current form. While the behaviour should be the same, the code runs much more slowly and there can be considerable logging to stderr depending on what you choose to log. Signed-off-by: Giuliano Procida <gprocida@google.com> --- src/abg-ir.cc | 111 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 17 deletions(-) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 4b7e180d..e797a341 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -651,18 +651,81 @@ struct type_name_comp {return operator()(type_base_sptr(l), type_base_sptr(r));} }; // end struct type_name_comp +class hold +{ + public: + hold(const void* l, const void* r) + : p_(std::make_pair(l, r)) + { + st_.push_back(p_); + } + ~hold() + { + ABG_ASSERT(st_.size()); + ABG_ASSERT(st_.back() == p_); + st_.pop_back(); + } + static bool has(const void* l, const void* r) + { + return find(st_.begin(), st_.end(), std::make_pair(l, r)) != st_.end(); + } + private: + const std::pair<const void *, const void *> p_; + thread_local static std::vector<std::pair<const void*, const void*>> st_; +}; + +thread_local std::vector<std::pair<const void*, const void*>> hold::st_; + +const bool debug_equality = true; + template<typename T> -bool equality_helper(const T* lptr, const T* rptr, +bool equality_helper(size_t line, const T* lptr, const T* rptr, const type_base* lcanon, const type_base* rcanon) { - return lcanon && rcanon ? lcanon == rcanon : equals(*lptr, *rptr, 0); + if (debug_equality) + { + // Short cut equality with identical pointers to avoid wasteful deep + // comparisons. + if (lptr == rptr) + return true; + // Record outcome of canonical type pointer comparison. + char c = lcanon ? rcanon ? lcanon == rcanon ? '1' : '0' : 'L' : rcanon ? 'R' : 'X'; + // If we are already holding this pair of type_base pointers we should + // assume they are equal in recursive calls. + bool held = hold::has(lptr, rptr); + bool eq; + if (held) + eq = true; + else + { + hold it(static_cast<const void *>(lptr), + static_cast<const void *>(rptr)); + eq = equals(*lptr, *rptr, 0); + } + // Record outcome of plain comparison. + char p = held ? 'H' : eq ? '1' : '0'; + if (c + p == '0' + '1') + { + // Avoid tearing the output. + std::ostringstream os; + os << line << " canon=" << c << " plain=" << p + << " '" << lptr->get_cached_pretty_representation(true) + << "' '" << rptr->get_cached_pretty_representation(true) << "'\n"; + std::cerr << os.str(); + } + return lcanon && rcanon ? lcanon == rcanon : eq; + } + else + { + return lcanon && rcanon ? lcanon == rcanon : equals(*lptr, *rptr, 0); + } } template<typename T> -bool equality_helper(const T* lptr, const T* rptr) +bool equality_helper(size_t line, const T* lptr, const T* rptr) { - return equality_helper(lptr, rptr, + return equality_helper(line, lptr, rptr, lptr->get_naked_canonical_type(), rptr->get_naked_canonical_type()); } @@ -12706,7 +12769,7 @@ type_decl::operator==(const decl_base& o) const const type_decl* other = dynamic_cast<const type_decl*>(&o); if (!other) return false; - return equality_helper(this, other); + return equality_helper(__LINE__, this, other); } /// Return true if both types equals. @@ -12883,7 +12946,7 @@ scope_type_decl::operator==(const decl_base& o) const const scope_type_decl* other = dynamic_cast<const scope_type_decl*>(&o); if (!other) return false; - return equality_helper(this, other); + return equality_helper(__LINE__, this, other); } /// Equality operator between two scope_type_decl. @@ -13247,7 +13310,7 @@ qualified_type_def::operator==(const decl_base& o) const dynamic_cast<const qualified_type_def*>(&o); if (!other) return false; - return equality_helper(this, other); + return equality_helper(__LINE__, this, other); } /// Equality operator for qualified types. @@ -13614,7 +13677,8 @@ pointer_type_def::operator==(const decl_base& o) const const pointer_type_def* other = is_pointer_type(&o); if (!other) return false; - return equality_helper(this, other); + // Sometimes canonically distinct but compare as equal. + return equality_helper(__LINE__, this, other); } /// Return true iff both instances of pointer_type_def are equal. @@ -13918,7 +13982,7 @@ reference_type_def::operator==(const decl_base& o) const dynamic_cast<const reference_type_def*>(&o); if (!other) return false; - return equality_helper(this, other); + return equality_helper(__LINE__, this, other); } /// Equality operator of the @ref reference_type_def type. @@ -14460,7 +14524,7 @@ array_type_def::subrange_type::operator==(const decl_base& o) const dynamic_cast<const subrange_type*>(&o); if (!other) return false; - return equality_helper(this, other); + return equality_helper(__LINE__, this, other); } /// Equality operator. @@ -14778,7 +14842,8 @@ array_type_def::operator==(const decl_base& o) const dynamic_cast<const array_type_def*>(&o); if (!other) return false; - return equality_helper(this, other); + // Sometimes canonically distinct but compare as equal. + return equality_helper(__LINE__, this, other); } bool @@ -15254,7 +15319,8 @@ enum_type_decl::operator==(const decl_base& o) const const enum_type_decl* op = dynamic_cast<const enum_type_decl*>(&o); if (!op) return false; - return equality_helper(this, op); + // Sometimes canonically distinct but compare as equal. + return equality_helper(__LINE__, this, op); } /// Equality operator. @@ -15604,7 +15670,9 @@ typedef_decl::operator==(const decl_base& o) const const typedef_decl* other = dynamic_cast<const typedef_decl*>(&o); if (!other) return false; - return equality_helper(this, other); + // Sometimes canonically distinct but compare as equal. + // Sometimes canonically equal but compare as distinct. + return equality_helper(__LINE__, this, other); } /// Equality operator @@ -16699,7 +16767,9 @@ function_type::operator==(const type_base& other) const const function_type* o = dynamic_cast<const function_type*>(&other); if (!o) return false; - return equality_helper(this, o); + // Sometimes canonically distinct but compare as equal. + // Sometimes canonically equal but compare as distinct. + return equality_helper(__LINE__, this, o); } /// Return a copy of the pretty representation of the current @ref @@ -19074,7 +19144,9 @@ class_or_union::operator==(const decl_base& other) const other_canonical_type = op->get_naked_definition_of_declaration()->get_naked_canonical_type(); - return equality_helper(this, op, canonical_type, other_canonical_type); + // Or should this just be definition_of_declaration equality? + // Sometimes canonically equal but compare as distinct. + return equality_helper(__LINE__, this, op, canonical_type, other_canonical_type); } /// Equality operator. @@ -20925,7 +20997,10 @@ class_decl::operator==(const decl_base& other) const other_canonical_type = op->get_naked_definition_of_declaration()->get_naked_canonical_type(); - return equality_helper(this, op, canonical_type, other_canonical_type); + // Or should this just be definition_of_declaration equality? + // Sometimes canonically distinct but compare as equal. + // Sometimes canonically equal but compare as distinct. + return equality_helper(__LINE__, this, op, canonical_type, other_canonical_type); } /// Equality operator for class_decl. @@ -21706,7 +21781,9 @@ union_decl::operator==(const decl_base& other) const const union_decl* op = dynamic_cast<const union_decl*>(&other); if (!op) return false; - return equality_helper(this, op); + // Sometimes canonically distinct but compare as equal. + // Sometimes canonically equal but compare as distinct. + return equality_helper(__LINE__, this, op); } /// Equality operator for union_decl. -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Type equality refactor and instrumentation 2020-07-08 9:53 [PATCH 0/3] Type equality refactor and instrumentation Giuliano Procida ` (2 preceding siblings ...) 2020-07-08 9:53 ` [PATCH 3/3] Add some type equality paranoia Giuliano Procida @ 2020-07-27 7:32 ` Dodji Seketeli 2020-07-27 10:55 ` Giuliano Procida 3 siblings, 1 reply; 9+ messages in thread From: Dodji Seketeli @ 2020-07-27 7:32 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich Hello Giuliano, Giuliano Procida <gprocida@google.com> a écrit: > Hi Dodji. > > This series refactors various operator== methods making their common > functionality evident. > > The first patch is just a prelude to make the second smaller. > > The second patch does the refactoring. I'm not attached to the name > 'equality_helper'. Thank you for working on this. I have looked into these first two patches and they look good to me in general. I'll post their individual review as usual, shortly. > The third patch is not intended for direct inclusion in libabigail but > builds on the refactoring to investigate how equality and canonical > types work in practice. It identifies some potential discrepancies, > but they may be entirely expected. In it's current form, I'd rather hold the inclusion of this one for now. It's super intrusive, clutters the code quite a bit and I am not really sure about its practical use at the moment. > In general, it can be risky to define operator== in a way where > reflexivity, symmetry and transitivity do not obviously hold or can be > sensitive to small code changes or in way where equality, say for > class_decl_sptr, can be affected by something like canonicalisation. > More instrumentation could be added to check behaviour. The comparison code is tricky. The number one reason for this is that it has to be fast. So yes, it's risky. One category of tests that are "simple" to perform and in practise are quite powerful to detect and debug potential comparison issues are "identity tests". That is, comparing a binary against itself and requiring that the result be the empty set. This is why the option "--abidiff" was added to abidw. So I tend to favor schemes that keep the code the less cluttered and the most "debuggable" as possible, as opposed to adding a lot of instrumentation. I guess some balance has to found there. Cheers, -- Dodji ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Type equality refactor and instrumentation 2020-07-27 7:32 ` [PATCH 0/3] Type equality refactor and instrumentation Dodji Seketeli @ 2020-07-27 10:55 ` Giuliano Procida 0 siblings, 0 replies; 9+ messages in thread From: Giuliano Procida @ 2020-07-27 10:55 UTC (permalink / raw) To: Dodji Seketeli; +Cc: libabigail, kernel-team, Matthias Männich Hi Dodji. On Mon, 27 Jul 2020 at 08:33, Dodji Seketeli <dodji@seketeli.org> wrote: > > Hello Giuliano, > > Giuliano Procida <gprocida@google.com> a écrit: > > > Hi Dodji. > > > > This series refactors various operator== methods making their common > > functionality evident. > > > > The first patch is just a prelude to make the second smaller. > > > > The second patch does the refactoring. I'm not attached to the name > > 'equality_helper'. > > Thank you for working on this. > > I have looked into these first two patches and they look good to me in general. > I'll post their individual review as usual, shortly. > > > The third patch is not intended for direct inclusion in libabigail but > > builds on the refactoring to investigate how equality and canonical > > types work in practice. It identifies some potential discrepancies, > > but they may be entirely expected. > > In it's current form, I'd rather hold the inclusion of this one for now. > It's super intrusive, clutters the code quite a bit and I am not really > sure about its practical use at the moment. The code is absolutely not intended to be committed to master. On top of the obvious impact on the code, tests were still running after 60h with this turned on! > > In general, it can be risky to define operator== in a way where > > reflexivity, symmetry and transitivity do not obviously hold or can be > > sensitive to small code changes or in way where equality, say for > > class_decl_sptr, can be affected by something like canonicalisation. > > More instrumentation could be added to check behaviour. > > The comparison code is tricky. The number one reason for this is that > it has to be fast. So yes, it's risky. > > One category of tests that are "simple" to perform and in practise are > quite powerful to detect and debug potential comparison issues are > "identity tests". That is, comparing a binary against itself and > requiring that the result be the empty set. This is why the option > "--abidiff" was added to abidw. > > So I tend to favor schemes that keep the code the less cluttered and the > most "debuggable" as possible, as opposed to adding a lot of > instrumentation. I guess some balance has to found there. This was intended for one-off (or occasional) instrumentation. Running with the instrumentation, I noticed various places where canonical and structural comparison differ. If the comparisons are happening during canonicalisation then things may be temporarily off. Or I may have a bug in the instrumentation. At any rate, the commit adds code comments showing where certain discrepancies were noted during "make check". For example, function_type nodes sometimes compare as canonically equal but not "structurally" and vice versa. "Structurally" is not really fully structurally as recursive calls attempt canonical comparison first. If there are any you think are of concern, let me know and I can investigate further and open a bug with findings. I've reworked my series to account for your version of the second commit. The instrumentation commit can be found amongst the changes in https://github.com/myxoid/libabigail/commits/type-equality-paranoia Regards, Giuliano. > Cheers, > > -- > Dodji ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-27 16:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-08 9:53 [PATCH 0/3] Type equality refactor and instrumentation Giuliano Procida 2020-07-08 9:53 ` [PATCH 1/3] abg-ir.cc: Tidy some operator== definitions Giuliano Procida 2020-07-08 9:53 ` [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper Giuliano Procida 2020-07-27 7:56 ` Dodji Seketeli 2020-07-27 10:36 ` Giuliano Procida 2020-07-27 16:12 ` Dodji Seketeli 2020-07-08 9:53 ` [PATCH 3/3] Add some type equality paranoia Giuliano Procida 2020-07-27 7:32 ` [PATCH 0/3] Type equality refactor and instrumentation Dodji Seketeli 2020-07-27 10:55 ` Giuliano Procida
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).