[PATCH v4 3/4] Linux symbol CRCs: support 0 and report presence changes

[Giuliano Procida <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