From: Matthias Maennich <maennich@google.com>
To: Giuliano Procida <gprocida@google.com>
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
Date: Thu, 17 Mar 2022 12:01:45 +0000 [thread overview]
Message-ID: <YjMjKTt13CEPBszR@google.com> (raw)
In-Reply-To: <20220316163055.4127796-5-gprocida@google.com>
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 <gprocida@google.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
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<uint64_t>& crc = {},
> const abg_compat::optional<std::string>& 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<uint64_t>& crc = {},
> const abg_compat::optional<std::string>& 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<uint64_t>&
> get_crc() const;
>
> void
>- set_crc(uint64_t crc);
>+ set_crc(const abg_compat::optional<uint64_t>& crc);
>
> const abg_compat::optional<std::string>&
> 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<uint64_t> crc_;
> abg_compat::optional<std::string> 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<uint64_t>& crc,
> const abg_compat::optional<std::string>& 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<uint64_t>& crc,
> const abg_compat::optional<std::string>& 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<uint64_t>& crc,
> const abg_compat::optional<std::string>& 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<uint64_t>&
> 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<uint64_t>& 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<uint64_t>& crc1 = symbol1->get_crc();
>+ const abg_compat::optional<uint64_t>& 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
>
next prev parent reply other threads:[~2022-03-17 12:01 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-14 18:13 [PATCH 0/2] Bug 28954 - add Linux Kernel symbol namespace support Giuliano Procida
2022-03-14 18:13 ` [PATCH 1/2] optional: add operator== and operator!= Giuliano Procida
2022-03-15 11:02 ` Matthias Maennich
2022-03-16 9:31 ` Giuliano Procida
2022-03-14 18:13 ` [PATCH 2/2] add Linux kernel symbol namespace support Giuliano Procida
2022-03-15 11:25 ` Matthias Maennich
2022-03-16 16:30 ` [PATCH v2 0/4] add symbol namespace support, update symbol CRC support Giuliano Procida
2022-03-16 16:30 ` [PATCH v2 1/4] optional: minor improvements Giuliano Procida
2022-03-17 10:56 ` Matthias Maennich
2022-03-16 16:30 ` [PATCH v2 2/4] crc_changed: eliminate copying of shared_ptr values Giuliano Procida
2022-03-17 11:01 ` Matthias Maennich
2022-03-16 16:30 ` [PATCH v2 3/4] add Linux kernel symbol namespace support Giuliano Procida
2022-03-17 11:57 ` Matthias Maennich
2022-03-16 16:30 ` [PATCH v2 4/4] Linux symbol CRCs: support 0 and report presence changes Giuliano Procida
2022-03-17 12:01 ` Matthias Maennich [this message]
2022-03-17 16:38 ` [PATCH v3 0/4] add symbol namespace support, update symbol CRC support Giuliano Procida
2022-03-17 16:38 ` [PATCH v3 1/4] crc_changed: eliminate copying of shared_ptr values Giuliano Procida
2022-03-17 16:38 ` [PATCH v3 2/4] optional: minor improvements Giuliano Procida
2022-03-17 16:38 ` [PATCH v3 3/4] Linux symbol CRCs: support 0 and report presence changes Giuliano Procida
2022-03-17 16:38 ` [PATCH v3 4/4] add Linux kernel symbol namespace support Giuliano Procida
2022-03-21 12:53 ` Matthias Maennich
2022-03-21 15:52 ` Giuliano Procida
2022-03-21 16:02 ` [PATCH v4 0/4] add symbol namespace support, update symbol CRC support Giuliano Procida
2022-03-21 16:02 ` [PATCH v4 1/4] crc_changed: eliminate copying of shared_ptr values Giuliano Procida
2022-03-21 16:02 ` [PATCH v4 2/4] optional: minor improvements Giuliano Procida
2022-03-21 16:02 ` [PATCH v4 3/4] Linux symbol CRCs: support 0 and report presence changes Giuliano Procida
2022-03-21 16:02 ` [PATCH v4 4/4] add Linux kernel symbol namespace support Giuliano Procida
2022-06-13 14:25 ` [PATCH v5 0/4] add symbol namespace support, update symbol CRC support Giuliano Procida
2022-06-30 16:39 ` Dodji Seketeli
2022-06-13 14:25 ` [PATCH v5 1/4] crc_changed: eliminate copying of shared_ptr values Giuliano Procida
2022-06-30 16:40 ` Dodji Seketeli
2022-06-13 14:25 ` [PATCH v5 2/4] optional: minor improvements Giuliano Procida
2022-06-30 16:40 ` Dodji Seketeli
2022-06-13 14:25 ` [PATCH v5 3/4] Linux symbol CRCs: support 0 and report presence changes Giuliano Procida
2022-06-30 16:41 ` Dodji Seketeli
2022-06-13 14:25 ` [PATCH v5 4/4] add Linux kernel symbol namespace support Giuliano Procida
2022-07-01 12:54 ` Dodji Seketeli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YjMjKTt13CEPBszR@google.com \
--to=maennich@google.com \
--cc=dodji@seketeli.org \
--cc=gprocida@google.com \
--cc=kernel-team@android.com \
--cc=libabigail@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).