From patchwork Mon Jul 4 12:50:42 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 55697 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 37234385C319 for ; Mon, 4 Jul 2022 12:50:50 +0000 (GMT) X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by sourceware.org (Postfix) with ESMTPS id 1B554385BAEA for ; Mon, 4 Jul 2022 12:50:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1B554385BAEA Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by mail.gandi.net (Postfix) with ESMTPSA id 84B20E0004; Mon, 4 Jul 2022 12:50:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seketeli.org; s=gm1; t=1656939043; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type; bh=4lrfR/GmG6aYg5a9CZjcrJVkVvsiNLRNPoqw0aMIrKM=; b=XZKy5GQPcjzs5W1ZEKOpyjJcbl8k+DdXexFXUIpxM2TVMhOZFGTYxq6HGE8qGKNvi0b2Y9 wFP6tP9ZHvjiSsxtMg4xoA9NPB4xZYwieKUSQVFDBBsFYiWnVodEAs5BpuQ80BkOrgAt/6 UOFoq1LfhbiS6vlWPzbLQJSnksR1Qt2Xx+KZ23z6jPb/KFOq7YfHK025DPCn8GM2MkgrHP q5L3p6iisBtdEVVMQaS5+MtY9GZ3524Rw85qPC2T6DBXGLqGhU8VNpdltXklNyN71zExG7 jNdzbPv+HyN+FCXmaTExr/eg9+9mR0zD6DXXq+R4JljJXvnzJc6ixz5nZzImdQ== Received: by localhost (Postfix, from userid 1000) id A0A965800FC; Mon, 4 Jul 2022 14:50:42 +0200 (CEST) From: Dodji Seketeli To: libabigail@sourceware.org Subject: [PATCH, applied] dwarf-reader,ir: Don't canonicalize enums too early & too naively Organization: Me, myself and I X-Operating-System: Fedora 37 X-URL: http://www.seketeli.net/~dodji Date: Mon, 04 Jul 2022 14:50:42 +0200 Message-ID: <87a69pca8t.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: , Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" Hello, When looking at several self comparison failures[1], I notice that the DWARF reader was early-canonicalizing enum types. So, sometimes, when there are declaration-only enum types that need to be resolved (later) to their proper definition, by the time we reach read_context::resolve_declaration_only_enums, canonicalization is already done and so we fail to resolve the decl-only enum; in that case, the decl-only enum is later wrongly considered as different from its definition, leading to spurious errors down the road. This patch thus delays canonicalizing of enum types from the DWARF reader. Once that is done, the patch fixes the comparison of enum types to look through decl-only enum types and compare their definitions instead. The patch also look through decl-only enums during canonicalization. [1]: The self comparison failures could be reproduced by the commands: $ tools/fedabipkgdiff --debug --abipkgdiff build/tools/abipkgdiff --self-compare -a --from fc36 dovecot $ tools/fedabipkgdiff --debug --abipkgdiff build/tools/abipkgdiff --self-compare -a --from fc36 btrfs-progs * src/abg-dwarf-reader.cc (maybe_canonicalize_type): Delay enum type canonicalization. * src/abg-ir.cc (type_base::get_canonical_type_for): Look through all decl-only types, not just decl-only classes. (equals): In the overload for enums, look through decl-only enums. Also, fix redundant enumerators detection to make it more robust, otherwise, some regression tests break. Signed-off-by: Dodji Seketeli --- src/abg-dwarf-reader.cc | 2 + src/abg-ir.cc | 96 ++++++++++++++++++++++++++++++++--------- 2 files changed, 77 insertions(+), 21 deletions(-) diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc index 3b75ddf1..32a2cead 100644 --- a/src/abg-dwarf-reader.cc +++ b/src/abg-dwarf-reader.cc @@ -15288,6 +15288,7 @@ maybe_canonicalize_type(const Dwarf_Die *die, read_context& ctxt) || is_function_type(peeled_type) || is_array_type(peeled_type) || is_qualified_type(peeled_type) + || is_enum_type(peeled_type) || is_typedef(t)) // We delay canonicalization of classes/unions or typedef, // pointers, references and array to classes/unions. This is @@ -15344,6 +15345,7 @@ maybe_canonicalize_type(const type_base_sptr& t, || is_function_type(peeled_type) || is_array_type(peeled_type) || is_qualified_type(peeled_type) + || is_enum_type(peeled_type) ||(is_decl(peeled_type) && is_decl(peeled_type)->get_is_anonymous())) // We delay canonicalization of classes/unions or typedef, // pointers, references and array to classes/unions. This is diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 0ba595a8..8f924505 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -13830,28 +13830,27 @@ type_base::get_canonical_type_for(type_base_sptr t) // This type should not be canonicalized! return type_base_sptr(); + if (is_decl(t)) + t = is_type(look_through_decl_only(is_decl(t))); + + // Look through decl-only types (classes, unions and enums) bool decl_only_class_equals_definition = (odr_is_relevant(*t) || env->decl_only_class_equals_definition()); class_or_union_sptr class_or_union = is_class_or_union_type(t); - // Look through declaration-only classes when we are dealing with - // C++ or languages where we assume the "One Definition Rule". In - // that context, we assume that a declaration-only non-anonymous - // class equals all fully defined classes of the same name. + // In the context of types from C++ or languages where we assume the + // "One Definition Rule", we assume that a declaration-only + // non-anonymous class equals all fully defined classes of the same + // name. // // Otherwise, all classes, including declaration-only classes are // canonicalized and only canonical comparison is going to be used // in the system. if (decl_only_class_equals_definition) if (class_or_union) - { - class_or_union = look_through_decl_only_class(class_or_union); - if (class_or_union->get_is_declaration_only()) - return type_base_sptr(); - else - t = class_or_union; - } + if (class_or_union->get_is_declaration_only()) + return type_base_sptr(); class_decl_sptr is_class = is_class_type(t); if (t->get_canonical_type()) @@ -17694,8 +17693,25 @@ bool equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k) { bool result = true; - if (*l.get_underlying_type() != *r.get_underlying_type()) + + // + // Look through decl-only-enum. + // + + const enum_type_decl *def1 = + l.get_is_declaration_only() + ? is_enum_type(l.get_naked_definition_of_declaration()) + : &l; + + const enum_type_decl *def2 = + r.get_is_declaration_only() + ? is_enum_type(r.get_naked_definition_of_declaration()) + : &r; + + if (!!def1 != !!def2) { + // One enum is decl-only while the other is not. + // So the two enums are different. result = false; if (k) *k |= SUBTYPE_CHANGE_KIND; @@ -17703,14 +17719,34 @@ equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k) ABG_RETURN_FALSE; } - if (!(l.decl_base::operator==(r) && l.type_base::operator==(r))) + // + // At this point, both enums have the same state of decl-only-ness. + // So we can compare oranges to oranges. + // + + if (!def1) + def1 = &l; + if (!def2) + def2 = &r; + + if (def1->get_underlying_type() != def2->get_underlying_type()) + { + result = false; + if (k) + *k |= SUBTYPE_CHANGE_KIND; + else + ABG_RETURN_FALSE; + } + + if (!(def1->decl_base::operator==(*def2) + && def1->type_base::operator==(*def2))) { result = false; if (k) { - if (!l.decl_base::operator==(r)) + if (!def1->decl_base::operator==(*def2)) *k |= LOCAL_NON_TYPE_CHANGE_KIND; - if (!l.type_base::operator==(r)) + if (!def1->type_base::operator==(*def2)) *k |= LOCAL_TYPE_CHANGE_KIND; } else @@ -17741,11 +17777,28 @@ equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k) // // of the enumerator e1. // }; // + // Note however that in the case below, the enums are different. + // + // enum foo + // { + // e0 = 0; + // e1 = 1; + // }; + // + // enum foo + // { + // e0 = 0; + // e2 = 1; // <-- this enum value is present in the first version + // // of foo, but is not redundant with any enumerator + // // in the second version of of enum foo. + // }; + // // These two enums are considered equal. - for(const auto &e : l.get_enumerators()) - if (!is_enumerator_present_in_enum(e, r) - && !is_enumerator_value_redundant(e, r)) + for(const auto &e : def1->get_enumerators()) + if (!is_enumerator_present_in_enum(e, *def2) + && (!is_enumerator_value_redundant(e, *def2) + || !is_enumerator_value_redundant(e, *def1))) { result = false; if (k) @@ -17757,9 +17810,10 @@ equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k) ABG_RETURN_FALSE; } - for(const auto &e : r.get_enumerators()) - if (!is_enumerator_present_in_enum(e, l) - && !is_enumerator_value_redundant(e, r)) + for(const auto &e : def2->get_enumerators()) + if (!is_enumerator_present_in_enum(e, *def1) + && (!is_enumerator_value_redundant(e, *def1) + || !is_enumerator_value_redundant(e, *def2))) { result = false; if (k)