From patchwork Wed Jul 8 09:53:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 39955 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3F54A386181A; Wed, 8 Jul 2020 09:53:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3F54A386181A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1594202014; bh=vU8A7TrGZnk5CMMiYxkmPIYdHSdcecAfXMMAIjWkPqU=; h=Date:In-Reply-To:References:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Help:List-Subscribe:From:Reply-To:Cc:From; b=BASMScqCoQ/BiGEAh4g+Oy8avmcKrNyW82wAwVCspz8ocXjIJf4nweabdGmTQUbDg 4V8g4EGJ2wzYp0wfj4zz/eeC3RcTJZIgr/8VNQCG2fF+hrpaMfvXDPBJU2je9we11b mTZqrO1rO8ETF0vCgyzRplxJrZFr2D4HYdQfV4jU= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mail-wr1-x44a.google.com (mail-wr1-x44a.google.com [IPv6:2a00:1450:4864:20::44a]) by sourceware.org (Postfix) with ESMTPS id F09A43861834 for ; Wed, 8 Jul 2020 09:53:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F09A43861834 Received: by mail-wr1-x44a.google.com with SMTP id v3so21703933wrq.10 for ; Wed, 08 Jul 2020 02:53:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=vU8A7TrGZnk5CMMiYxkmPIYdHSdcecAfXMMAIjWkPqU=; b=NacI15FuT76hXm80rsbxiTzib9/BHgF+5+pgA1kuwhFj5f9Gs6EevCPD922++LwoiX aAuDiGNXe6DhM0XUVdidbK/wmYA51rI1gIyEgHk8OAIP+s6LFRB8H0XULyw3vZcSrdCJ +OPl+bk6w4Gd20cJMxt8iPsv4uyIW8Mn/ZzdC3ZxnNL3VcFO1AkQmWreGzk1rfbnHfT3 VI4C9/U3+8kQh3cxHpW9m9Z3g79mpvTV4MmrBeSaOJYD7Sn0ZiMWkYMLjwIlutLeu+KO 4L7jIwczKS4cHjr3RNcesajyEa8Efo31WTmLRhafY2Lrq/dX0eCpouRoAwgHAQyLe1Kw eSeA== X-Gm-Message-State: AOAM5319HDo4gZKtd+QIuYC/MxGsqQk+g60Rv2/lOPd71VJ8XtAWKtiP DleeYbXxuntbF339S0rCMFWWpjNXJFdwZ8d5BHiwElAoxpQdvahkCjnpHSiJ/CnZm3H2beKAGhp NUyg4ClFMOT4ueE7YQrPW8bcRLf1hdQ4NJs31ISpBmoR0I8biYqGVl01iWbQMXtcUB/2+wHY= X-Google-Smtp-Source: ABdhPJwSJjryicZyS3Kcaze1YSislYKIy6pEl5SzKXM1/WiBr93HYIDrHlXlp/YKRiaGXqG/xcyAv5oclkxT2Q== X-Received: by 2002:adf:8521:: with SMTP id 30mr54870367wrh.238.1594202010893; Wed, 08 Jul 2020 02:53:30 -0700 (PDT) Date: Wed, 8 Jul 2020 10:53:15 +0100 In-Reply-To: <20200708095315.948634-1-gprocida@google.com> Message-Id: <20200708095315.948634-4-gprocida@google.com> Mime-Version: 1.0 References: <20200708095315.948634-1-gprocida@google.com> X-Mailer: git-send-email 2.27.0.383.g050319c2ae-goog Subject: [PATCH 3/3] Add some type equality paranoia. To: libabigail@sourceware.org X-Spam-Status: No, score=-23.3 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-Patchwork-Original-From: Giuliano Procida via Libabigail From: Giuliano Procida Reply-To: Giuliano Procida Cc: maennich@google.com, kernel-team@android.com Errors-To: libabigail-bounces@sourceware.org Sender: "Libabigail" 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 --- 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 p_; + thread_local static std::vector> st_; +}; + +thread_local std::vector> hold::st_; + +const bool debug_equality = true; + template -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(lptr), + static_cast(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 -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(&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(&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(&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(&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(&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(&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(&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(&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(&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(&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.