[PATCH v2 3/4] add Linux kernel symbol namespace support

[Giuliano Procida <gprocida@google.com>]

Bug 28954 - add Linux Kernel symbol namespace support

Each Linux kernel symbol can be exported to a specified named
namespace or left in the global (nameless) namespace.

One complexity is that the offset symbol values require slightly
different interpretation for vmlinux and .ko loadable modules as the
former has a fixed load address but the latter are relocatable.

The global namespace is explicitly represented as the empty string, at
least when it comes to the value of __kstrtabns_FOO symbols, but the
common interpretation is that such symbols lack an export namespace.

I would rather not have to make use of "empty implies missing" in many
places, so the code here represents namespace as optional<string> and
only the symtab reader cares about empty strings in __ksymtab_strings.

	* include/abg-ir.h (elf_symbol::elf_symbol): Add ns argument.
	(elf_symbol::create): Add ns argument.
	(elf_symbol::get_namespace): Declare new function.
	(elf_symbol::set_namespace): Declare new function.
	and set_namespace.
	* src/abg-comp-filter.cc (namespace_changed): Define new
	helper functions.
	(categorize_harmful_diff_node): Also call namespace_changed().
	* src/abg-ir.cc (elf_symbol::priv): Add namespace_ member.
	(elf_symbol::priv::priv): Add namespace_ to initialisers.
	(elf_symbol::elf_symbol): Take new ns argument and pass it to
	priv constructor.
	(elf_symbol::create): Take new ns argument and pass it to
	elf_symbol constructor.
	(elf_symbol::get_namespace): Define new function.
	(elf_symbol::set_namespace): Define new function.
	* src/abg-reader.cc (build_elf_symbol): If namespace
	attribute is present, set symbol namespace.
	* src/abg-reporter-priv.cc (maybe_report_diff_for_symbol): If
	symbol namespaces differ, report this.
	* src/abg-symtab-reader.cc (symtab::load): Try to get
	__ksymtab_strings metadata and data. Use these to look up
	__kstrtabns_FOO namespace entries. Set symbol namespace where
	found.
	* src/abg-writer.cc (write_elf_symbol): Emit namespace
	attribute, if symbol has a namespace.
	* tests/data/Makefile.am: Add new test files.
	* tests/data/test-abidiff/test-namespace-0.xml: New test file.
	* tests/data/test-abidiff/test-namespace-1.xml: Likewise
	* tests/data/test-abidiff/test-namespace-report.txt: Likewise.
	* tests/test-abidiff.cc: Add new test case.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 include/abg-ir.h                              |  9 +++
 src/abg-comp-filter.cc                        | 40 +++++++++++-
 src/abg-ir.cc                                 | 30 ++++++++-
 src/abg-reader.cc                             |  7 +++
 src/abg-reporter-priv.cc                      | 18 ++++++
 src/abg-symtab-reader.cc                      | 61 +++++++++++++++++++
 src/abg-writer.cc                             |  3 +
 tests/data/Makefile.am                        |  3 +
 tests/data/test-abidiff/test-namespace-0.xml  | 15 +++++
 tests/data/test-abidiff/test-namespace-1.xml  | 15 +++++
 .../test-abidiff/test-namespace-report.txt    | 17 ++++++
 tests/test-abidiff.cc                         |  6 ++
 12 files changed, 221 insertions(+), 3 deletions(-)
 create mode 100644 tests/data/test-abidiff/test-namespace-0.xml
 create mode 100644 tests/data/test-abidiff/test-namespace-1.xml
 create mode 100644 tests/data/test-abidiff/test-namespace-report.txt

diff --git a/include/abg-ir.h b/include/abg-ir.h
index a2f4e1a7..7c42bea7 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"
@@ -921,6 +922,7 @@ private:
 	     visibility		vi,
 	     bool		is_in_ksymtab = false,
 	     uint64_t		crc = 0,
+	     const abg_compat::optional<std::string>&	ns = {},
 	     bool		is_suppressed = false);
 
   elf_symbol(const elf_symbol&);
@@ -946,6 +948,7 @@ public:
 	 visibility	    vi,
 	 bool		    is_in_ksymtab = false,
 	 uint64_t	    crc = 0,
+	 const abg_compat::optional<std::string>&	ns = {},
 	 bool		    is_suppressed = false);
 
   const environment*
