[v2,4/4] 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.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 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%)
  

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
>