From patchwork Tue Oct 13 10:18:38 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 40710 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 554B23840C3C; Tue, 13 Oct 2020 10:18:47 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 554B23840C3C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1602584327; bh=8zxxbEX3V3Y0TimCztB0TpCFTZ0DjnbuECpV0jfNedE=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Help: List-Subscribe:From:Reply-To:From; b=pJnzQf/QZJQXWy7NEQ7fQ5oOy8Ma/jZVR9yRkWX9JVeHOrBuFhFpWfHMbvWAwwiNk ITM79mHyd0ID5dF4lF/OYQQvwIbech/7YecM/oCPXXDoF1X4QTSH6+EFxRm17S6Xnx JaHoLFlSjfJ/2fb6m5u7OqkH/+URIcAajsZdExBQ= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id A51DB38708E3 for ; Tue, 13 Oct 2020 10:18:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A51DB38708E3 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-353-aNODA_xyNdOK11abfsoClA-1; Tue, 13 Oct 2020 06:18:42 -0400 X-MC-Unique: aNODA_xyNdOK11abfsoClA-1 Received: by mail-wr1-f69.google.com with SMTP id x16so10231737wrg.7 for ; Tue, 13 Oct 2020 03:18:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:organization:cc:date:message-id :user-agent:mime-version; bh=8zxxbEX3V3Y0TimCztB0TpCFTZ0DjnbuECpV0jfNedE=; b=fw8L+vPzuWe2Juqj7PLLwjSKs5zy62cMexoPU9cuJIwdFAgJQxKmyuJqptWhSHRnOw IOzGjJyMM9DW4oMy9RmUN39jGhJm3H7pCHiB/sFR6Qd6vHVhM9r+TU7StxZzrIV2rnDz lVhAh8sXxG/W2zxYpjMd8UU0BvvPez4rznJ4xt6SLCFkPzfE45VX66PLpb13UJLAvNNO 1BbWTIu1rGyyGc6OLn0HqHre8sJ/utOiwE63mhIwe52Nx9f+ks8lcufcIUk8Ipt8tENI UAJMxQDCYY/hJSwt8TWcPWKgecULkzHw2XhhOG0oZ5m1291T+YV+CmTWbXG8zBvHLzsN kAFg== X-Gm-Message-State: AOAM5302oK/MagIj6T9lZl4G+dRLZGgWw9h7EwrFnueFtob4DtDVkXDC jYxmwFrBD/JVNkqNqcPgWE8SxdH8K3lXqbWc6SIPvxCtfqGch7o8QqqTt9edU++H3k8gK40gSkj QRyi/jT/7FpjPqnzSnksC X-Received: by 2002:a1c:4c03:: with SMTP id z3mr14424482wmf.24.1602584320653; Tue, 13 Oct 2020 03:18:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyA4XR0viewER9XDLWnUyV4TGwMCaR5dhngZRubfy2t/x4ayNbUCdFWiXtJ4ixOHy9bTqaFwQ== X-Received: by 2002:a1c:4c03:: with SMTP id z3mr14424459wmf.24.1602584320326; Tue, 13 Oct 2020 03:18:40 -0700 (PDT) Received: from localhost (91-166-131-130.subs.proxad.net. [91.166.131.130]) by smtp.gmail.com with ESMTPSA id o3sm25153843wru.15.2020.10.13.03.18.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 13 Oct 2020 03:18:39 -0700 (PDT) Received: by localhost (Postfix, from userid 1000) id 7FB535800FC; Tue, 13 Oct 2020 12:18:38 +0200 (CEST) To: libabigail@sourceware.org Subject: [PATCH] Structurally compare the few non-canonicalized types in general Organization: Red Hat / France X-Operating-System: Fedora 34 X-URL: http://www.redhat.com Date: Tue, 13 Oct 2020 12:18:38 +0200 Message-ID: <875z7ebf9d.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP 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: Dodji Seketeli via Libabigail From: Dodji Seketeli Reply-To: Dodji Seketeli Errors-To: libabigail-bounces@sourceware.org Sender: "Libabigail" Hello, In the abixml writer, we recently stopped using type_base::dynamic_hash as a slow path to hash a non-canonicalized type. Rather, we use an arbitrary constant as a hash for those non-canonicalized types. This amounts to using structural comparison for those types. The function that implements this hashing scheme is hash_as_canonical_type_or_constant. A potential concern for that approach was the possible negative impact on speed. As it turns out since the change went in, there was no noticeable speed impact raised after testing. Which is great! In insight, this is understandable as the number of non-canonicalized types should be extremely reduced now that we canonicalize pretty much all types. As far as I understand it at this point, the only types that would not be canonicalized are unresolved declaration-only types. And, all in all, structurally comparing unresolved declaration-only types should be "fast enough". If we ever stumble across any other non-canonicalized type, I think that would be an artifact of a bug that ought to be fixed. On that basis, I went ahead and used hash_as_canonical_type_or_constant throughout the code base and did away with the use of type_base::dynamic_hash for now, until it's properly audited, regression tested and got ready for the use cases where it might make sense. This patch thus makes hash_type use hash_as_canonical_type_or_constant. The writer is then back to using hash_type again, as it used to; but at the same time, it's still using structural comparison for non-canonilized types. So is hash_type_or_decl, which now uses hash_type to hash types, rather than using type_base::dynamic_hash. Note that the comparison engine heavily uses hash_type_or_decl to hash diff nodes. So with this small patch the comparison engine is now using structural comparison of non-canonicalized types (and diff nodes), just as the abixml writer does. * src/abg-hash.cc (type_base::dynamic_hash::operator()): Add a comment. * src/abg-ir.cc (hash_type_or_decl): Use hash_type for types. * src/abg-writer.cc (type_hasher::operator()): Use hash_type. Signed-off-by: Dodji Seketeli --- src/abg-hash.cc | 5 ++++ src/abg-ir.cc | 67 ++--------------------------------------------- src/abg-writer.cc | 2 +- 3 files changed, 8 insertions(+), 66 deletions(-) diff --git a/src/abg-hash.cc b/src/abg-hash.cc index 5d2861f3..ca67689c 100644 --- a/src/abg-hash.cc +++ b/src/abg-hash.cc @@ -991,6 +991,11 @@ operator()(const shared_ptr t) const /// inheritance hierarchy, make sure to handle the most derived type /// first. /// +/// FIXME: This hashing function is not maintained and is surely +/// broken in subtle ways. In pratice, the various *::hash functors +/// should be audited before they are used here. They should all +/// match what is done in the 'equals' functions in abg-ir.cc. +/// /// @param t a pointer to the type declaration to be hashed /// /// @return the resulting hash diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 24f42e54..1b99c18f 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -22977,36 +22977,7 @@ hash_type_or_decl(const type_or_decl_base *tod) if (tod == 0) ; else if (const type_base* t = is_type(tod)) - { - // If the type has a canonical type, then use the pointer value - // as a hash. This is the fastest we can get. - if (type_base* ct = t->get_naked_canonical_type()) - result = reinterpret_cast(ct); - else if (const class_decl* cl = is_class_type(t)) - { - if (cl->get_is_declaration_only() - && cl->get_naked_definition_of_declaration()) - // The is a declaration-only class, so it has no canonical - // type; but then it's class definition has one. Let's - // use that one. - return hash_type_or_decl(cl->get_naked_definition_of_declaration()); - else - { - // The class really has no canonical type, let's use the - // slow path of hashing the class recursively. Well - // it's not that slow as the hash value is quickly going - // to result to zero anyway. - type_base::dynamic_hash hash; - result = hash(t); - } - } - else - { - // Let's use the slow path of hashing the class recursively. - type_base::dynamic_hash hash; - result = hash(t); - } - } + result = hash_type(t); else if (const decl_base* d = is_decl(tod)) { if (var_decl* v = is_var_decl(d)) @@ -23079,41 +23050,7 @@ hash_type_or_decl(const type_or_decl_base *tod) /// @return the resulting hash value. size_t hash_type(const type_base *t) -{ - size_t result = 0; - - // If the type has a canonical type, then use the pointer value - // as a hash. This is the fastest we can get. - if (type_base* ct = t->get_naked_canonical_type()) - result = reinterpret_cast(ct); - else if (const class_decl* cl = is_class_type(t)) - { - if (cl->get_is_declaration_only() - && cl->get_naked_definition_of_declaration()) - // The is a declaration-only class, so it has no canonical - // type; but then it's class definition has one. Let's - // use that one. - return hash_type - (is_class_type(cl->get_naked_definition_of_declaration())); - else - { - // The class really has no canonical type, let's use the - // slow path of hashing the class recursively. Well - // it's not that slow as the hash value is quickly going - // to result to zero anyway. - type_base::dynamic_hash hash; - result = hash(t); - } - } - else - { - // Let's use the slow path of hashing the class recursively. - type_base::dynamic_hash hash; - result = hash(t); - } - - return result; -} +{return hash_as_canonical_type_or_constant(t);} /// Hash an ABI artifact that is either a type of a decl. /// diff --git a/src/abg-writer.cc b/src/abg-writer.cc index 59060e10..4c751c26 100644 --- a/src/abg-writer.cc +++ b/src/abg-writer.cc @@ -136,7 +136,7 @@ struct type_hasher { size_t operator()(const type_base* t) const - {return hash_as_canonical_type_or_constant(t);} + {return hash_type(t);} }; // end struct type_hasher /// A convenience typedef for a map that associates a pointer to type