[PATCH v3 3/4] Linux symbol CRCs: support 0 and report presence changes
Giuliano Procida
gprocida@google.com
Thu Mar 17 16:38:57 GMT 2022
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
More information about the Libabigail
mailing list