From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 4FFDA3858D1E for ; Wed, 30 Nov 2022 16:16:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4FFDA3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669825018; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type; bh=xJWYylIDk1S0bfb9+2BYK3P2zxUC2/utzWBPl0XmSr0=; b=VV0dlgnqdZ1yIZMHQBvI12XN+4b/H8GAhxlHCG8CBcWRE76GBJ/fBb6nBJ1CEZsUWuiX1h KxN6Qlb8duVjYg9DzGJUh01Ui5B7fksDKiMyfLkU8+NaYbx2z3ef15pqcSGtge8n8F7FS0 fGgnWqIA6LFqxMYZvKclqfGLg0fw118= Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-491-oNe8BM-cNbyG7rXrOqlgHA-1; Wed, 30 Nov 2022 11:16:57 -0500 X-MC-Unique: oNe8BM-cNbyG7rXrOqlgHA-1 Received: by mail-qk1-f200.google.com with SMTP id h8-20020a05620a284800b006b5c98f09fbso41020081qkp.21 for ; Wed, 30 Nov 2022 08:16:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=xJWYylIDk1S0bfb9+2BYK3P2zxUC2/utzWBPl0XmSr0=; b=Rdf/ReN67SDK/XM3IUEuO3a7Jsjju40UqHH/9Po0l6C9aAc2yQtiOi+zDY7Qry9dlQ zqLmVoA2m78szMMVE1qXRU38tUDTdbXMpMbczSUOkQ6spevtFLRopSvufeiq7Gbdl5EW Utqp77BoFBZxAa7oLVKtgc+h1mgVBm9c1AEp/y7Ya+nKKUO9jRBfTL7jDMN5a20C1BP3 JMQ2mZBuNhVmPbVnzbZl265cLqYdl8lMvlVLgwltD27XyJS24ws9Mex/ACzXjs1pkD0h l7ZlxO1RnQsqqGHUCTOWbEs3QrPX6cJD0r/u6FKd6wZmjtdOu5mzvzVPFws6ZP7bX6A5 1INg== X-Gm-Message-State: ANoB5pl2RCRc72K1XtoABx7iD4mGwwS2AH//vD1B1suCInWmnRFBjYxT t0InW/bQLt0cRsnXbO8ZkkJwm+57EqYJP7ZeMpiBq0Iu8QUz777iwaqRPyu/3soItVR2r2V8AC1 HJwzJNaRvYQEkdwuTrvYe7L1LwPqJZRfI8+uxUMsrd5D7aPHdtY8c1bQgh+CkT7EjVWsG X-Received: by 2002:ac8:7344:0:b0:3a6:8920:5b2b with SMTP id q4-20020ac87344000000b003a689205b2bmr5924517qtp.438.1669825016323; Wed, 30 Nov 2022 08:16:56 -0800 (PST) X-Google-Smtp-Source: AA0mqf6yQ1eh3xl7i/yAoW1vImXznqjU2XWN3ymnfNomkiC+xFZE5f3oz8riRh5A9fxpNfl3nfZ77w== X-Received: by 2002:ac8:7344:0:b0:3a6:8920:5b2b with SMTP id q4-20020ac87344000000b003a689205b2bmr5924470qtp.438.1669825015829; Wed, 30 Nov 2022 08:16:55 -0800 (PST) Received: from localhost ([88.120.130.27]) by smtp.gmail.com with ESMTPSA id bp39-20020a05620a45a700b006cbe3be300esm1498563qkb.12.2022.11.30.08.16.54 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Nov 2022 08:16:55 -0800 (PST) Received: by localhost (Postfix, from userid 1000) id 463CCC1B73CB; Wed, 30 Nov 2022 17:16:53 +0100 (CET) From: Dodji Seketeli To: libabigail@sourceware.org Subject: [PATCH, applied] Fix spurious deleted/added virtual destructor change report Organization: Red Hat / France X-Operating-System: AlmaLinux 9.0 X-URL: http://www.redhat.com Date: Wed, 30 Nov 2022 17:16:53 +0100 Message-ID: <87lensto22.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.6 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 List-Id: Hello, While looking at something else, I noticed this spurious change report: 1 member function deletion: 'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:361:1 1 member function insertion: 'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:370:1 This is when running tests/runtestdiffpkg and the result is in tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt. To understand what is going on, one need to refer to the definitions of the C++ ABI at https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions. When a class has a virtual destructor, historically, there might be 3 different and co-existing actual implementations of the destructors: the D0, D1 and D2 destructors. There might even be a D3 destructor, actually. In https://github.com/itanium-cxx-abi/cxx-abi/issues/73, one can see that there might be new D4 and D5 destructors. Each one of them might replace the set of the D0,D1,D2 destructors. So, in a new version of a binary, the virtual D4 destructor might replace the previous ones, without it being an ABI issue. The switch to the D4 virtual destructor is what libabigail is naively reporting in the change report above. This patch detects this kind of changes in the pipeline when the edit script from the diff2 algorithm is interpreted for virtual member functions and drops it on the floor. * src/abg-comparison.cc (find_virtual_dtor_in_map): Define new static function. (class_diff::ensure_lookup_tables_populated): If a virtual destructor is removed from the old binary version but is added to the new one (but through a different name), let's assume the virtual destructor is still there so there is no ABI issue from that point of view. * tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt: Adjust. Signed-off-by: Dodji Seketeli Applied to the master branch. --- src/abg-comparison.cc | 92 ++++++++++++++++++- ...bb-4.3-3.20141204.fc23.x86_64-report-0.txt | 8 -- 2 files changed, 91 insertions(+), 9 deletions(-) diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 0868d384..0ecd0f27 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -5146,6 +5146,27 @@ class_diff::lookup_tables_empty(void) const && priv_->changed_bases_.empty()); } +/// Find a virtual destructor in a map of member functions +/// +/// @param map the map of member functions. Note that the key of the +/// map is the member function name. The key is the member function. +/// +/// @return an iterator to the destructor found or, if no virtual destructor +/// was found, return map.end() +static string_member_function_sptr_map::const_iterator +find_virtual_dtor_in_map(const string_member_function_sptr_map& map) +{ + for (string_member_function_sptr_map::const_iterator i = map.begin(); + i !=map.end(); + ++i) + { + if (get_member_function_is_dtor(i->second) + && get_member_function_is_virtual(i->second)) + return i; + } + return map.end(); +} + /// If the lookup tables are not yet built, walk the differences and /// fill them. void @@ -5277,6 +5298,45 @@ class_diff::ensure_lookup_tables_populated(void) const // underlying symbols are deleted as well; otherwise, consider // that the member function in question hasn't been deleted. + // Also, while walking the deleted member functions, we attend at + // a particular cleanup business related to (virtual) C++ + // destructors: + // + // In the binary, there can be at least three types of + // destructors, defined in the document + // https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions: + // + // 1/ Base object destructor (aka D2 destructor): + // + // "A function that runs the destructors for non-static data + // members of T and non-virtual direct base classes of T. " + // + // 2/ Complete object destructor (aka D1 destructor): + // + // "A function that, in addition to the actions required of a + // base object destructor, runs the destructors for the + // virtual base classes of T." + // + // 3/ Deleting destructor (aka D0 destructor): + // + // "A function that, in addition to the actions required of a + // complete object destructor, calls the appropriate + // deallocation function (i.e,. operator delete) for T." + // + // With binaries generated by GCC, these destructors might be ELF + // clones of each others, meaning, their ELF symbols can be + // aliases. + // + // Also, note that because the actual destructor invoked by user + // code is virtual, it's invoked through the vtable. So the + // presence of the underlying D0, D1, D2 in the binary might vary + // without that variation being an ABI issue, provided that the + // destructor invoked through the vtable is present. + // + // So, a particular virtual destructor implementation for a class + // might disapear and be replaced by another one in a subsequent + // version of the binary. If all versions of the binary have an + // actual virtual destructor, things might be considered fine. vector to_delete; corpus_sptr f = context()->get_first_corpus(), s = context()->get_second_corpus(); @@ -5287,7 +5347,37 @@ class_diff::ensure_lookup_tables_populated(void) const ++i) { if (get_member_function_is_virtual(i->second)) - continue; + { + if (get_member_function_is_dtor(i->second)) + { + // If a particular virtual destructor is deleted, + // but the new binary still have a virtual + // destructor for that class we consider that things + // are fine. For instance, in the + // tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt + // test, a new D4 destructor replaces the old ones. + // But because the virtual destructor is still + // there, this is not an ABI issue. So let's detect + // this case. + auto it = + find_virtual_dtor_in_map(p->inserted_member_functions_); + if (it != p->inserted_member_functions_.end()) + { + // So the deleted virtual destructor is not + // really deleted, because a proper virtual + // destructor was added to the new version. + // Let's remove the deleted/added virtual + // destructor then. + string name = + (!i->second->get_linkage_name().empty()) + ? i->second->get_linkage_name() + : i->second->get_pretty_representation(); + to_delete.push_back(name); + p->inserted_member_functions_.erase(it); + } + } + continue; + } // We assume that all non-virtual member functions functions // we look at here have ELF symbols. if (!i->second->get_symbol() diff --git a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt index 89e9c5ce..07d2f187 100644 --- a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt +++ b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt @@ -30,10 +30,6 @@ implicit parameter 0 of type 'tbb::filter*' has sub-type changes: in pointed to type 'class tbb::filter' at pipeline.h:65:1: type size hasn't changed - 1 member function deletion: - 'method virtual tbb::filter::~filter(int)' at pipeline.cpp:698:1 - 1 member function insertion: - 'method virtual tbb::filter::~filter(int)' at pipeline.cpp:688:1 no member function changes (4 filtered); 1 data member changes (4 filtered): type of 'tbb::internal::input_buffer* my_input_buffer' changed: @@ -171,10 +167,6 @@ implicit parameter 0 of type 'tbb::internal::concurrent_queue_base_v3*' has sub-type changes: in pointed to type 'class tbb::internal::concurrent_queue_base_v3' at _concurrent_queue_impl.h:834:1: type size hasn't changed - 1 member function deletion: - 'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:361:1 - 1 member function insertion: - 'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:370:1 no member function changes (7 filtered); 1 data member change: type of 'tbb::internal::concurrent_queue_rep* my_rep' changed: -- 2.31.1 -- Dodji