[applied] Bug 29912 - Better support an ELF symbol alias that designates several functions

Message ID 878rfoojco.fsf@redhat.com
State New
Headers
Series [applied] Bug 29912 - Better support an ELF symbol alias that designates several functions |

Commit Message

Dodji Seketeli March 22, 2023, 4:12 p.m. UTC
  Hello,

When an ELF symbol alias designates two different functions,
Libabigail can be confused as to which function to consider.

This confusion indirectly leads to showing spurious changes in the
return type of some functions when an ELF symbol designates more than
one function.

In other words, when an ELF symbol designates two (or more) functions,
the comparison engine needs a way to tell the two functions apart.  It
needs an other way to identify the functions.

This patch fixes the confusion by using the pretty representation of
the functions in those cases.

Please note that to replicate the issue reported in this bug, here is
the command I used:

    $ fedabipkgdiff --debug --self-compare -a --from fc37 smesh

	* include/abg-corpus.h (corpus::lookup_functions): Return a set of
	functions rather than a vector of functions where a function can
	be present more than once.  This allows to determine if a symbol
	designates more than one function.
	(corpus::exported_decls_builder::priv_): Make this public so that
	some outside code can access it.
	(corpus::exported_decls_builder::fn_id_maps_to_several_fns):
	Declare new function.
	(corpus::exported_decls_builder::maybe_add_fn_to_exported_fns):
	Remove useless const here.
	* include/abg-fwd.h (get_function_id_or_pretty_representation):
	Declare new function.
	* include/abg-ir.h
	(elf_symbol::get_alias_with_default_symbol_version): Declare new
	member function.
	* src/abg-comparison.cc
	(corpus_diff::priv::ensure_lookup_tables_populated): Use the new
	get_function_id_or_pretty_representation rather than
	function_decl::get_id() to identify a function.
	* src/abg-corpus-priv.h (str_fn_ptr_set_map_type): Define this new
	typedef of unordered_map<string, std::unordered_set<function_decl*> >.
	(corpus::exported_decls_builder::priv::id_fns_map_): Change the
	type of this to the new str_fn_ptr_set_map_type.
	(corpus::exported_decls_builder::priv::{id_fns_map, fn_id_is_in_id_fns_map,
	fn_is_in_fns, fn_is_in_id_fns_map}): Adjust to using a set of
	functions rather than a vector.
	(corpus::exported_decls_builder::fn_is_in_fns_by_repr): Define new
	static function.
	(corpus::exported_decls_builder::add_fn_to_exported): Remove
	useless const.
	* src/abg-corpus.cc
	(corpus::exported_decls_builder::fn_id_maps_to_several_fns):
	Define new function.
	(corpus::exported_decls_builder::maybe_add_fn_to_exported_fns):
	Remove useless const.
	(corpus::lookup_functions): Return a set of functions rather than
	a vector of functions where a function can be present more than
	once.  This allows to determine if a symbol designates more than
	one function.
	* src/abg-dwarf-reader.cc
	(reader::symbol_already_belongs_to_a_function): Adjust.
	* src/abg-fe-iface.cc (fe_iface::maybe_add_fn_to_exported_decls):
	Adjust.
	* src/abg-ir.cc (get_function_id_or_pretty_representation): Define
	new function.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 include/abg-corpus.h    | 13 +++---
 include/abg-fwd.h       |  3 ++
 include/abg-ir.h        |  3 ++
 src/abg-comparison.cc   |  4 +-
 src/abg-corpus-priv.h   | 96 ++++++++++++++++++++++++++++++-----------
 src/abg-corpus.cc       | 29 +++++++++++--
 src/abg-dwarf-reader.cc | 15 +++----
 src/abg-fe-iface.cc     |  2 +-
 src/abg-ir.cc           | 24 +++++++++++
 9 files changed, 142 insertions(+), 47 deletions(-)
  

Patch

