Linux symbol CRCs: support 0 and report presence changes

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>

diff --git a/include/abg-ir.h b/include/abg-ir.h
index dad47eaf98b0d8493870cdac45408fcb1ab00773..5b7649f4d3850fcffc8c3b9236269d9868cb789a 100644 (file)
--- 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"
@@ -926,7 +927,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&);
@@ -951,7 +952,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*
@@ -1023,11 +1024,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 65f9732c90a5e3f4370ef3647af6a9496d07a30b..3e2d32086743a07e9d4e8d5265c7e8c1e4095d58 100644 (file)
--- 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 9bbe99f278530afa90326c104066a477867db08f..9a89808eb5067b7ef9b84aa6079e4dba76b2503d 100644 (file)
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1724,7 +1724,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_;
@@ -1741,7 +1741,7 @@ struct elf_symbol::priv
       is_defined_(false),
       is_common_(false),
       is_in_ksymtab_(false),
-      crc_(0),
+      crc_(),
       is_suppressed_(false)
   {}
 
@@ -1756,7 +1756,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),
@@ -1826,7 +1826,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,
@@ -1897,7 +1897,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,
@@ -1923,8 +1923,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.
@@ -2132,8 +2131,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_;}
 
@@ -2141,7 +2140,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 74ef9e51cd0189af60f62278d03ab2c2ee1266ed..53b8398da7a835304845a27d0160f624762d60b1 100644 (file)
--- 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 22851f3062e8ce9ff9ac6eb21cbee6e6226b4d4b..5b53fdaab5080c57b66b6304c1d452fe5b278046 100644 (file)
--- 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 1863b3471b7d31a4cdf8bcf76c4c404faf3f06b4..9bfea5d679ea89e6d72f9e196d369ef7729b5812 100644 (file)
--- a/src/abg-writer.cc
+++ b/src/abg-writer.cc
@@ -3193,10 +3193,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 9300c8546fdf0ac99e8645259e99b9ea4722df6f..ffd6bf26a06fcd47194894464258b36bff9062f1 100644 (file)
--- 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 (file)
index 0000000..0db42f6
--- /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 (file)
index 0000000..e11f29c
--- /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 622f7177893d0a7e6d840072bf37274e3d6eb4c9..63153b1dab4628336450d6c6230433ff1b031e71 100644 (file)
--- 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 7dc2504240cff6e94651138c1aeb88a09b80dce2..efef9a9962d27f90084e6f0c063c06527c42a0ea 100644 (file)
--- 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());
   }
 }