From: Giuliano Procida <gprocida@google.com>
To: libabigail@sourceware.org
Cc: dodji@seketeli.org, kernel-team@android.com, gprocida@google.com,
maennich@google.com
Subject: [PATCH v3 3/4] Linux symbol CRCs: support 0 and report presence changes
Date: Thu, 17 Mar 2022 16:38:57 +0000 [thread overview]
Message-ID: <20220317163858.353762-4-gprocida@google.com> (raw)
In-Reply-To: <20220317163858.353762-1-gprocida@google.com>
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.
Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/abg-ir.h | 9 +++++----
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, 77 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 a2f4e1a7..b05a8c6f 100644
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -21,6 +21,7 @@
#include <functional>
#include <set>
#include <unordered_map>
+#include "abg-cxx-compat.h"
#include "abg-fwd.h"
#include "abg-hash.h"
#include "abg-traverse.h"
@@ -920,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 = {},
bool is_suppressed = false);
elf_symbol(const elf_symbol&);
@@ -945,7 +946,7 @@ public:
const version& ve,
visibility vi,
bool is_in_ksymtab = false,
- uint64_t crc = 0,
+ const abg_compat::optional<uint64_t>& crc = {},
bool is_suppressed = false);
const environment*
@@ -1017,11 +1018,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);
bool
is_suppressed() const;
diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index f90fdc78..31590284 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 0ef5e8b2..853b01ee 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_;
bool is_suppressed_;
elf_symbol_wptr main_symbol_;
elf_symbol_wptr next_alias_;
@@ -1744,7 +1744,7 @@ struct elf_symbol::priv
is_defined_(false),
is_common_(false),
is_in_ksymtab_(false),
- crc_(0),
+ crc_(),
is_suppressed_(false)
{}
@@ -1759,7 +1759,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,
bool is_suppressed)
: env_(e),
index_(i),
@@ -1829,7 +1829,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,
bool is_suppressed)
: priv_(new priv(e,
i,
@@ -1900,7 +1900,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,
bool is_suppressed)
{
elf_symbol_sptr sym(new elf_symbol(e, i, s, n, t, b, d, c, ve, vi,
@@ -1926,8 +1926,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());
if (equals && l.is_variable())
// These are variable symbols. Let's compare their symbol size.
@@ -2135,8 +2134,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_;}
@@ -2144,7 +2143,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 for the 'is-suppressed' property.
diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 31885692..7a9ad1f9 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));
return e;
}
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 7012f5dc..f9dd928b 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 7802128d..0a1e7b66 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 << "'";
o << "/>\n";
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index a7eb7ff0..90bd1934 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-PR27985-report.txt \
test-abidiff/test-PR27985-v0.c \
test-abidiff/test-PR27985-v0.o \
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 32858ece..a02bbe3d 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 16:39 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
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 ` Giuliano Procida [this message]
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=20220317163858.353762-4-gprocida@google.com \
--to=gprocida@google.com \
--cc=dodji@seketeli.org \
--cc=kernel-team@android.com \
--cc=libabigail@sourceware.org \
--cc=maennich@google.com \
/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).