From patchwork Wed Mar 16 16:30:55 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 52013 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 894883888C41 for ; Wed, 16 Mar 2022 16:31:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 894883888C41 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1647448299; bh=S0Qf+6do/qEGhIRUxMR7fkpudQCRDldRNie4Qrhex4c=; h=Date:In-Reply-To:References:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Help:List-Subscribe:From:Reply-To:Cc:From; b=SzOV13nDwcnjnUODiqoyTArH/PlfQ4gaV69qHiKJDl0Gu7X5yWJH4lMNW2MQLCivq c8eo5+psy6Am3xgbHFw3ojosQBzHER5xpnu0RPWlN/ZQnr73McO3Tbk7n4tHzWAMLQ 2S5gqIGQbn9ao56ybnJu+QQ43mq0dRXaxAsDj/pY= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mail-wm1-x34a.google.com (mail-wm1-x34a.google.com [IPv6:2a00:1450:4864:20::34a]) by sourceware.org (Postfix) with ESMTPS id 2755F3888837 for ; Wed, 16 Mar 2022 16:31:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2755F3888837 Received: by mail-wm1-x34a.google.com with SMTP id f19-20020a7bcd13000000b0038c01defd5aso1236241wmj.7 for ; Wed, 16 Mar 2022 09:31:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=S0Qf+6do/qEGhIRUxMR7fkpudQCRDldRNie4Qrhex4c=; b=lLX7vFr8KpBVaaj6JDeemARTuYQ5bl/sOxU/2UIb9W5l3s9RxDVMjvfIBfN7vReb8/ Blg6N0hATbeOt8KW9q7/AHjWOZTrP8X4DdQXAuQbwBodCihbigzTFIyWdMPZ1bnoRHl1 E4ICFjEhcJ7rUP1yBclNur1Ayo7AXiDe8aGEPLtUV38vr4HqKrtM8zWO45PuT51w7gwN IMEE8ObrbxQ/tGuBrJq3kkkUnEgcorlDpm4Bik3TDDqGH343mhtZYrRipmwjX2yevuuL ufbJcxrGJBj4wcJf8TGRKXhMVRcBS+rYcDHv9Tdkl1ZwBHkl1JHzIOzmUxCvwaqnm7Ew dYRA== X-Gm-Message-State: AOAM5314wgtTEsuGxtGHrb+YOmyapK0KfjSACY2WSFVoW+U5O+SCnBX1 radgKdqM5jp5c7/Xn4JZbitz9Tk2G7YXhXUj7knalpf7JsaixBRzipUNkfF9zql1IFxo2mmuCXl 1e3TrebX7VoZVqjYopWrLKEYWMAv+o8/EgGrQCNLMTRt1kzfPkPKvO83XW31uHDvMZuDQHKc= X-Google-Smtp-Source: ABdhPJy5zJm48mzNROp4FeOspTHA9SW1MhBj4gtdV3g3XY5SMGFJtu1WH/a4e8Nmma3FyQSjjFBtxFsmCmbe7A== X-Received: from tef.lon.corp.google.com ([2a00:79e0:d:210:5871:c940:7381:f8f4]) (user=gprocida job=sendgmr) by 2002:a5d:68d2:0:b0:1f0:653f:108d with SMTP id p18-20020a5d68d2000000b001f0653f108dmr556943wrw.283.1647448291945; Wed, 16 Mar 2022 09:31:31 -0700 (PDT) Date: Wed, 16 Mar 2022 16:30:55 +0000 In-Reply-To: <20220316163055.4127796-1-gprocida@google.com> Message-Id: <20220316163055.4127796-5-gprocida@google.com> Mime-Version: 1.0 References: <20220314181312.3436802-1-gprocida@google.com> <20220316163055.4127796-1-gprocida@google.com> X-Mailer: git-send-email 2.35.1.894.gb6a874cedc-goog Subject: [PATCH v2 4/4] Linux symbol CRCs: support 0 and report presence changes To: libabigail@sourceware.org X-Spam-Status: No, score=-22.6 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, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" The CRC with value zero was used to mean "absent". This can be better modelled using optional. This commit makes this change and also tweaks reporting so that disappearing / appearing CRCs are noted. This should be essentially impossible unless CRCs are enabled / disabled altogether but would be very noteworthy otherwise. * include/abg-ir.h (elf_symbol::elf_symbol): Argument crc is now an optional defaulted to absent. (elf_symbol::create): Likewise. (elf_symbol::get_crc): Now returns an optional uint64_t. (elf_symbol::set_src): Now takes an optional uint64_t. * src/abg-comp-filter.cc (crc_changed): Simplify comparison. * src/abg-ir.cc (elf_symbol::priv): Member crc_ is now an optional uint64_t. (elf_symbol::priv::priv): Argument crc is now an optional uint64_t. (elf_symbol::elf_symbol): Likewise. (elf_symbol::create): Argument crc is now an optional uint64_t and defaults to absent. (textually_equals): Simplify comparison. (elf_symbol::get_crc): Now returns an optional uint64_t. (elf_symbol::set_crc): Now takes an optional uint64_t. * src/abg-reader.cc (build_elf_symbol): Treat CRC 0 the same as other CRC values. * src/abg-reporter-priv.cc (maybe_report_diff_for_symbol): Treat CRC 0 the same as other CRC values and also report changes to CRC presence. * src/abg-writer.cc (write_elf_symbol): Treat CRC 0 the same as other CRC values. * tests/data/Makefile: Remove test-abidiff/test-crc-report.txt and add test-abidiff/test-crc-report-{0-1,1-0,1-2}.txt. * tests/data/test-abidiff/test-crc-report-0-1.txt: Report showing additional of CRCs. * tests/data/test-abidiff/test-crc-report-1-0.txt: Report showing removal of CRCs. * tests/data/test-abidiff/test-crc-report-1-2.txt: Renamed from tests/data/test-abidiff/test-crc-report.txt. * tests/test-abidiff.cc: Update test cases that no longer generate empty reports. * tests/test-symtab.cc: Update KernelSymtabsWithCRC test. Signed-off-by: Giuliano Procida Reviewed-by: Matthias Maennich --- include/abg-ir.h | 8 ++++---- src/abg-comp-filter.cc | 4 +--- src/abg-ir.cc | 19 +++++++++--------- src/abg-reader.cc | 8 ++------ src/abg-reporter-priv.cc | 20 ++++++++++++++----- src/abg-writer.cc | 6 +++--- tests/data/Makefile.am | 4 +++- .../data/test-abidiff/test-crc-report-0-1.txt | 16 +++++++++++++++ .../data/test-abidiff/test-crc-report-1-0.txt | 16 +++++++++++++++ ...crc-report.txt => test-crc-report-1-2.txt} | 0 tests/test-abidiff.cc | 6 +++--- tests/test-symtab.cc | 8 ++++---- 12 files changed, 76 insertions(+), 39 deletions(-) create mode 100644 tests/data/test-abidiff/test-crc-report-0-1.txt create mode 100644 tests/data/test-abidiff/test-crc-report-1-0.txt rename tests/data/test-abidiff/{test-crc-report.txt => test-crc-report-1-2.txt} (100%) diff --git a/include/abg-ir.h b/include/abg-ir.h index 7c42bea7..7c558828 100644 --- a/include/abg-ir.h +++ b/include/abg-ir.h @@ -921,7 +921,7 @@ private: const version& ve, visibility vi, bool is_in_ksymtab = false, - uint64_t crc = 0, + const abg_compat::optional& crc = {}, const abg_compat::optional& ns = {}, bool is_suppressed = false); @@ -947,7 +947,7 @@ public: const version& ve, visibility vi, bool is_in_ksymtab = false, - uint64_t crc = 0, + const abg_compat::optional& crc = {}, const abg_compat::optional& ns = {}, bool is_suppressed = false); @@ -1020,11 +1020,11 @@ public: void set_is_in_ksymtab(bool is_in_ksymtab); - uint64_t + const abg_compat::optional& get_crc() const; void - set_crc(uint64_t crc); + set_crc(const abg_compat::optional& crc); const abg_compat::optional& get_namespace() const; diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index c179b6bd..22da5244 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -234,9 +234,7 @@ crc_changed(const function_or_var_decl_sptr& f, const auto& symbol_s = s->get_symbol(); if (!symbol_f || !symbol_s) return false; - const auto crc_f = symbol_f->get_crc(); - const auto crc_s = symbol_s->get_crc(); - return crc_f != 0 && crc_s != 0 && crc_f != crc_s; + return symbol_f->get_crc() != symbol_s->get_crc(); } /// Test if the current diff tree node carries a CRC change in either a diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 78154622..f90be2e4 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -1727,7 +1727,7 @@ struct elf_symbol::priv // STT_COMMON definition of that name that has the largest size. bool is_common_; bool is_in_ksymtab_; - uint64_t crc_; + abg_compat::optional crc_; abg_compat::optional namespace_; bool is_suppressed_; elf_symbol_wptr main_symbol_; @@ -1745,7 +1745,7 @@ struct elf_symbol::priv is_defined_(false), is_common_(false), is_in_ksymtab_(false), - crc_(0), + crc_(), namespace_(), is_suppressed_(false) {} @@ -1761,7 +1761,7 @@ struct elf_symbol::priv const elf_symbol::version& ve, elf_symbol::visibility vi, bool is_in_ksymtab, - uint64_t crc, + const abg_compat::optional& crc, const abg_compat::optional& ns, bool is_suppressed) : env_(e), @@ -1835,7 +1835,7 @@ elf_symbol::elf_symbol(const environment* e, const version& ve, visibility vi, bool is_in_ksymtab, - uint64_t crc, + const abg_compat::optional& crc, const abg_compat::optional& ns, bool is_suppressed) : priv_(new priv(e, @@ -1910,7 +1910,7 @@ elf_symbol::create(const environment* e, const version& ve, visibility vi, bool is_in_ksymtab, - uint64_t crc, + const abg_compat::optional& crc, const abg_compat::optional& ns, bool is_suppressed) { @@ -1937,8 +1937,7 @@ textually_equals(const elf_symbol&l, && l.is_defined() == r.is_defined() && l.is_common_symbol() == r.is_common_symbol() && l.get_version() == r.get_version() - && (l.get_crc() == 0 || r.get_crc() == 0 - || l.get_crc() == r.get_crc()) + && l.get_crc() == r.get_crc() && l.get_namespace() == r.get_namespace()); if (equals && l.is_variable()) @@ -2147,8 +2146,8 @@ elf_symbol::set_is_in_ksymtab(bool is_in_ksymtab) /// Getter of the 'crc' property. /// -/// @return the CRC (modversions) value for Linux Kernel symbols (if present) -uint64_t +/// @return the CRC (modversions) value for Linux Kernel symbols, if any +const abg_compat::optional& elf_symbol::get_crc() const {return priv_->crc_;} @@ -2156,7 +2155,7 @@ elf_symbol::get_crc() const /// /// @param crc the new CRC (modversions) value for Linux Kernel symbols void -elf_symbol::set_crc(uint64_t crc) +elf_symbol::set_crc(const abg_compat::optional& crc) {priv_->crc_ = crc;} /// Getter of the 'namespace' property. diff --git a/src/abg-reader.cc b/src/abg-reader.cc index 0a8ecd70..f9e420f1 100644 --- a/src/abg-reader.cc +++ b/src/abg-reader.cc @@ -3239,10 +3239,6 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node, is_default_version = true; } - uint64_t crc = 0; - if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "crc")) - crc = strtoull(CHAR_STR(s), NULL, 0); - elf_symbol::type type = elf_symbol::NOTYPE_TYPE; read_elf_symbol_type(node, type); @@ -3266,8 +3262,8 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node, e->set_is_suppressed(is_suppressed); - if (crc != 0) - e->set_crc(crc); + if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "crc")) + e->set_crc(strtoull(CHAR_STR(s), NULL, 0)); if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "namespace")) { diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc index 9ebcdf00..9ad733f4 100644 --- a/src/abg-reporter-priv.cc +++ b/src/abg-reporter-priv.cc @@ -1149,13 +1149,23 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr& symbol1, << "\n"; } - if (symbol1->get_crc() != 0 && symbol2->get_crc() != 0 - && symbol1->get_crc() != symbol2->get_crc()) + const abg_compat::optional& crc1 = symbol1->get_crc(); + const abg_compat::optional& crc2 = symbol2->get_crc(); + if (crc1 != crc2) { + const std::string none = "(none)"; out << indent << "CRC (modversions) changed from " - << std::showbase << std::hex - << symbol1->get_crc() << " to " << symbol2->get_crc() - << std::noshowbase << std::dec + << std::showbase << std::hex; + if (crc1.has_value()) + out << crc1.value(); + else + out << none; + out << " to "; + if (crc2.has_value()) + out << crc2.value(); + else + out << none; + out << std::noshowbase << std::dec << "\n"; } diff --git a/src/abg-writer.cc b/src/abg-writer.cc index 692abc75..03fb658b 100644 --- a/src/abg-writer.cc +++ b/src/abg-writer.cc @@ -3131,10 +3131,10 @@ write_elf_symbol(const elf_symbol_sptr& sym, if (sym->is_common_symbol()) o << " is-common='yes'"; - if (sym->get_crc() != 0) + if (sym->get_crc().has_value()) o << " crc='" - << std::hex << std::showbase << sym->get_crc() << "'" - << std::dec << std::noshowbase; + << std::hex << std::showbase << sym->get_crc().value() + << std::dec << std::noshowbase << "'"; if (sym->get_namespace().has_value()) o << " namespace='" << sym->get_namespace().value() << "'"; diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 7ea24d58..7388697b 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -90,7 +90,9 @@ test-abidiff/test-empty-corpus-2.xml \ test-abidiff/test-crc-0.xml \ test-abidiff/test-crc-1.xml \ test-abidiff/test-crc-2.xml \ -test-abidiff/test-crc-report.txt \ +test-abidiff/test-crc-report-0-1.txt \ +test-abidiff/test-crc-report-1-0.txt \ +test-abidiff/test-crc-report-1-2.txt \ test-abidiff/test-namespace-0.xml \ test-abidiff/test-namespace-1.xml \ test-abidiff/test-namespace-report.txt \ diff --git a/tests/data/test-abidiff/test-crc-report-0-1.txt b/tests/data/test-abidiff/test-crc-report-0-1.txt new file mode 100644 index 00000000..0db42f68 --- /dev/null +++ b/tests/data/test-abidiff/test-crc-report-0-1.txt @@ -0,0 +1,16 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 2 Changed, 0 Added variables + +1 function with some indirect sub-type change: + + [C] 'function void exported_function()' has some indirect sub-type changes: + CRC (modversions) changed from (none) to 0xe52d5bcf + +2 Changed variables: + + [C] 'int exported_variable' was changed: + CRC (modversions) changed from (none) to 0xee94d699 + + [C] 'int exported_variable_gpl' was changed: + CRC (modversions) changed from (none) to 0x41336c46 + diff --git a/tests/data/test-abidiff/test-crc-report-1-0.txt b/tests/data/test-abidiff/test-crc-report-1-0.txt new file mode 100644 index 00000000..e11f29c1 --- /dev/null +++ b/tests/data/test-abidiff/test-crc-report-1-0.txt @@ -0,0 +1,16 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 2 Changed, 0 Added variables + +1 function with some indirect sub-type change: + + [C] 'function void exported_function()' has some indirect sub-type changes: + CRC (modversions) changed from 0xe52d5bcf to (none) + +2 Changed variables: + + [C] 'int exported_variable' was changed: + CRC (modversions) changed from 0xee94d699 to (none) + + [C] 'int exported_variable_gpl' was changed: + CRC (modversions) changed from 0x41336c46 to (none) + diff --git a/tests/data/test-abidiff/test-crc-report.txt b/tests/data/test-abidiff/test-crc-report-1-2.txt similarity index 100% rename from tests/data/test-abidiff/test-crc-report.txt rename to tests/data/test-abidiff/test-crc-report-1-2.txt diff --git a/tests/test-abidiff.cc b/tests/test-abidiff.cc index 009f7de5..93e44d6a 100644 --- a/tests/test-abidiff.cc +++ b/tests/test-abidiff.cc @@ -113,19 +113,19 @@ static InOutSpec specs[] = { "data/test-abidiff/test-crc-0.xml", "data/test-abidiff/test-crc-1.xml", - "data/test-abidiff/empty-report.txt", + "data/test-abidiff/test-crc-report-0-1.txt", "output/test-abidiff/test-crc-report-0-1.txt" }, { "data/test-abidiff/test-crc-1.xml", "data/test-abidiff/test-crc-0.xml", - "data/test-abidiff/empty-report.txt", + "data/test-abidiff/test-crc-report-1-0.txt", "output/test-abidiff/test-crc-report-1-0.txt" }, { "data/test-abidiff/test-crc-1.xml", "data/test-abidiff/test-crc-2.xml", - "data/test-abidiff/test-crc-report.txt", + "data/test-abidiff/test-crc-report-1-2.txt", "output/test-abidiff/test-crc-report-1-2.txt" }, { diff --git a/tests/test-symtab.cc b/tests/test-symtab.cc index 7d7a2df0..85303563 100644 --- a/tests/test-symtab.cc +++ b/tests/test-symtab.cc @@ -420,9 +420,9 @@ TEST_CASE("Symtab::KernelSymtabsWithCRC", "[symtab, crc, kernel, ksymtab]") { const std::string binary = base_path + "one_of_each.ko"; const corpus_sptr& corpus = assert_symbol_count(binary, 2, 2); - CHECK(corpus->lookup_function_symbol("exported_function")->get_crc() != 0); - CHECK(corpus->lookup_function_symbol("exported_function_gpl")->get_crc() != 0); - CHECK(corpus->lookup_variable_symbol("exported_variable")->get_crc() != 0); - CHECK(corpus->lookup_variable_symbol("exported_variable_gpl")->get_crc() != 0); + CHECK(corpus->lookup_function_symbol("exported_function")->get_crc()); + CHECK(corpus->lookup_function_symbol("exported_function_gpl")->get_crc()); + CHECK(corpus->lookup_variable_symbol("exported_variable")->get_crc()); + CHECK(corpus->lookup_variable_symbol("exported_variable_gpl")->get_crc()); } }