@@ -1023,6 +1026,12 @@ public:
   void
   set_crc(uint64_t crc);
 
+  const abg_compat::optional<std::string>&
+  get_namespace() const;
+
+  void
+  set_namespace(const abg_compat::optional<std::string>& ns);
+
   bool
   is_suppressed() const;
 
diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index f90fdc78..c179b6bd 100644
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -256,6 +256,43 @@ crc_changed(const diff* diff)
   return false;
 }
 
+/// Test if there was a function or variable namespace change.
+///
+/// @param f the first function or variable to consider.
+///
+/// @param s the second function or variable to consider.
+///
+/// @return true if the test is positive, false otherwise.
+template <typename function_or_var_decl_sptr>
+static bool
+namespace_changed(const function_or_var_decl_sptr& f,
+		  const function_or_var_decl_sptr& s)
+{
+  const auto& symbol_f = f->get_symbol();
+  const auto& symbol_s = s->get_symbol();
+  if (!symbol_f || !symbol_s)
+    return false;
+  return symbol_f->get_namespace() != symbol_s->get_namespace();
+}
+
+/// Test if the current diff tree node carries a namespace change in
+/// either a function or a variable.
+///
+/// @param diff the diff tree node to consider.
+///
+/// @return true if the test is positive, false otherwise.
+static bool
+namespace_changed(const diff* diff)
+{
+  if (const function_decl_diff* d =
+	dynamic_cast<const function_decl_diff*>(diff))
+    return namespace_changed(d->first_function_decl(),
+			     d->second_function_decl());
+  if (const var_diff* d = dynamic_cast<const var_diff*>(diff))
+    return namespace_changed(d->first_var(), d->second_var());
+  return false;
+}
+
 /// Test if there was a function name change, but there there was no
 /// change in name of the underlying symbol.  IOW, if the name of a
 /// function changed, but the symbol of the new function is equal to
@@ -1783,7 +1820,8 @@ categorize_harmful_diff_node(diff *d, bool pre)
 	      || non_static_data_member_added_or_removed(d)
 	      || base_classes_added_or_removed(d)
 	      || has_harmful_enum_change(d)
-	      || crc_changed(d)))
+	      || crc_changed(d)
+	      || namespace_changed(d)))
 	category |= SIZE_OR_OFFSET_CHANGE_CATEGORY;
 
       if (has_virtual_mem_fn_change(d))
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 0ef5e8b2..78154622 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1728,6 +1728,7 @@ struct elf_symbol::priv
   bool			is_common_;
   bool			is_in_ksymtab_;
   uint64_t		crc_;
+  abg_compat::optional<std::string>	namespace_;
   bool			is_suppressed_;
   elf_symbol_wptr	main_symbol_;
   elf_symbol_wptr	next_alias_;
@@ -1745,6 +1746,7 @@ struct elf_symbol::priv
       is_common_(false),
       is_in_ksymtab_(false),
       crc_(0),
+      namespace_(),
       is_suppressed_(false)
   {}
 
@@ -1760,6 +1762,7 @@ struct elf_symbol::priv
        elf_symbol::visibility	  vi,
        bool			  is_in_ksymtab,
        uint64_t			  crc,
+       const abg_compat::optional<std::string>&	ns,
        bool			  is_suppressed)
     : env_(e),
       index_(i),
@@ -1773,6 +1776,7 @@ struct elf_symbol::priv
       is_common_(c),
       is_in_ksymtab_(is_in_ksymtab),
       crc_(crc),
