From patchwork Wed Mar 22 16:12:55 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 66747 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 AE1CF385B50D for ; Wed, 22 Mar 2023 16:13:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org AE1CF385B50D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1679501588; bh=5ODrEKypg5+F4Gnoas/v4gPN2uP3mnrOgjo/XwUtyf0=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Help: List-Subscribe:From:Reply-To:From; b=PBLp8tvgAhzf0OFiJPWRDibDce2EMgWWjBquQaaVaDpIWtcEaxpAxUvd49vdmNSZZ 9W1EXBK/2Lv9gIsKGKQTaBAZyL6rgEJ8VcLuHZYIjIVXqiOWN7gLxCorfRiRvXGtzN Moib5/W0ZxkJQ3BeXWiNk0xv+zMsvrTmpYkBUaaY= 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 [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 0B262385B506 for ; Wed, 22 Mar 2023 16:13:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0B262385B506 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-376-tJHlaOc7M8-raN1swTVM4A-1; Wed, 22 Mar 2023 12:13:00 -0400 X-MC-Unique: tJHlaOc7M8-raN1swTVM4A-1 Received: by mail-qt1-f199.google.com with SMTP id u1-20020a05622a198100b003e12a0467easo6102013qtc.11 for ; Wed, 22 Mar 2023 09:13:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679501579; h=mime-version:user-agent:message-id:date:organization:subject:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5ODrEKypg5+F4Gnoas/v4gPN2uP3mnrOgjo/XwUtyf0=; b=YPfShhV2nXUCWcaUvA3GXBFjJ5Db/AzGv6aCPWUIb5i05pMbbGf96S6nYSur6gt9P6 VSyuxVxgY4aeoVn+SAxZubUK7bp5MaR0Z82vr8dig9bvxgPiA9QRGwDLrGDUVlaTsZOv fPxYGSMx1K9jgGd7ZT2GHYBWG/oost9ogugy11Y2PjWu4j5n94SqddOd3LaBa29+DgIw p2uUFEOhVDuuvrzLM64XM1nctbTJG91OCkIM3BGA3EXG1cEWp8Pprjk2LuLipDWFb7cq Nx2ijjnW2xMNuH8JACJR3Ju9DC7oy+l+u+/b6Wrhsb+P2t2AFeqAJvghlt3nT+m0Iq0U 9e7Q== X-Gm-Message-State: AO0yUKXvn2Gwy31tCQra0a7Jub71CwPZ7/ZCz+Kl6qHWvXvrTo93TIkm 5g/47P3pnAG8680gpMZVeACDY3R32WKsH1yYIJONnAoTxO5WEBR5x3dlRzrxFGQRDcmw9cE8Rbx TpF1XWlQ8KQ4ccejzP07Hs1jWL6JBlV0dMTrRIpL5H7V17jMoO01D2tQeitmwDmwGYOnjSaJObL KQ X-Received: by 2002:a05:622a:1491:b0:3d9:218a:3390 with SMTP id t17-20020a05622a149100b003d9218a3390mr7090502qtx.8.1679501579276; Wed, 22 Mar 2023 09:12:59 -0700 (PDT) X-Google-Smtp-Source: AK7set/JKUndtVoVBQjzMYlGss0ryRIUe4fuyPj8SYrNDlvWgmGW4LIHGheAYsJyqGpLkKdiUDJpSA== X-Received: by 2002:a05:622a:1491:b0:3d9:218a:3390 with SMTP id t17-20020a05622a149100b003d9218a3390mr7090398qtx.8.1679501578509; Wed, 22 Mar 2023 09:12:58 -0700 (PDT) Received: from localhost ([88.120.130.27]) by smtp.gmail.com with ESMTPSA id y3-20020a37f603000000b0074382b756c2sm11528377qkj.14.2023.03.22.09.12.57 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Mar 2023 09:12:58 -0700 (PDT) Received: by localhost (Postfix, from userid 1000) id 12C44581C79; Wed, 22 Mar 2023 17:12:55 +0100 (CET) To: libabigail@sourceware.org Subject: [PATCH, applied] Bug 29912 - Better support an ELF symbol alias that designates several functions Organization: Red Hat / France X-Operating-System: Fedora 38 X-URL: http://www.redhat.com Date: Wed, 22 Mar 2023 17:12:55 +0100 Message-ID: <878rfoojco.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=-11.8 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_H2, SPF_HELO_NONE, SPF_NONE, TXREP 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: , X-Patchwork-Original-From: Dodji Seketeli via Libabigail From: Dodji Seketeli Reply-To: Dodji Seketeli Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" Hello, When an ELF symbol alias designates two different functions, Libabigail can be confused as to which function to consider. This confusion indirectly leads to showing spurious changes in the return type of some functions when an ELF symbol designates more than one function. In other words, when an ELF symbol designates two (or more) functions, the comparison engine needs a way to tell the two functions apart. It needs an other way to identify the functions. This patch fixes the confusion by using the pretty representation of the functions in those cases. Please note that to replicate the issue reported in this bug, here is the command I used: $ fedabipkgdiff --debug --self-compare -a --from fc37 smesh * include/abg-corpus.h (corpus::lookup_functions): Return a set of functions rather than a vector of functions where a function can be present more than once. This allows to determine if a symbol designates more than one function. (corpus::exported_decls_builder::priv_): Make this public so that some outside code can access it. (corpus::exported_decls_builder::fn_id_maps_to_several_fns): Declare new function. (corpus::exported_decls_builder::maybe_add_fn_to_exported_fns): Remove useless const here. * include/abg-fwd.h (get_function_id_or_pretty_representation): Declare new function. * include/abg-ir.h (elf_symbol::get_alias_with_default_symbol_version): Declare new member function. * src/abg-comparison.cc (corpus_diff::priv::ensure_lookup_tables_populated): Use the new get_function_id_or_pretty_representation rather than function_decl::get_id() to identify a function. * src/abg-corpus-priv.h (str_fn_ptr_set_map_type): Define this new typedef of unordered_map >. (corpus::exported_decls_builder::priv::id_fns_map_): Change the type of this to the new str_fn_ptr_set_map_type. (corpus::exported_decls_builder::priv::{id_fns_map, fn_id_is_in_id_fns_map, fn_is_in_fns, fn_is_in_id_fns_map}): Adjust to using a set of functions rather than a vector. (corpus::exported_decls_builder::fn_is_in_fns_by_repr): Define new static function. (corpus::exported_decls_builder::add_fn_to_exported): Remove useless const. * src/abg-corpus.cc (corpus::exported_decls_builder::fn_id_maps_to_several_fns): Define new function. (corpus::exported_decls_builder::maybe_add_fn_to_exported_fns): Remove useless const. (corpus::lookup_functions): Return a set of functions rather than a vector of functions where a function can be present more than once. This allows to determine if a symbol designates more than one function. * src/abg-dwarf-reader.cc (reader::symbol_already_belongs_to_a_function): Adjust. * src/abg-fe-iface.cc (fe_iface::maybe_add_fn_to_exported_decls): Adjust. * src/abg-ir.cc (get_function_id_or_pretty_representation): Define new function. Signed-off-by: Dodji Seketeli --- include/abg-corpus.h | 13 +++--- include/abg-fwd.h | 3 ++ include/abg-ir.h | 3 ++ src/abg-comparison.cc | 4 +- src/abg-corpus-priv.h | 96 ++++++++++++++++++++++++++++++----------- src/abg-corpus.cc | 29 +++++++++++-- src/abg-dwarf-reader.cc | 15 +++---- src/abg-fe-iface.cc | 2 +- src/abg-ir.cc | 24 +++++++++++ 9 files changed, 142 insertions(+), 47 deletions(-) diff --git a/include/abg-corpus.h b/include/abg-corpus.h index 090bad14..cbc7ec24 100644 --- a/include/abg-corpus.h +++ b/include/abg-corpus.h @@ -218,7 +218,7 @@ public: virtual const functions& get_functions() const; - const vector* + const std::unordered_set* lookup_functions(const string& id) const; void @@ -302,13 +302,13 @@ operator&=(corpus::origin &l, corpus::origin r); /// parameters needed. class corpus::exported_decls_builder { - class priv; - std::unique_ptr priv_; - // Forbid default construction. exported_decls_builder(); public: + class priv; + std::unique_ptr priv_; + friend class corpus; exported_decls_builder(functions& fns, @@ -327,6 +327,9 @@ public: functions& exported_functions(); + std::unordered_set* + fn_id_maps_to_several_fns(function_decl*); + const variables& exported_variables() const; @@ -334,7 +337,7 @@ public: exported_variables(); void - maybe_add_fn_to_exported_fns(const function_decl*); + maybe_add_fn_to_exported_fns(function_decl*); void maybe_add_var_to_exported_vars(const var_decl*); diff --git a/include/abg-fwd.h b/include/abg-fwd.h index fc3f4374..747a625c 100644 --- a/include/abg-fwd.h +++ b/include/abg-fwd.h @@ -1004,6 +1004,9 @@ get_function_type_name(const function_type*, bool internal = false); interned_string get_function_type_name(const function_type&, bool internal = false); +interned_string +get_function_id_or_pretty_representation(function_decl *fn); + interned_string get_method_type_name(const method_type_sptr&, bool internal = false); diff --git a/include/abg-ir.h b/include/abg-ir.h index 0ca8afcf..6d81097a 100644 --- a/include/abg-ir.h +++ b/include/abg-ir.h @@ -1094,6 +1094,9 @@ public: elf_symbol_sptr get_alias_which_equals(const elf_symbol& other) const; + elf_symbol_sptr + get_alias_with_default_symbol_version() const; + string get_aliases_id_string(const string_elf_symbols_map_type& symtab, bool include_symbol_itself = true) const; diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 1533d9d1..baae56a3 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -9036,7 +9036,7 @@ corpus_diff::priv::ensure_lookup_tables_populated() ABG_ASSERT(i < first_->get_functions().size()); function_decl* deleted_fn = first_->get_functions()[i]; - string n = deleted_fn->get_id(); + string n = get_function_id_or_pretty_representation(deleted_fn); ABG_ASSERT(!n.empty()); // The below is commented out because there can be several // functions with the same ID in the corpus. So several @@ -9056,7 +9056,7 @@ corpus_diff::priv::ensure_lookup_tables_populated() { unsigned i = *iit; function_decl* added_fn = second_->get_functions()[i]; - string n = added_fn->get_id(); + string n = get_function_id_or_pretty_representation(added_fn); ABG_ASSERT(!n.empty()); // The below is commented out because there can be several // functions with the same ID in the corpus. So several diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h index 422b7870..412db377 100644 --- a/src/abg-corpus-priv.h +++ b/src/abg-corpus-priv.h @@ -40,6 +40,12 @@ typedef vector regex_t_sptrs_type; /// Convenience typedef for a hash map which key is a string and which /// data is a vector of abigail::ir::function_decl* typedef unordered_map > str_fn_ptrs_map_type; + +/// Convenience typedef for a hash map which key is a string and which +/// data is a set of abigail::ir::function_decl* +typedef unordered_map > +str_fn_ptr_set_map_type; + /// Convenience typedef for a hash map which key is a string and /// which data is an abigail::ir::var_decl*. typedef unordered_map str_var_ptr_map_type; @@ -63,7 +69,7 @@ class corpus::exported_decls_builder::priv // template parameters of the second instantiation are just typedefs // of the first instantiation, for instance. So there can be cases // where one ID appertains to more than one function. - str_fn_ptrs_map_type id_fns_map_; + str_fn_ptr_set_map_type id_fns_map_; str_var_ptr_map_type id_var_map_; strings_type& fns_suppress_regexps_; regex_t_sptrs_type compiled_fns_suppress_regexp_; @@ -197,7 +203,7 @@ public: /// /// @return a map which key is a string and which data is a pointer /// to a function. - const str_fn_ptrs_map_type& + const str_fn_ptr_set_map_type& id_fns_map() const {return id_fns_map_;} @@ -210,7 +216,7 @@ public: /// /// @return a map which key is a string and which data is a pointer /// to a function. - str_fn_ptrs_map_type& + str_fn_ptr_set_map_type& id_fns_map() {return id_fns_map_;} @@ -267,11 +273,11 @@ public: /// /// @return the pointer to the vector of functions with ID @p fn_id, /// or nil if no function with that ID exists. - vector* + std::unordered_set* fn_id_is_in_id_fns_map(const string& fn_id) { - str_fn_ptrs_map_type& m = id_fns_map(); - str_fn_ptrs_map_type::iterator i = m.find(fn_id); + str_fn_ptr_set_map_type& m = id_fns_map(); + str_fn_ptr_set_map_type::iterator i = m.find(fn_id); if (i == m.end()) return 0; return &i->second; @@ -286,47 +292,87 @@ public: /// @p fn, that are present in the id-functions map, or nil if no /// function with the same ID as @p fn is present in the /// id-functions map. - vector* + std::unordered_set* fn_id_is_in_id_fns_map(const function_decl* fn) { string fn_id = fn->get_id(); return fn_id_is_in_id_fns_map(fn_id); } - /// Test if a given function is present in a vector of functions. + /// Test if a given function is present in a set of functions. /// /// The function compares the ID and the qualified name of /// functions. /// /// @param fn the function to consider. /// - /// @parm fns the vector of functions to consider. + /// @parm fns the set of functions to consider. static bool - fn_is_in_fns(const function_decl* fn, const vector& fns) + fn_is_in_fns(function_decl* fn, + const std::unordered_set& fns) { if (fns.empty()) return false; + if (fns.find(fn) != fns.end()) + return true; + const string fn_id = fn->get_id(); - for (vector::const_iterator i = fns.begin(); - i != fns.end(); - ++i) - if ((*i)->get_id() == fn_id - && (*i)->get_qualified_name() == fn->get_qualified_name()) + for (const auto f : fns) + if (f->get_id() == fn_id + && f->get_qualified_name() == fn->get_qualified_name()) return true; return false; } + /// Test if a given function is present in a set of functions, + /// by looking at the pretty representation of the function, in + /// addition to looking at its ID. + /// + /// This is useful because sometimes a given ELF symbol (alias) + /// might be for several different functions. In that case, using + /// the function pretty representation might be a way to + /// differentiate the functions having the same ELF symbol alias. + /// + /// The function compares the ID and the qualified name of + /// functions. + /// + /// @param fn the function to consider. + /// + /// @parm fns the set of functions to consider. + /// + /// @return true if @p fn is present in @p fns. + static bool + fn_is_in_fns_by_repr(function_decl* fn, + const std::unordered_set& fns, + string& pretty_representation) + { + if (!fn_is_in_fns(fn, fns)) + return false; + + const string repr = fn->get_pretty_representation(); + const string fn_id = fn->get_id(); + for (const auto f : fns) + if (f->get_id() == fn_id + && f->get_pretty_representation() == repr) + { + pretty_representation = repr; + return true; + } + + return false; + } + /// Test if a function is in the id-functions map. /// /// @param fn the function to consider. /// /// @return true iff the function is in the id-functions map. bool - fn_is_in_id_fns_map(const function_decl* fn) + fn_is_in_id_fns_map(function_decl* fn) { - vector* fns = fn_id_is_in_id_fns_map(fn); + std::unordered_set* fns = fn_id_is_in_id_fns_map(fn); if (fns && fn_is_in_fns(fn, *fns)) return true; return false; @@ -344,10 +390,10 @@ public: // First associate the function id to the function. string fn_id = fn->get_id(); - vector* fns = fn_id_is_in_id_fns_map(fn_id); + std::unordered_set* fns = fn_id_is_in_id_fns_map(fn_id); if (!fns) - fns = &(id_fns_map()[fn_id] = vector()); - fns->push_back(fn); + fns = &(id_fns_map()[fn_id] = std::unordered_set()); + fns->insert(fn); // Now associate all aliases of the underlying symbol to the // function too. @@ -361,8 +407,8 @@ public: goto loop; fns = fn_id_is_in_id_fns_map(fn_id); if (!fns) - fns = &(id_fns_map()[fn_id] = vector()); - fns->push_back(fn); + fns = &(id_fns_map()[fn_id] = std::unordered_set()); + fns->insert(fn); loop: sym = sym->get_next_alias(); } @@ -403,12 +449,12 @@ public: /// /// @param fn the function to add to the set of exported functions. void - add_fn_to_exported(const function_decl* fn) + add_fn_to_exported(function_decl* fn) { if (!fn_is_in_id_fns_map(fn)) { - fns_.push_back(const_cast(fn)); - add_fn_to_id_fns_map(const_cast(fn)); + fns_.push_back(fn); + add_fn_to_id_fns_map(fn); } } diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc index cf6685d6..fa308151 100644 --- a/src/abg-corpus.cc +++ b/src/abg-corpus.cc @@ -109,6 +109,28 @@ corpus::functions& corpus::exported_decls_builder::exported_functions() {return priv_->fns_;} +/// Test if a given function ID maps to several functions in the same corpus. +/// +/// The magic of ELF symbol aliases makes it possible for an ELF +/// symbol alias to designate several different functions. This +/// function tests if the ELF symbol of a given function has a aliases +/// that designates another function or not. +/// +/// @param fn the function to consider. +/// +/// @return the set of functions designated by the ELF symbol of @p +/// fn, or nullptr if the function ID maps to just @p fn. +std::unordered_set* +corpus::exported_decls_builder::fn_id_maps_to_several_fns(function_decl* fn) +{ + std::unordered_set *fns_for_id = + priv_->fn_id_is_in_id_fns_map(fn); + if (fns_for_id && fns_for_id->size() > 1) + return fns_for_id; + + return nullptr; +} + /// Getter for the reference to the vector of exported variables. /// This vector is shared with with the @ref corpus. It's where the /// set of exported variable is ultimately stored. @@ -133,7 +155,7 @@ corpus::exported_decls_builder::exported_variables() /// /// @param fn the function to add the set of exported functions. void -corpus::exported_decls_builder::maybe_add_fn_to_exported_fns(const function_decl* fn) +corpus::exported_decls_builder::maybe_add_fn_to_exported_fns(function_decl* fn) { if (!fn->get_is_in_public_symbol_table()) return; @@ -1314,12 +1336,11 @@ corpus::get_functions() const /// /// @return the vector functions which ID is @p id, or nil if no /// function with that ID was found. -const vector* +const std::unordered_set* corpus::lookup_functions(const string& id) const { exported_decls_builder_sptr b = get_exported_decls_builder(); - str_fn_ptrs_map_type::const_iterator i = - b->priv_->id_fns_map_.find(id); + auto i = b->priv_->id_fns_map_.find(id); if (i == b->priv_->id_fns_map_.end()) return 0; return &i->second; diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc index 037538af..66b99fd5 100644 --- a/src/abg-dwarf-reader.cc +++ b/src/abg-dwarf-reader.cc @@ -4460,19 +4460,14 @@ public: string id = fn->get_id_string(); - const vector *fns = corp->lookup_functions(id); + const std::unordered_set *fns = corp->lookup_functions(id); if (!fns) return false; - for (vector::const_iterator i = fns->begin(); - i != fns->end(); - ++i) - { - function_decl* f = *i; - ABG_ASSERT(f); - if (f->get_symbol()) - return true; - } + for (auto f : *fns) + if (f->get_symbol()) + return true; + return false; } diff --git a/src/abg-fe-iface.cc b/src/abg-fe-iface.cc index f3798cd6..bc272425 100644 --- a/src/abg-fe-iface.cc +++ b/src/abg-fe-iface.cc @@ -310,7 +310,7 @@ fe_iface::maybe_add_fn_to_exported_decls(const function_decl* fn) if (fn) if (corpus::exported_decls_builder* b = corpus()->get_exported_decls_builder().get()) - b->maybe_add_fn_to_exported_fns(fn); + b->maybe_add_fn_to_exported_fns(const_cast(fn)); } /// Try and add the representation of the ABI of a variable to the set diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 047b755d..514013c7 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -8900,6 +8900,30 @@ get_function_type_name(const function_type& fn_type, return env.intern(o.str()); } +/// Get the ID of a function, or, if the ID can designate several +/// different functions, get its pretty representation. +/// +/// @param fn the function to consider +/// +/// @return the function ID of pretty representation of @p fn. +interned_string +get_function_id_or_pretty_representation(function_decl *fn) +{ + ABG_ASSERT(fn); + + interned_string result = fn->get_environment().intern(fn->get_id()); + + if (corpus *c = fn->get_corpus()) + { + corpus::exported_decls_builder_sptr b = + c->get_exported_decls_builder(); + if (b->fn_id_maps_to_several_fns(fn)) + result = fn->get_environment().intern(fn->get_pretty_representation()); + } + + return result; +} + /// Get the name of a given method type and return a copy of it. /// /// @param fn_type the function type to consider.