diff --git a/include/abg-corpus.h b/include/abg-corpus.h
index 090bad14..cbc7ec24 100644
--- a/include/abg-corpus.h
+++ b/include/abg-corpus.h
@@ -218,7 +218,7 @@  public:
   virtual const functions&
   get_functions() const;
 
-  const vector<function_decl*>*
+  const std::unordered_set<function_decl*>*
   lookup_functions(const string& id) const;
 
   void
@@ -302,13 +302,13 @@  operator&=(corpus::origin &l, corpus::origin r);
 /// parameters needed.
 class corpus::exported_decls_builder
 {
-  class priv;
-  std::unique_ptr<priv> priv_;
-
   // Forbid default construction.
   exported_decls_builder();
 
 public:
+  class priv;
+  std::unique_ptr<priv> priv_;
+
   friend class corpus;
 
   exported_decls_builder(functions& fns,
@@ -327,6 +327,9 @@  public:
   functions&
   exported_functions();
 
+  std::unordered_set<function_decl*>*
+  fn_id_maps_to_several_fns(function_decl*);
+
   const variables&
   exported_variables() const;
 
@@ -334,7 +337,7 @@  public:
   exported_variables();
 
   void
-  maybe_add_fn_to_exported_fns(const function_decl*);
+  maybe_add_fn_to_exported_fns(function_decl*);
 
   void
   maybe_add_var_to_exported_vars(const var_decl*);
diff --git a/include/abg-fwd.h b/include/abg-fwd.h
index fc3f4374..747a625c 100644
--- a/include/abg-fwd.h
+++ b/include/abg-fwd.h
@@ -1004,6 +1004,9 @@  get_function_type_name(const function_type*, bool internal = false);
 interned_string
 get_function_type_name(const function_type&, bool internal = false);
 
+interned_string
+get_function_id_or_pretty_representation(function_decl *fn);
+
 interned_string
 get_method_type_name(const method_type_sptr&, bool internal = false);
 
diff --git a/include/abg-ir.h b/include/abg-ir.h
index 0ca8afcf..6d81097a 100644
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -1094,6 +1094,9 @@  public:
   elf_symbol_sptr
   get_alias_which_equals(const elf_symbol& other) const;
 
+  elf_symbol_sptr
+  get_alias_with_default_symbol_version() const;
+
   string
   get_aliases_id_string(const string_elf_symbols_map_type& symtab,
 			bool include_symbol_itself = true) const;
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 1533d9d1..baae56a3 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -9036,7 +9036,7 @@  corpus_diff::priv::ensure_lookup_tables_populated()
 	ABG_ASSERT(i < first_->get_functions().size());
 
 	function_decl* deleted_fn = first_->get_functions()[i];
-	string n = deleted_fn->get_id();
+	string n = get_function_id_or_pretty_representation(deleted_fn);
 	ABG_ASSERT(!n.empty());
 	// The below is commented out because there can be several
 	// functions with the same ID in the corpus.  So several
@@ -9056,7 +9056,7 @@  corpus_diff::priv::ensure_lookup_tables_populated()
 	  {
 	    unsigned i = *iit;
 	    function_decl* added_fn = second_->get_functions()[i];
-	    string n = added_fn->get_id();
+	    string n = get_function_id_or_pretty_representation(added_fn);
 	    ABG_ASSERT(!n.empty());
 	    // The below is commented out because there can be several
 	    // functions with the same ID in the corpus.  So several
diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
index 422b7870..412db377 100644
--- a/src/abg-corpus-priv.h
+++ b/src/abg-corpus-priv.h
@@ -40,6 +40,12 @@  typedef vector<regex_t_sptr> regex_t_sptrs_type;
 /// Convenience typedef for a hash map which key is a string and which
 /// data is a vector of abigail::ir::function_decl*
 typedef unordered_map<string, vector<function_decl*> > str_fn_ptrs_map_type;
+
+/// Convenience typedef for a hash map which key is a string and which
+/// data is a set of abigail::ir::function_decl*
+typedef unordered_map<string, std::unordered_set<function_decl*> >
+str_fn_ptr_set_map_type;
+
 /// Convenience typedef for a hash map which key is a string and
 /// which data is an abigail::ir::var_decl*.
 typedef unordered_map<string, var_decl*> str_var_ptr_map_type;
@@ -63,7 +69,7 @@  class corpus::exported_decls_builder::priv
   // template parameters of the second instantiation are just typedefs
   // of the first instantiation, for instance.  So there can be cases
   // where one ID appertains to more than one function.
-  str_fn_ptrs_map_type	id_fns_map_;
+  str_fn_ptr_set_map_type	id_fns_map_;
   str_var_ptr_map_type	id_var_map_;
   strings_type&	fns_suppress_regexps_;
   regex_t_sptrs_type	compiled_fns_suppress_regexp_;
@@ -197,7 +203,7 @@  public:
   ///
   /// @return a map which key is a string and which data is a pointer
   /// to a function.
-  const str_fn_ptrs_map_type&
+  const str_fn_ptr_set_map_type&
   id_fns_map() const
   {return id_fns_map_;}
 
@@ -210,7 +216,7 @@  public:
   ///
   /// @return a map which key is a string and which data is a pointer
   /// to a function.
-  str_fn_ptrs_map_type&
+  str_fn_ptr_set_map_type&
   id_fns_map()
   {return id_fns_map_;}
 
@@ -267,11 +273,11 @@  public:
   ///
   /// @return the pointer to the vector of functions with ID @p fn_id,
   /// or nil if no function with that ID exists.
-  vector<function_decl*>*
+  std::unordered_set<function_decl*>*
   fn_id_is_in_id_fns_map(const string& fn_id)
   {
-    str_fn_ptrs_map_type& m = id_fns_map();
-    str_fn_ptrs_map_type::iterator i = m.find(fn_id);
+    str_fn_ptr_set_map_type& m = id_fns_map();
+    str_fn_ptr_set_map_type::iterator i = m.find(fn_id);
     if (i == m.end())
       return 0;
     return &i->second;
@@ -286,47 +292,87 @@  public:
   /// @p fn, that are present in the id-functions map, or nil if no
   /// function with the same ID as @p fn is present in the
   /// id-functions map.
-  vector<function_decl*>*
+  std::unordered_set<function_decl*>*
   fn_id_is_in_id_fns_map(const function_decl* fn)
   {
     string fn_id = fn->get_id();
     return fn_id_is_in_id_fns_map(fn_id);
   }
 
-  /// Test if a given function is present in a vector of functions.
+  /// Test if a given function is present in a set of functions.
   ///
   /// The function compares the ID and the qualified name of
   /// functions.
   ///
   /// @param fn the function to consider.
   ///
-  /// @parm fns the vector of functions to consider.
+  /// @parm fns the set of functions to consider.
   static bool
-  fn_is_in_fns(const function_decl* fn, const vector<function_decl*>& fns)
+  fn_is_in_fns(function_decl* fn,
+	       const std::unordered_set<function_decl*>& fns)
   {
     if (fns.empty())
       return false;
 
+    if (fns.find(fn) != fns.end())
+      return true;
+
     const string fn_id = fn->get_id();
-    for (vector<function_decl*>::const_iterator i = fns.begin();
-	 i != fns.end();
-	 ++i)
-      if ((*i)->get_id() == fn_id
-	  && (*i)->get_qualified_name() == fn->get_qualified_name())
+    for (const auto f : fns)
+      if (f->get_id() == fn_id
+	  && f->get_qualified_name() == fn->get_qualified_name())
 	return true;
 
     return false;
   }
 
+  /// Test if a given function is present in a set of functions,
+  /// by looking at the pretty representation of the function, in
+  /// addition to looking at its ID.
+  ///
+  /// This is useful because sometimes a given ELF symbol (alias)
+  /// might be for several different functions.  In that case, using
+  /// the function pretty representation might be a way to
+  /// differentiate the functions having the same ELF symbol alias.
+  ///
+  /// The function compares the ID and the qualified name of
+  /// functions.
+  ///
+  /// @param fn the function to consider.
+  ///
+  /// @parm fns the set of functions to consider.
+  ///
+  /// @return true if @p fn is present in @p fns.
+  static bool
+  fn_is_in_fns_by_repr(function_decl* fn,
+		       const std::unordered_set<function_decl*>& fns,
+		       string& pretty_representation)
+  {
+    if (!fn_is_in_fns(fn, fns))
+      return false;
+
+    const string repr = fn->get_pretty_representation();
+    const string fn_id = fn->get_id();
+    for (const auto f : fns)
+      if (f->get_id() == fn_id
+	  && f->get_pretty_representation() == repr)
+	{
+	  pretty_representation = repr;
+	  return true;
+	}
+
+    return false;
+  }
+
   ///  Test if a function is in the id-functions map.
   ///
   ///  @param fn the function to consider.
   ///
   ///  @return true iff the function is in the id-functions map.
   bool
-  fn_is_in_id_fns_map(const function_decl* fn)
+  fn_is_in_id_fns_map(function_decl* fn)
   {
-    vector<function_decl*>* fns = fn_id_is_in_id_fns_map(fn);
+    std::unordered_set<function_decl*>* fns = fn_id_is_in_id_fns_map(fn);
     if (fns && fn_is_in_fns(fn, *fns))
       return true;
     return false;
@@ -344,10 +390,10 @@  public:
 
     // First associate the function id to the function.
     string fn_id = fn->get_id();
-    vector<function_decl*>* fns = fn_id_is_in_id_fns_map(fn_id);
+    std::unordered_set<function_decl*>* fns = fn_id_is_in_id_fns_map(fn_id);
     if (!fns)
-      fns = &(id_fns_map()[fn_id] = vector<function_decl*>());
-    fns->push_back(fn);
+      fns = &(id_fns_map()[fn_id] = std::unordered_set<function_decl*>());
+    fns->insert(fn);
 
     // Now associate all aliases of the underlying symbol to the
     // function too.
@@ -361,8 +407,8 @@  public:
 	  goto loop;
 	fns = fn_id_is_in_id_fns_map(fn_id);
 	if (!fns)
-	  fns = &(id_fns_map()[fn_id] = vector<function_decl*>());
-	fns->push_back(fn);
+	  fns = &(id_fns_map()[fn_id] = std::unordered_set<function_decl*>());
+	fns->insert(fn);
       loop:
 	sym = sym->get_next_alias();
       }
@@ -403,12 +449,12 @@  public:
   ///
   /// @param fn the function to add to the set of exported functions.
   void
-  add_fn_to_exported(const function_decl* fn)
+  add_fn_to_exported(function_decl* fn)
   {
     if (!fn_is_in_id_fns_map(fn))
       {
-	fns_.push_back(const_cast<function_decl*>(fn));
-	add_fn_to_id_fns_map(const_cast<function_decl*>(fn));
+	fns_.push_back(fn);
+	add_fn_to_id_fns_map(fn);
       }
   }
 
diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc
index cf6685d6..fa308151 100644
--- a/src/abg-corpus.cc
+++ b/src/abg-corpus.cc
@@ -109,6 +109,28 @@  corpus::functions&
 corpus::exported_decls_builder::exported_functions()
 {return priv_->fns_;}
 
+/// Test if a given function ID maps to several functions in the same corpus.
+///
+/// The magic of ELF symbol aliases makes it possible for an ELF
+/// symbol alias to designate several different functions.  This
+/// function tests if the ELF symbol of a given function has a aliases
+/// that designates another function or not.
+///
+/// @param fn the function to consider.
+///
+/// @return the set of functions designated by the ELF symbol of @p
+/// fn, or nullptr if the function ID maps to just @p fn.
+std::unordered_set<function_decl*>*
+corpus::exported_decls_builder::fn_id_maps_to_several_fns(function_decl* fn)
+{
+  std::unordered_set<function_decl*> *fns_for_id =
+    priv_->fn_id_is_in_id_fns_map(fn);
+  if (fns_for_id && fns_for_id->size() > 1)
+    return fns_for_id;
+
+  return nullptr;
+}
+
 /// Getter for the reference to the vector of exported variables.
 /// This vector is shared with with the @ref corpus.  It's where the
 /// set of exported variable is ultimately stored.
@@ -133,7 +155,7 @@  corpus::exported_decls_builder::exported_variables()
 ///
 /// @param fn the function to add the set of exported functions.
 void
-corpus::exported_decls_builder::maybe_add_fn_to_exported_fns(const function_decl* fn)
+corpus::exported_decls_builder::maybe_add_fn_to_exported_fns(function_decl* fn)
 {
   if (!fn->get_is_in_public_symbol_table())
     return;
@@ -1314,12 +1336,11 @@  corpus::get_functions() const
 ///
 /// @return the vector functions which ID is @p id, or nil if no
 /// function with that ID was found.
-const vector<function_decl*>*
+const std::unordered_set<function_decl*>*
 corpus::lookup_functions(const string& id) const
 {
   exported_decls_builder_sptr b = get_exported_decls_builder();
-  str_fn_ptrs_map_type::const_iterator i =
-    b->priv_->id_fns_map_.find(id);
+  auto i = b->priv_->id_fns_map_.find(id);
   if (i == b->priv_->id_fns_map_.end())
     return 0;
   return &i->second;
diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 037538af..66b99fd5 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -4460,19 +4460,14 @@  public:
 
     string id = fn->get_id_string();
 
-    const vector<function_decl*> *fns = corp->lookup_functions(id);
+    const std::unordered_set<function_decl*> *fns = corp->lookup_functions(id);
     if (!fns)
       return false;
 
-    for (vector<function_decl*>::const_iterator i = fns->begin();
-	 i != fns->end();
-	 ++i)
-      {
-	function_decl* f = *i;
-	ABG_ASSERT(f);
-	if (f->get_symbol())
-	  return true;
-      }
+    for (auto f : *fns)
+      if (f->get_symbol())
+	return true;
+
     return false;
   }
 
diff --git a/src/abg-fe-iface.cc b/src/abg-fe-iface.cc
index f3798cd6..bc272425 100644
--- a/src/abg-fe-iface.cc
+++ b/src/abg-fe-iface.cc
@@ -310,7 +310,7 @@  fe_iface::maybe_add_fn_to_exported_decls(const function_decl* fn)
   if (fn)
     if (corpus::exported_decls_builder* b =
 	corpus()->get_exported_decls_builder().get())
-      b->maybe_add_fn_to_exported_fns(fn);
+      b->maybe_add_fn_to_exported_fns(const_cast<function_decl*>(fn));
 }
 
 /// Try and add the representation of the ABI of a variable to the set
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 047b755d..514013c7 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -8900,6 +8900,30 @@  get_function_type_name(const function_type& fn_type,
   return env.intern(o.str());
 }
 
+/// Get the ID of a function, or, if the ID can designate several
+/// different functions, get its pretty representation.
+///
+/// @param fn the function to consider
+///
+/// @return the function ID of pretty representation of @p fn.
+interned_string
+get_function_id_or_pretty_representation(function_decl *fn)
+{
+  ABG_ASSERT(fn);
+
+  interned_string result = fn->get_environment().intern(fn->get_id());
+
+  if (corpus *c = fn->get_corpus())
+    {
+      corpus::exported_decls_builder_sptr b =
+	c->get_exported_decls_builder();
+      if (b->fn_id_maps_to_several_fns(fn))
+	result = fn->get_environment().intern(fn->get_pretty_representation());
+    }
+
+  return result;
+}
+
 /// Get the name of a given method type and return a copy of it.
 ///
 /// @param fn_type the function type to consider.