+      namespace_(ns),
       is_suppressed_(is_suppressed)
   {
     if (!is_common_)
@@ -1818,6 +1822,8 @@ elf_symbol::elf_symbol()
 /// @param vi the visibility of the symbol.
 ///
 /// @param crc the CRC (modversions) value of Linux Kernel symbols
+///
+/// @param ns the namespace of Linux Kernel symbols, if any
 elf_symbol::elf_symbol(const environment* e,
 		       size_t		  i,
 		       size_t		  s,
@@ -1830,6 +1836,7 @@ elf_symbol::elf_symbol(const environment* e,
 		       visibility	  vi,
 		       bool		  is_in_ksymtab,
 		       uint64_t		  crc,
+		       const abg_compat::optional<std::string>&	ns,
 		       bool		  is_suppressed)
   : priv_(new priv(e,
 		   i,
@@ -1843,6 +1850,7 @@ elf_symbol::elf_symbol(const environment* e,
 		   vi,
 		   is_in_ksymtab,
 		   crc,
+		   ns,
 		   is_suppressed))
 {}
 
@@ -1886,6 +1894,8 @@ elf_symbol::create()
 ///
 /// @param crc the CRC (modversions) value of Linux Kernel symbols
 ///
+/// @param ns the namespace of Linux Kernel symbols, if any
+///
 /// @return a (smart) pointer to a newly created instance of @ref
 /// elf_symbol.
 elf_symbol_sptr
@@ -1901,10 +1911,11 @@ elf_symbol::create(const environment* e,
 		   visibility	      vi,
 		   bool		      is_in_ksymtab,
 		   uint64_t	      crc,
+		   const abg_compat::optional<std::string>&	ns,
 		   bool		      is_suppressed)
 {
   elf_symbol_sptr sym(new elf_symbol(e, i, s, n, t, b, d, c, ve, vi,
-				     is_in_ksymtab, crc, is_suppressed));
+				     is_in_ksymtab, crc, ns, is_suppressed));
   sym->priv_->main_symbol_ = sym;
   return sym;
 }
@@ -1927,7 +1938,8 @@ textually_equals(const elf_symbol&l,
 		 && 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())
     // These are variable symbols.  Let's compare their symbol size.
@@ -2147,6 +2159,20 @@ void
 elf_symbol::set_crc(uint64_t crc)
 {priv_->crc_ = crc;}
 
+/// Getter of the 'namespace' property.
+///
+/// @return the namespace for Linux Kernel symbols, if any
+const abg_compat::optional<std::string>&
+elf_symbol::get_namespace() const
+{return priv_->namespace_;}
+
+/// Setter of the 'namespace' property.
+///
+/// @param ns the new namespace for Linux Kernel symbols, if any
+void
+elf_symbol::set_namespace(const abg_compat::optional<std::string>& ns)
+{priv_->namespace_ = ns;}
+
 /// Getter for the 'is-suppressed' property.
 ///
 /// @return true iff the current symbol has been suppressed by a
diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 31885692..0a8ecd70 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -3269,6 +3269,13 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node,
   if (crc != 0)
     e->set_crc(crc);
 
+  if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "namespace"))
+    {
+      std::string ns;
+      xml::xml_char_sptr_to_string(s, ns);
+      e->set_namespace(ns);
+    }
+
   return e;
 }
 
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 7012f5dc..9ebcdf00 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -1158,6 +1158,24 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr&	symbol1,
 	  << std::noshowbase << std::dec
 	  << "\n";
     }
+
+  const abg_compat::optional<std::string>& ns1 = symbol1->get_namespace();
+  const abg_compat::optional<std::string>& ns2 = symbol2->get_namespace();
+  if (ns1 != ns2)
+    {
+      const std::string none = "(none)";
+      out << indent << "namespace changed from ";
+      if (ns1.has_value())
+	out << "'" << ns1.value() << "'";
+      else
+	out << none;
+      out << " to ";
+      if (ns2.has_value())
+	out << "'" << ns2.value() << "'";
+      else
+	out << none;
+      out << "\n";
+    }
 }
 
 /// For a given symbol, emit a string made of its name and version.
diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
index b42ce87d..e59bd62c 100644
--- a/src/abg-symtab-reader.cc
+++ b/src/abg-symtab-reader.cc
@@ -232,9 +232,33 @@ symtab::load_(Elf*	       elf_handle,
       return false;
     }
 
+  // The __kstrtab_strings sections is basically an ELF strtab but does not
+  // support elf_strptr lookups. A single call to elf_getdata gives a handle to
+  // section data.
+  //
+  // The value of a __kstrtabns_FOO (or other similar) symbol is an address
+  // within the __kstrtab_strings section. To look up the string value, we need
+  // to translate from load address to section offset by subtracting the base
+  // address of the section.
+  Elf_Scn* strings_section = elf_helpers::find_ksymtab_strings_section(elf_handle);
+  size_t strings_offset = 0;
+  const char* strings_data = nullptr;
+  size_t strings_size = 0;
+  if (strings_section)
+    {
+      GElf_Shdr strings_sheader;
+      gelf_getshdr(strings_section, &strings_sheader);
+      strings_offset = strings_sheader.sh_addr;
+      Elf_Data* data = elf_getdata(strings_section, nullptr);
+      ABG_ASSERT(data->d_off == 0);
+      strings_data = reinterpret_cast<const char *>(data->d_buf);
+      strings_size = data->d_size;
+    }
+
   const bool is_kernel = elf_helpers::is_linux_kernel(elf_handle);
   std::unordered_set<std::string> exported_kernel_symbols;
   std::unordered_map<std::string, uint64_t> crc_values;
