[v3,4/4] add Linux kernel symbol namespace support

  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 symbol values which identify a string in
the __ksymtab_strings section must be interpretated differently for
vmlinux and .ko loadable modules as the former has a fixed load
address but the latter are relocatable. For vmlinux, the section base
address needs to be subtracted to obtain a section-relative offset.

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): Get ELF header to
	distinguish vmlinux from .ko. 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                              |  8 +++
 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                      | 66 +++++++++++++++++++
 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, 225 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
  

On Thu, Mar 17, 2022 at 04:38:58PM +0000, Giuliano Procida wrote:
>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 symbol values which identify a string in
>the __ksymtab_strings section must be interpretated differently for
>vmlinux and .ko loadable modules as the former has a fixed load
>address but the latter are relocatable. For vmlinux, the section base
>address needs to be subtracted to obtain a section-relative offset.
>
>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): Get ELF header to
>	distinguish vmlinux from .ko. 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                              |  8 +++
> 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                      | 66 +++++++++++++++++++
> 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, 225 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 b05a8c6f..7c558828 100644
>--- a/include/abg-ir.h
>+++ b/include/abg-ir.h
>@@ -922,6 +922,7 @@ private:
> 	     visibility		vi,
> 	     bool		is_in_ksymtab = false,
> 	     const abg_compat::optional<uint64_t>&	crc = {},
>+	     const abg_compat::optional<std::string>&	ns = {},
> 	     bool		is_suppressed = false);
>
>   elf_symbol(const elf_symbol&);
>@@ -947,6 +948,7 @@ public:
> 	 visibility	    vi,
> 	 bool		    is_in_ksymtab = false,
> 	 const abg_compat::optional<uint64_t>&		crc = {},
>+	 const abg_compat::optional<std::string>&	ns = {},
> 	 bool		    is_suppressed = false);
>
>   const environment*
>@@ -1024,6 +1026,12 @@ public:
>   void
>   set_crc(const abg_compat::optional<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 31590284..22da5244 100644
>--- a/src/abg-comp-filter.cc
>+++ b/src/abg-comp-filter.cc
>@@ -254,6 +254,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
>@@ -1781,7 +1818,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 853b01ee..f90be2e4 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_;
>   abg_compat::optional<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_(),
>+      namespace_(),
>       is_suppressed_(false)
>   {}
>
>@@ -1760,6 +1762,7 @@ struct elf_symbol::priv
>        elf_symbol::visibility	  vi,
>        bool			  is_in_ksymtab,
>        const abg_compat::optional<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,
> 		       const abg_compat::optional<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,
> 		   const abg_compat::optional<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;
> }
>@@ -1926,7 +1937,8 @@ 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() == 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.
>@@ -2146,6 +2158,20 @@ void
> elf_symbol::set_crc(const abg_compat::optional<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 7a9ad1f9..f9e420f1 100644
>--- a/src/abg-reader.cc
>+++ b/src/abg-reader.cc
>@@ -3265,6 +3265,13 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node,
>   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"))
>+    {
>+      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 f9dd928b..9ad733f4 100644
>--- a/src/abg-reporter-priv.cc
>+++ b/src/abg-reporter-priv.cc
>@@ -1168,6 +1168,24 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr&	symbol1,
>       out << 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..123ef84a 100644
>--- a/src/abg-symtab-reader.cc
>+++ b/src/abg-symtab-reader.cc
>@@ -204,6 +204,13 @@ symtab::load_(Elf*	       elf_handle,
> 	      ir::environment* env,
> 	      symbol_predicate is_suppressed)
> {
>+  GElf_Ehdr ehdr_mem;
>+  GElf_Ehdr* header = gelf_getehdr(elf_handle, &ehdr_mem);
>+  if (!header)
>+    {
>+      std::cerr << "Could not load get ELF header: Skipping symtab load.\n";

nit: "Could not load ELF header: ..."

>+      return false;
>+    }
>
>   Elf_Scn* symtab_section = elf_helpers::find_symbol_table_section(elf_handle);
>   if (!symtab_section)
>@@ -232,9 +239,34 @@ 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.

raw 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 vmlinux load address to section offset by subtracting the
>+  // base address of the section. This adjustment is not needed for loadable
>+  // modules which are relocatable.

... identifiable by ELF type ET_REL.

>+  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 = header->e_type == ET_REL ? 0 : 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;

Since you later assume strings_data and strings_size being valid based
on the existence of strings_section, perhaps assert that the extraction
worked?
        ABG_ASSERT(strings_data);
        ABG_ASSERT(strings_size > 0); // otherwise, why would there be a section?

>+    }
>+
>   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 +320,29 @@ 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 may
>+	  // be a vmlinux load address so we need to subtract the offset before
>+	  // looking it up in that section.
>+          const size_t value = sym->st_value;

odd indentation

>+	  const size_t offset = 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);

           ABG_ASSERT(namespaces.emplace(name.substr(12), std::move(ns)).second);


With the few nits addressed, feel free to add

Reviewed-by: Matthias Maennich <maennich@google.com>

Cheers,
Matthias

>+	    }
>+	  continue;
>+	}
>
>       // filter out uninteresting entries and only keep functions/variables for
>       // now. The rest might be interesting in the future though.
>@@ -402,6 +457,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 0a1e7b66..03fb658b 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().value()
>       << 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 90bd1934..7388697b 100644
>--- a/tests/data/Makefile.am
>+++ b/tests/data/Makefile.am
>@@ -93,6 +93,9 @@ test-abidiff/test-crc-2.xml \
> 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 \
> 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 a02bbe3d..93e44d6a 100644
>--- a/tests/test-abidiff.cc
>+++ b/tests/test-abidiff.cc
>@@ -128,6 +128,12 @@ static InOutSpec specs[] =
>     "data/test-abidiff/test-crc-report-1-2.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
>
  

