From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) by sourceware.org (Postfix) with ESMTPS id 87CE3395200C for ; Thu, 17 Mar 2022 12:01:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 87CE3395200C Received: by mail-wr1-x433.google.com with SMTP id h15so7041283wrc.6 for ; Thu, 17 Mar 2022 05:01:48 -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:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JL0Hb5T71tuVe5ieqxtF2ms/JP9SOIMJ7qkjyXpyN2E=; b=vah/35cS/CYob4/O8Y0nBeSqCBWh/cwNPpqlPl6JIMOheKIQhq5VYNXE4S4zNj5iGO DU2uRRDOBrKJeirLP4PrixkW4PHo3jlhESe/bVtZ6SPa9roJUYMvRYuu4rKL6/Cql/x4 3yY5dE8X96Y2gaK301pVEK03BifeB84ONMi7mWLoRWigUlNZJNYEF7ArQRWO/XTHe88e LXwhFXOvN/CanmjUjS9LzhP/QXM8SbeO1k/XaWsUi3ZdDrkLq1k67Lsv/IPcfPwzIxfm jIkt7ttKDlzZLoCiwng0vv9TgIZH6nN/ar48Ms49J3K1KBZSo4doF8sWhAZwEQbK2VCB Nc1g== X-Gm-Message-State: AOAM533wsSPwKf1Eeu+Vuol7+IQUmwUn7CjpxPLGGiftgu5GbLfsloax TA/3oQXGF1fv3sWVMi2CD2vA+Q== X-Google-Smtp-Source: ABdhPJxuCZSXJvTcKItMVOpBuB//mEPrRMI9cdNVYgFIbxddORV16W2nKqfGjwcKDTCbOuwD/UMnlQ== X-Received: by 2002:adf:f606:0:b0:203:8dff:f4ac with SMTP id t6-20020adff606000000b002038dfff4acmr3719612wrp.12.1647518506957; Thu, 17 Mar 2022 05:01:46 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:8b6c:955c:c308:5566]) by smtp.gmail.com with ESMTPSA id i35-20020adf90a6000000b00203e767a1d2sm1959453wri.103.2022.03.17.05.01.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Mar 2022 05:01:46 -0700 (PDT) Date: Thu, 17 Mar 2022 12:01:45 +0000 From: Matthias Maennich To: Giuliano Procida Cc: libabigail@sourceware.org, dodji@seketeli.org, kernel-team@android.com Subject: Re: [PATCH v2 4/4] Linux symbol CRCs: support 0 and report presence changes Message-ID: References: <20220314181312.3436802-1-gprocida@google.com> <20220316163055.4127796-1-gprocida@google.com> <20220316163055.4127796-5-gprocida@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20220316163055.4127796-5-gprocida@google.com> X-Spam-Status: No, score=-29.7 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_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-List-Received-Date: Thu, 17 Mar 2022 12:01:51 -0000 On Wed, Mar 16, 2022 at 04:30:55PM +0000, Giuliano Procida wrote: >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 Cheers, Matthias >--- > 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()); > } > } >-- >2.35.1.894.gb6a874cedc-goog >