+  std::unordered_map<std::string, std::string> namespaces;
 
   const bool is_arm32 = elf_helpers::architecture_is_arm32(elf_handle);
   const bool is_ppc64 = elf_helpers::architecture_is_ppc64(elf_handle);
@@ -288,6 +312,32 @@ symtab::load_(Elf*	       elf_handle,
 	  ABG_ASSERT(crc_values.emplace(name.substr(6), sym->st_value).second);
 	  continue;
 	}
+      if (strings_section && is_kernel && name.rfind("__kstrtabns_", 0) == 0)
+	{
+	  // This symbol lives in the __ksymtab_strings section but st_value is
+	  // a load address so, for vmlinux only, we need to subtract an offset
+	  // before looking it up in that section. Kernel module offsets must
+	  // not be adjusted. The simple way to distinguish the cases is to see
+	  // if the adjustment would overflow.
+          const size_t value = sym->st_value;
+	  const size_t offset =
+              value < strings_offset ? value : value - strings_offset;
+	  // check offset
+	  ABG_ASSERT(offset < strings_size);
+	  // find the terminating NULL
+	  const char* first = strings_data + offset;
+	  const char* last = strings_data + strings_size;
+	  const char* limit = std::find(first, last, 0);
+	  // check NULL found
+	  ABG_ASSERT(limit < last);
+	  // interpret the empty namespace name as no namespace name
+	  if (first < limit)
+	    {
+	      const std::string ns(first, limit - first);
+	      ABG_ASSERT(namespaces.emplace(name.substr(12), ns).second);
+	    }
+	  continue;
+	}
 
       // filter out uninteresting entries and only keep functions/variables for
       // now. The rest might be interesting in the future though.
@@ -402,6 +452,17 @@ symtab::load_(Elf*	       elf_handle,
 	symbol->set_crc(crc_entry.second);
     }
 
+  // Now add the namespaces
+  for (const auto& namespace_entry : namespaces)
+    {
+      const auto r = name_symbol_map_.find(namespace_entry.first);
+      if (r == name_symbol_map_.end())
+	continue;
+
+      for (const auto& symbol : r->second)
+	symbol->set_namespace(namespace_entry.second);
+    }
+
   // sort the symbols for deterministic output
   std::sort(symbols_.begin(), symbols_.end(), symbol_sort);
 