Hi.

On Mon, 21 Mar 2022 at 12:53, Matthias Maennich <maennich@google.com> wrote:
>
> On Thu, Mar 17, 2022 at 04:38:58PM +0000, Giuliano Procida wrote:
> >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 symbol values which identify a string in
> >the __ksymtab_strings section must be interpretated differently for
> >vmlinux and .ko loadable modules as the former has a fixed load
> >address but the latter are relocatable. For vmlinux, the section base
> >address needs to be subtracted to obtain a section-relative offset.
> >
> >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): Get ELF header to
> >       distinguish vmlinux from .ko. 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                              |  8 +++
> > 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                      | 66 +++++++++++++++++++
> > 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, 225 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 b05a8c6f..7c558828 100644
> >--- a/include/abg-ir.h
> >+++ b/include/abg-ir.h
> >@@ -922,6 +922,7 @@ private:
> >            visibility         vi,
> >            bool               is_in_ksymtab = false,
> >            const abg_compat::optional<uint64_t>&      crc = {},
> >+           const abg_compat::optional<std::string>&   ns = {},
> >            bool               is_suppressed = false);
> >
> >   elf_symbol(const elf_symbol&);
> >@@ -947,6 +948,7 @@ public:
> >        visibility         vi,
> >        bool               is_in_ksymtab = false,
> >        const abg_compat::optional<uint64_t>&          crc = {},
> >+       const abg_compat::optional<std::string>&       ns = {},
> >        bool               is_suppressed = false);
> >
> >   const environment*
> >@@ -1024,6 +1026,12 @@ public:
> >   void
> >   set_crc(const abg_compat::optional<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 31590284..22da5244 100644
> >--- a/src/abg-comp-filter.cc
> >+++ b/src/abg-comp-filter.cc
> >@@ -254,6 +254,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
> >@@ -1781,7 +1818,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 853b01ee..f90be2e4 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_;
> >   abg_compat::optional<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_(),
> >+      namespace_(),
> >       is_suppressed_(false)
> >   {}
> >
> >@@ -1760,6 +1762,7 @@ struct elf_symbol::priv
> >        elf_symbol::visibility   vi,
> >        bool                     is_in_ksymtab,
> >        const abg_compat::optional<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,
> >                      const abg_compat::optional<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,
> >                  const abg_compat::optional<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;
> > }
> >@@ -1926,7 +1937,8 @@ 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() == 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.
> >@@ -2146,6 +2158,20 @@ void
> > elf_symbol::set_crc(const abg_compat::optional<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 7a9ad1f9..f9e420f1 100644
> >--- a/src/abg-reader.cc
> >+++ b/src/abg-reader.cc
> >@@ -3265,6 +3265,13 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node,
> >   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"))
> >+    {
> >+      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 f9dd928b..9ad733f4 100644
> >--- a/src/abg-reporter-priv.cc
> >+++ b/src/abg-reporter-priv.cc
> >@@ -1168,6 +1168,24 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr&     symbol1,
> >       out << 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..123ef84a 100644
> >--- a/src/abg-symtab-reader.cc
> >+++ b/src/abg-symtab-reader.cc
> >@@ -204,6 +204,13 @@ symtab::load_(Elf*               elf_handle,
> >             ir::environment* env,
> >             symbol_predicate is_suppressed)
> > {
> >+  GElf_Ehdr ehdr_mem;
> >+  GElf_Ehdr* header = gelf_getehdr(elf_handle, &ehdr_mem);
> >+  if (!header)
> >+    {
> >+      std::cerr << "Could not load get ELF header: Skipping symtab load.\n";
>
> nit: "Could not load ELF header: ..."
>
> >+      return false;
> >+    }
> >
> >   Elf_Scn* symtab_section = elf_helpers::find_symbol_table_section(elf_handle);
> >   if (!symtab_section)
> >@@ -232,9 +239,34 @@ 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.
>
> raw section data

Documentation actually refers to "washed" data, so "raw" doesn't sound right.
I'll insert "washed".

>
> >+  //
> >+  // 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 vmlinux load address to section offset by subtracting the
> >+  // base address of the section. This adjustment is not needed for loadable
> >+  // modules which are relocatable.
>
> ... identifiable by ELF type ET_REL.
>

Done.

> >+  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 = header->e_type == ET_REL ? 0 : 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;
>
> Since you later assume strings_data and strings_size being valid based
> on the existence of strings_section, perhaps assert that the extraction
> worked?
>         ABG_ASSERT(strings_data);
>         ABG_ASSERT(strings_size > 0); // otherwise, why would there be a section?
>

If the section is empty, null and 0 are expected. Later offset bounds
checking will assert if any lookups are actually attempted.
Any checking here is at best redundant and at worst (very
theoretically) premature.

If strings_data is outside mapped memory and strings_size is non-zero
that will SEGV. I don't think it's worth doing special checks for the
various strings_size == 0 possibilities.

> >+    }
> >+
> >   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 +320,29 @@ 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 may
> >+        // be a vmlinux load address so we need to subtract the offset before
> >+        // looking it up in that section.
> >+          const size_t value = sym->st_value;
>
> odd indentation
>

Done (tabification).

> >+        const size_t offset = 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);
>
>            ABG_ASSERT(namespaces.emplace(name.substr(12), std::move(ns)).second);
>

Done (inlined).

>
> With the few nits addressed, feel free to add
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
>
> Cheers,
> Matthias
>

Thanks!

Giuliano.

> >+          }
> >+        continue;
> >+      }
> >
> >       // filter out uninteresting entries and only keep functions/variables for
> >       // now. The rest might be interesting in the future though.
> >@@ -402,6 +457,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 0a1e7b66..03fb658b 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().value()
> >       << 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 90bd1934..7388697b 100644
> >--- a/tests/data/Makefile.am
> >+++ b/tests/data/Makefile.am
> >@@ -93,6 +93,9 @@ test-abidiff/test-crc-2.xml \
> > 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 \
> > 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 a02bbe3d..93e44d6a 100644
> >--- a/tests/test-abidiff.cc
> >+++ b/tests/test-abidiff.cc
> >@@ -128,6 +128,12 @@ static InOutSpec specs[] =
> >     "data/test-abidiff/test-crc-report-1-2.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
> >