diff --git a/src/abg-writer.cc b/src/abg-writer.cc
index 7802128d..692abc75 100644
--- a/src/abg-writer.cc
+++ b/src/abg-writer.cc
@@ -3136,6 +3136,9 @@ write_elf_symbol(const elf_symbol_sptr&	sym,
       << std::hex << std::showbase << sym->get_crc() << "'"
       << std::dec << std::noshowbase;
 
+  if (sym->get_namespace().has_value())
+    o << " namespace='" << sym->get_namespace().value() << "'";
+
   o << "/>\n";
 
   return true;
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index a7eb7ff0..7ea24d58 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -91,6 +91,9 @@ 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-namespace-0.xml \
+test-abidiff/test-namespace-1.xml \
+test-abidiff/test-namespace-report.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-namespace-0.xml b/tests/data/test-abidiff/test-namespace-0.xml
new file mode 100644
index 00000000..5a9f5cd2
--- /dev/null
+++ b/tests/data/test-abidiff/test-namespace-0.xml
@@ -0,0 +1,15 @@
+<abi-corpus path='test.o' architecture='elf-amd-x86_64'>
+  <elf-variable-symbols>
+    <elf-symbol name='v1' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
+    <elf-symbol name='v2' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='this'/>
+    <elf-symbol name='v3' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='that'/>
+    <elf-symbol name='v4' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace=''/>
+  </elf-variable-symbols>
+  <abi-instr version='1.0' address-size='64' path='test.c' comp-dir-path='/tmp' language='LANG_C89'>
+    <type-decl name='int' size-in-bits='32' id='type-id-1'/>
+    <var-decl name='v1' type-id='type-id-1' mangled-name='v1' visibility='default' filepath='test.c' line='1' column='1' elf-symbol-id='v1'/>
+    <var-decl name='v2' type-id='type-id-1' mangled-name='v2' visibility='default' filepath='test.c' line='2' column='1' elf-symbol-id='v2'/>
+    <var-decl name='v3' type-id='type-id-1' mangled-name='v3' visibility='default' filepath='test.c' line='3' column='1' elf-symbol-id='v3'/>
+    <var-decl name='v4' type-id='type-id-1' mangled-name='v4' visibility='default' filepath='test.c' line='4' column='1' elf-symbol-id='v4'/>
+  </abi-instr>
+</abi-corpus>
diff --git a/tests/data/test-abidiff/test-namespace-1.xml b/tests/data/test-abidiff/test-namespace-1.xml
new file mode 100644
index 00000000..9814844b
--- /dev/null
+++ b/tests/data/test-abidiff/test-namespace-1.xml
@@ -0,0 +1,15 @@
+<abi-corpus path='test.o' architecture='elf-amd-x86_64'>
+  <elf-variable-symbols>
+    <elf-symbol name='v1' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='this'/>
+    <elf-symbol name='v2' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='that'/>
+    <elf-symbol name='v3' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace=''/>
+    <elf-symbol name='v4' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
+  </elf-variable-symbols>
+  <abi-instr version='1.0' address-size='64' path='test.c' comp-dir-path='/tmp' language='LANG_C89'>
+    <type-decl name='int' size-in-bits='32' id='type-id-1'/>
+    <var-decl name='v1' type-id='type-id-1' mangled-name='v1' visibility='default' filepath='test.c' line='1' column='1' elf-symbol-id='v1'/>
+    <var-decl name='v2' type-id='type-id-1' mangled-name='v2' visibility='default' filepath='test.c' line='2' column='1' elf-symbol-id='v2'/>
+    <var-decl name='v3' type-id='type-id-1' mangled-name='v3' visibility='default' filepath='test.c' line='3' column='1' elf-symbol-id='v3'/>
+    <var-decl name='v4' type-id='type-id-1' mangled-name='v4' visibility='default' filepath='test.c' line='4' column='1' elf-symbol-id='v4'/>
+  </abi-instr>
+</abi-corpus>
diff --git a/tests/data/test-abidiff/test-namespace-report.txt b/tests/data/test-abidiff/test-namespace-report.txt
new file mode 100644
index 00000000..d2c421ed
--- /dev/null
+++ b/tests/data/test-abidiff/test-namespace-report.txt
@@ -0,0 +1,17 @@
+Functions changes summary: 0 Removed, 0 Changed, 0 Added function
+Variables changes summary: 0 Removed, 4 Changed, 0 Added variables
+
+4 Changed variables:
+
+  [C] 'int v1' was changed:
+    namespace changed from (none) to 'this'
+
+  [C] 'int v2' was changed:
+    namespace changed from 'this' to 'that'
+
+  [C] 'int v3' was changed:
+    namespace changed from 'that' to ''
+
+  [C] 'int v4' was changed:
+    namespace changed from '' to (none)
+
diff --git a/tests/test-abidiff.cc b/tests/test-abidiff.cc
index 32858ece..009f7de5 100644
--- a/tests/test-abidiff.cc
+++ b/tests/test-abidiff.cc
@@ -128,6 +128,12 @@ static InOutSpec specs[] =
     "data/test-abidiff/test-crc-report.txt",
     "output/test-abidiff/test-crc-report-1-2.txt"
   },
+  {
+    "data/test-abidiff/test-namespace-0.xml",
+    "data/test-abidiff/test-namespace-1.xml",
+    "data/test-abidiff/test-namespace-report.txt",
+    "output/test-abidiff/test-namespace-report-0-1.txt"
+  },
   {
     "data/test-abidiff/test-PR27616-v0.xml",
     "data/test-abidiff/test-PR27616-v1.xml",
-- 
2.35.1.894.gb6a874cedc-goog