[applied] corpus: Allow several variables with same ID to be exported

Message ID 87ed5wh567.fsf@redhat.com
State New
Headers
Series [applied] corpus: Allow several variables with same ID to be exported |

Commit Message

Dodji Seketeli Sept. 6, 2024, 8:56 p.m. UTC
  Hello,

The libQt6Network.so.6.3.1 exhibits some interesting debug info where
two variables with slightly different qualified names have the same ELF
symbol.

Consider the DIE for this variable:
 [ 1aab3]    variable             abbrev: 118
             name                 (GNU_strp_alt) "backendMutex"
             decl_file            (implicit_const) qsslsocket_p.h (42)
             decl_line            (data1) 196
             decl_column          (data1) 26
             linkage_name         (GNU_strp_alt) "_ZN17QSslSocketPrivate12backendMutexE"
             type                 (ref_udata) [ 1aa35]
             external             (flag_present) yes
             declaration          (flag_present) yes
             inline               (implicit_const) declared_inlined (3)
             location             (exprloc)
              [ 0] addr +0x1e3050 <_ZN17QSslSocketPrivate12backendMutexE>

This DIE designates a variable named backendMutex in the global
namespace.  It's associated to the ELF symbol
_ZN17QSslSocketPrivate12backendMutexE.

Then, later, there is this DIE:

 [d05d3e]    class_type           abbrev: 260
             name                 (GNU_strp_alt) "QSslSocketPrivate"
             declaration          (flag_present) yes
             sibling              (ref_udata) [d05d70]
[...]
 [d05d70]    variable             abbrev: 642
             name                 (GNU_strp_alt) "backendMutex"
             decl_file            (implicit_const) qsslsocket_p.h (54)
             decl_line            (data1) 196
             decl_column          (data1) 26
             linkage_name         (GNU_strp_alt) "_ZN17QSslSocketPrivate12backendMutexE"
             type                 (ref_addr) [  fdc5]
             external             (flag_present) yes
             declaration          (flag_present) yes
             inline               (implicit_const) declared_inlined (3)
             location             (exprloc)
              [ 0] addr +0x1e3050 <_ZN17QSslSocketPrivate12backendMutexE>

Here, this DIE represents a static data member named
QSslSocketPrivate::backendMutex, as it's member of the
QSslSocketPrivate class.  Note how it's also associated with the ELF
symbol _ZN17QSslSocketPrivate12backendMutexE.

So both variables are associated with the same ELF symbol and should
thus be exported from the ABI corpus.

Today, only one of these variables is represented as exported in the
corpus.  Depending on the order in which the variables are analyzed,
the variable that is represented as exported might be either one or
the other, leading to some spurious self-comparison errors.

This patch fixes the issue by allowing the corpus to represent more
than one variable having a given ELF symbol to be exported.  This is
similar to what is done already for functions.

This fixes the self comparison of the qt6-qtbase package, which can be
witnessed by issuing the command:

    $ fedabipkgdiff --self-compare -a --from fc36 qt6-qtbase

	* include/abg-corpus.h (corpus::lookup_variables): Replace
	corpus::lookup_variable with this one which returns the set of
	variables associated with a given ID of variable.
	* src/abg-corpus-priv.h (istr_var_ptr_set_map_type): Define new
	typedef.
	(corpus::exported_decls_builder::priv::id_vars_map_): Replace
	id_var_map_ with this new data member of type
	istr_var_ptr_set_map_type.
	(corpus::exported_decls_builder::priv::id_fns_map): Fix comment.
	(corpus::exported_decls_builder::priv::id_vars_map): Replace
	previous id_var_map with this member function.
	(corpus::exported_decls_builder::priv::var_id_is_in_id_vars_map):
	Replace the var_id_is_in_id_var_map member function.
	(corpus::exported_decls_builder::priv::var_is_in_vars): Define new
	static member function.
	(corpus::exported_decls_builder::priv::{var_is_in_id_vars_map, add_var_to_id_vars_map}):
	Define new member functions.
	(corpus::exported_decls_builder::priv::add_var_to_map): Remove.
	(corpus::exported_decls_builder::priv::add_var_to_exported):
	Adjust by using the new var_is_in_id_vars_map and
	add_var_to_id_vars_map.
	* src/abg-corpus.cc
	(corpus::exported_decls_builder::maybe_add_var_to_exported_vars):
	Adjust, use the new
	corpus::exported_decls_builder::priv::var_is_in_id_vars_map.
	(corpus::lookup_variables): Replace corpus::lookup_variable with
	this one which returns the set of variables associated with a
	given ID of variable.
	* tools/abicompat.cc
	(compare_expected_against_provided_variables): Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 include/abg-corpus.h  |   7 +-
 src/abg-corpus-priv.h | 171 ++++++++++++++++++++++++++++++++----------
 src/abg-corpus.cc     |  34 ++++++---
 tools/abicompat.cc    |  39 +++++-----
 4 files changed, 182 insertions(+), 69 deletions(-)
  

Comments

Ben Woodard Sept. 6, 2024, 9:58 p.m. UTC | #1
On 9/6/24 1:56 PM, Dodji Seketeli wrote:
> Hello,
>
> The libQt6Network.so.6.3.1 exhibits some interesting debug info where
> two variables with slightly different qualified names have the same ELF
> symbol.
>
> Consider the DIE for this variable:
>   [ 1aab3]    variable             abbrev: 118
>               name                 (GNU_strp_alt) "backendMutex"
>               decl_file            (implicit_const) qsslsocket_p.h (42)
>               decl_line            (data1) 196
>               decl_column          (data1) 26
>               linkage_name         (GNU_strp_alt) "_ZN17QSslSocketPrivate12backendMutexE"
>               type                 (ref_udata) [ 1aa35]
>               external             (flag_present) yes
>               declaration          (flag_present) yes
>               inline               (implicit_const) declared_inlined (3)
>               location             (exprloc)
>                [ 0] addr +0x1e3050 <_ZN17QSslSocketPrivate12backendMutexE>
>
> This DIE designates a variable named backendMutex in the global
> namespace.  It's associated to the ELF symbol
> _ZN17QSslSocketPrivate12backendMutexE.
>
> Then, later, there is this DIE:
>
>   [d05d3e]    class_type           abbrev: 260
>               name                 (GNU_strp_alt) "QSslSocketPrivate"
>               declaration          (flag_present) yes
>               sibling              (ref_udata) [d05d70]
> [...]
>   [d05d70]    variable             abbrev: 642
>               name                 (GNU_strp_alt) "backendMutex"
>               decl_file            (implicit_const) qsslsocket_p.h (54)
>               decl_line            (data1) 196
>               decl_column          (data1) 26
>               linkage_name         (GNU_strp_alt) "_ZN17QSslSocketPrivate12backendMutexE"
>               type                 (ref_addr) [  fdc5]
>               external             (flag_present) yes
>               declaration          (flag_present) yes
>               inline               (implicit_const) declared_inlined (3)
>               location             (exprloc)
>                [ 0] addr +0x1e3050 <_ZN17QSslSocketPrivate12backendMutexE>
>
> Here, this DIE represents a static data member named
> QSslSocketPrivate::backendMutex, as it's member of the
> QSslSocketPrivate class.  Note how it's also associated with the ELF
> symbol _ZN17QSslSocketPrivate12backendMutexE.

How does that work? How can they have the same C linkage name and work 
properly? What am I missing here?

-ben

>
> So both variables are associated with the same ELF symbol and should
> thus be exported from the ABI corpus.
>
> Today, only one of these variables is represented as exported in the
> corpus.  Depending on the order in which the variables are analyzed,
> the variable that is represented as exported might be either one or
> the other, leading to some spurious self-comparison errors.
>
> This patch fixes the issue by allowing the corpus to represent more
> than one variable having a given ELF symbol to be exported.  This is
> similar to what is done already for functions.
>
> This fixes the self comparison of the qt6-qtbase package, which can be
> witnessed by issuing the command:
>
>      $ fedabipkgdiff --self-compare -a --from fc36 qt6-qtbase
>
> 	* include/abg-corpus.h (corpus::lookup_variables): Replace
> 	corpus::lookup_variable with this one which returns the set of
> 	variables associated with a given ID of variable.
> 	* src/abg-corpus-priv.h (istr_var_ptr_set_map_type): Define new
> 	typedef.
> 	(corpus::exported_decls_builder::priv::id_vars_map_): Replace
> 	id_var_map_ with this new data member of type
> 	istr_var_ptr_set_map_type.
> 	(corpus::exported_decls_builder::priv::id_fns_map): Fix comment.
> 	(corpus::exported_decls_builder::priv::id_vars_map): Replace
> 	previous id_var_map with this member function.
> 	(corpus::exported_decls_builder::priv::var_id_is_in_id_vars_map):
> 	Replace the var_id_is_in_id_var_map member function.
> 	(corpus::exported_decls_builder::priv::var_is_in_vars): Define new
> 	static member function.
> 	(corpus::exported_decls_builder::priv::{var_is_in_id_vars_map, add_var_to_id_vars_map}):
> 	Define new member functions.
> 	(corpus::exported_decls_builder::priv::add_var_to_map): Remove.
> 	(corpus::exported_decls_builder::priv::add_var_to_exported):
> 	Adjust by using the new var_is_in_id_vars_map and
> 	add_var_to_id_vars_map.
> 	* src/abg-corpus.cc
> 	(corpus::exported_decls_builder::maybe_add_var_to_exported_vars):
> 	Adjust, use the new
> 	corpus::exported_decls_builder::priv::var_is_in_id_vars_map.
> 	(corpus::lookup_variables): Replace corpus::lookup_variable with
> 	this one which returns the set of variables associated with a
> 	given ID of variable.
> 	* tools/abicompat.cc
> 	(compare_expected_against_provided_variables): Adjust.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
>   include/abg-corpus.h  |   7 +-
>   src/abg-corpus-priv.h | 171 ++++++++++++++++++++++++++++++++----------
>   src/abg-corpus.cc     |  34 ++++++---
>   tools/abicompat.cc    |  39 +++++-----
>   4 files changed, 182 insertions(+), 69 deletions(-)
>
> diff --git a/include/abg-corpus.h b/include/abg-corpus.h
> index 2cebad2e..644dc627 100644
> --- a/include/abg-corpus.h
> +++ b/include/abg-corpus.h
> @@ -230,8 +230,11 @@ public:
>     const std::unordered_set<function_decl*>*
>     lookup_functions(const char* id) const;
>   
> -  const var_decl_sptr
> -  lookup_variable(const interned_string& id) const;
> +  const std::unordered_set<var_decl_sptr>*
> +  lookup_variables(const interned_string& id) const;
> +
> +  const std::unordered_set<var_decl_sptr>*
> +  lookup_variables(const char* id) const;
>   
>     void
>     sort_functions();
> diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
> index 100eaee7..5d5a9230 100644
> --- a/src/abg-corpus-priv.h
> +++ b/src/abg-corpus-priv.h
> @@ -63,6 +63,12 @@ typedef unordered_map<interned_string,
>   		      var_decl_sptr,
>   		      hash_interned_string> istr_var_ptr_map_type;
>   
> +/// Convenience typedef for a hash map which key is an interned_string
> +/// and which data is a set of abigail::ir::var_decl_sptr
> +typedef unordered_map<interned_string,
> +		      std::unordered_set<var_decl_sptr>,
> +		      hash_interned_string> istr_var_ptr_set_map_type;
> +
>   /// The type of the private data of @ref
>   /// corpus::exported_decls_builder type.
>   class corpus::exported_decls_builder::priv
> @@ -75,7 +81,7 @@ class corpus::exported_decls_builder::priv
>     functions&		fns_;
>     variables&		vars_;
>     // A map that associates a function ID (function symbol and its
> -  // version) to to a vector of functions with that ID.  Normally, one
> +  // version) to a vector of functions with that ID.  Normally, one
>     // would think that in the corpus, there must only one function for
>     // a given ID.  Actually, in c++, there can be two function template
>     // instantiations that produce the same function ID because the
> @@ -83,7 +89,7 @@ class corpus::exported_decls_builder::priv
>     // of the first instantiation, for instance.  So there can be cases
>     // where one ID appertains to more than one function.
>     istr_fn_ptr_set_map_type	id_fns_map_;
> -  istr_var_ptr_map_type	id_var_map_;
> +  istr_var_ptr_set_map_type	id_vars_map_;
>     strings_type&	fns_suppress_regexps_;
>     regex_t_sptrs_type	compiled_fns_suppress_regexp_;
>     strings_type&	vars_suppress_regexps_;
> @@ -223,12 +229,11 @@ public:
>     /// Getter for a map of the IDs of the functions that are present in
>     /// the set of exported functions.
>     ///
> -  /// This map is useful during the construction of the set of
> -  /// exported functions, at least to ensure that every function is
> -  /// present only once in that set.
> +  /// The map associates the ID of a function with the set of
> +  /// functions having the symbol that matches the ID.
>     ///
> -  /// @return a map which key is a string and which data is a pointer
> -  /// to a function.
> +  /// @return a map which key is a string and which data is a set of
> +  /// functions.
>     istr_fn_ptr_set_map_type&
>     id_fns_map()
>     {return id_fns_map_;}
> @@ -236,28 +241,26 @@ public:
>     /// Getter for a map of the IDs of the variables that are present in
>     /// the set of exported variables.
>     ///
> -  /// This map is useful during the construction of the set of
> -  /// exported variables, at least to ensure that every function is
> -  /// present only once in that set.
> +  /// The map associates the ID of a variable with the set of variables
> +  /// having the symbol that matches the ID.
>     ///
> -  /// @return a map which key is a string and which data is a pointer
> -  /// to a function.
> -  const istr_var_ptr_map_type&
> -  id_var_map() const
> -  {return id_var_map_;}
> +  /// @return a map which key is a string and which data is a set of
> +  /// variables.
> +  istr_var_ptr_set_map_type&
> +  id_vars_map()
> +  {return id_vars_map_;}
>   
>     /// Getter for a map of the IDs of the variables that are present in
>     /// the set of exported variables.
>     ///
> -  /// This map is useful during the construction of the set of
> -  /// exported variables, at least to ensure that every function is
> -  /// present only once in that set.
> +  /// The map associates the ID of a variable with the set of variables
> +  /// having the symbol that matches the ID.
>     ///
> -  /// @return a map which key is a string and which data is a pointer
> -  /// to a function.
> -  istr_var_ptr_map_type&
> -  id_var_map()
> -  {return id_var_map_;}
> +  /// @return a map which key is a string and which data is a set of
> +  /// variables.
> +  const istr_var_ptr_set_map_type&
> +  id_vars_map() const
> +  {return id_vars_map_;}
>   
>     /// Returns an ID for a given function.
>     ///
> @@ -428,34 +431,123 @@ public:
>       while (sym && !sym->is_main_symbol());
>     }
>   
> -  /// Test if a given (ID of a) varialble is present in the variable
> +  /// Test if a given (ID of a) variable is present in the variable
>     /// map.  In other words, it tests if a given variable is present in
>     /// the set of exported variables.
>     ///
>     /// @param fn_id the ID of the variable to consider.
>     ///
> -  /// @return true iff the variable designated by @p fn_id is present
> -  /// in the set of exported variables.
> -  bool
> -  var_id_is_in_id_var_map(const interned_string& var_id) const
> +  /// @return a pointer to the set of variables that have the same ID
> +  /// as @p var_id.
> +  std::unordered_set<var_decl_sptr>*
> +  var_id_is_in_id_vars_map(const interned_string& var_id)
>     {
> -    const istr_var_ptr_map_type& m = id_var_map();
> +    istr_var_ptr_set_map_type& m = id_vars_map();
>       auto i = m.find(var_id);
> -    return i != m.end();
> +    if (i != m.end())
> +      return &i->second;
> +    return nullptr;
>     }
>   
> -  /// Add a given variable to the map of functions that are present in
> -  /// the set of exported functions.
> +  /// Test if a given (ID of a) variable is present in the variable
> +  /// map.  In other words, it tests if a given variable is present in
> +  /// the set of exported variables.
> +  ///
> +  /// @param fn_id the ID of the variable to consider.
> +  ///
> +  /// @return a pointer to the set of variables that have the same ID
> +  /// as @p var_id.
> +  const std::unordered_set<var_decl_sptr>*
> +  var_id_is_in_id_vars_map(const interned_string& var_id) const
> +  {
> +    return const_cast<corpus::exported_decls_builder::priv*>(this)->
> +      var_id_is_in_id_vars_map(var_id);
> +  }
> +
> +  /// Test if a given variable is present in a set of variables.
> +  ///
> +  /// The variable compares the ID and the qualified name of
> +  /// variables.
>     ///
> -  /// @param id the variable to add to the map.
> +  /// @param fn the variable to consider.
> +  ///
> +  /// @parm fns the set of variables to consider.
> +  static bool
> +  var_is_in_vars(const var_decl_sptr& var,
> +		 const std::unordered_set<var_decl_sptr>& vars)
> +  {
> +    if (vars.empty())
> +      return false;
> +
> +    if (vars.find(var) != vars.end())
> +      return true;
> +
> +    const string var_id = var->get_id();
> +    for (const auto& v: vars)
> +      if (v->get_id() == var_id
> +	  && v->get_qualified_name() == var->get_qualified_name())
> +	return true;
> +
> +    return false;
> +  }
> +
> +  /// Test if a given variable is present in a set of variables.
> +  ///
> +  /// The variable compares the ID and the qualified name of
> +  /// variables.
> +  ///
> +  /// @param fn the variable to consider.
> +  ///
> +  /// @parm fns the set of variables to consider.
> +  bool
> +  var_is_in_id_vars_map(const var_decl_sptr& var)
> +  {
> +    if (!var)
> +      return false;
> +
> +    interned_string var_id = var->get_id();
> +    const std::unordered_set<var_decl_sptr>* vars =
> +      var_id_is_in_id_vars_map(var_id);
> +    if (vars && var_is_in_vars(var, *vars))
> +      return true;
> +    return false;
> +  }
> +
> +  /// Add a given variable to the map of variables that are present in
> +  /// the set of exported variables.
> +  ///
> +  /// @param fn the variable to add to the map.
>     void
> -  add_var_to_map(const var_decl_sptr& var)
> +  add_var_to_id_vars_map(const var_decl_sptr& var)
>     {
> -    if (var)
> +    if (!var)
> +      return;
> +
> +    // First associate the var id to the variable.
> +    interned_string var_id = var->get_id();
> +    std::unordered_set<var_decl_sptr>* vars = var_id_is_in_id_vars_map(var_id);
> +    if (!vars)
> +      vars = &(id_vars_map()[var_id] = std::unordered_set<var_decl_sptr>());
> +    vars->insert(var);
> +
> +    // Now associate all aliases of th underlying symbol to the
> +    // variable too.
> +    elf_symbol_sptr sym = var->get_symbol();
> +    ABG_ASSERT(sym);
> +    string sym_id;
> +    do
>         {
> -	const interned_string& var_id = get_id(*var);
> -	id_var_map()[var_id] = var;
> +	sym_id = sym->get_id_string();
> +	if (sym_id == var_id)
> +	  goto loop;
> +	vars = var_id_is_in_id_vars_map(var_id);
> +	if (!vars)
> +	  vars = &(id_vars_map()[var_id] = std::unordered_set<var_decl_sptr>());
> +	vars->insert(var);
> +      loop:
> +	sym = sym->get_next_alias();
>         }
> +    while (sym && !sym->is_main_symbol());
>     }
>   
>     /// Add a function to the set of exported functions.
> @@ -477,11 +569,10 @@ public:
>     void
>     add_var_to_exported(const var_decl_sptr& var)
>     {
> -    const interned_string& id = get_id(*var);
> -    if (!var_id_is_in_id_var_map(id))
> +    if (!var_is_in_id_vars_map(var))
>         {
>   	vars_.push_back(var);
> -	add_var_to_map(var);
> +	add_var_to_id_vars_map(var);
>         }
>     }
>   
> diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc
> index 61c9b63d..d3102982 100644
> --- a/src/abg-corpus.cc
> +++ b/src/abg-corpus.cc
> @@ -198,7 +198,7 @@ corpus::exported_decls_builder::maybe_add_var_to_exported_vars(const var_decl_sp
>     const interned_string& var_id = priv_->get_id(*var);
>     ABG_ASSERT(!var_id.empty());
>   
> -  if (priv_->var_id_is_in_id_var_map(var_id))
> +  if (priv_->var_is_in_id_vars_map(var))
>       return false;
>   
>     if (priv_->keep_wrt_id_of_vars_to_keep(var)
> @@ -1409,20 +1409,36 @@ corpus::lookup_functions(const char* id) const
>     return lookup_functions(string_id);
>   }
>   
> -/// Lookup the exported variable which has a given variable ID.
> +/// Lookup the exported variables which all have a given variable ID.
>   ///
>   /// @param id the ID of the variable to look up.
>   ///
> -/// @return the variable with ID @p id that was found or nil if none
> -/// was found.
> -const var_decl_sptr
> -corpus::lookup_variable(const interned_string& id) const
> +/// @return a pointer to the set of variables with ID @p id, or
> +/// nullptr if no variable was found with that ID.
> +const std::unordered_set<var_decl_sptr>*
> +corpus::lookup_variables(const interned_string& id) const
>   {
>     exported_decls_builder_sptr b = get_exported_decls_builder();
> -  auto i = b->priv_->id_var_map_.find(id);
> -  if (i == b->priv_->id_var_map_.end())
> +  auto i = b->priv_->id_vars_map_.find(id);
> +  if (i == b->priv_->id_vars_map_.end())
>       return nullptr;
> -  return i->second;
> +  return &i->second;
> +}
> +
> +/// Lookup the exported variables which all have a given variable ID.
> +///
> +/// @param id the ID of the variable to look up.
> +///
> +/// @return a pointer to the set of variables with ID @p id, or
> +/// nullptr if no variable was found with that ID.
> +const std::unordered_set<var_decl_sptr>*
> +corpus::lookup_variables(const char* id) const
> +{
> +  if (!id)
> +    return nullptr;
> +
> +  interned_string string_id = priv_->env.intern(id);
> +  return lookup_variables(string_id);
>   }
>   
>   /// Sort the set of functions exported by this corpus.
> diff --git a/tools/abicompat.cc b/tools/abicompat.cc
> index 05ac2bdb..b14185dd 100644
> --- a/tools/abicompat.cc
> +++ b/tools/abicompat.cc
> @@ -542,27 +542,30 @@ compare_expected_against_provided_variables(diff_context_sptr&		ctxt,
>       {
>         interned_string var_id = expected_var->get_id();
>         // ... against the variables exported by the library!
> -      const var_decl_sptr exported_var =
> +      const std::unordered_set<var_decl_sptr>* exported_vars =
>   	reverse_direction
> -	? app_corpus->lookup_variable(var_id)
> -	: lib_corpus->lookup_variable(var_id);
> -      if (exported_var)
> +	? app_corpus->lookup_variables(var_id)
> +	: lib_corpus->lookup_variables(var_id);
> +      if (exported_vars)
>   	{
> -	  // OK here is where we compare the variable expected by
> -	  // the application against the variable exported by the
> -	  // library.
> -	  diff_sptr type_diff =
> -	    compute_diff(expected_var->get_type(),
> -			 exported_var->get_type(),
> -			 ctxt);
> -	  if (type_diff && type_diff->to_be_reported())
> +	  for (auto exported_var : *exported_vars)
>   	    {
> -	      // So there is a type change between the variable
> -	      // expected by the application and the variable
> -	      // exported by the library.  Let's record that
> -	      // change so that we can report about it later.
> -	      var_changes.push_back(var_change(expected_var, type_diff, reverse_direction));
> -	      status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
> +	      // OK here is where we compare the variable expected by
> +	      // the application against the variable exported by the
> +	      // library.
> +	      diff_sptr type_diff =
> +		compute_diff(expected_var->get_type(),
> +			     exported_var->get_type(),
> +			     ctxt);
> +	      if (type_diff && type_diff->to_be_reported())
> +		{
> +		  // So there is a type change between the variable
> +		  // expected by the application and the variable
> +		  // exported by the library.  Let's record that
> +		  // change so that we can report about it later.
> +		  var_changes.push_back(var_change(expected_var, type_diff, reverse_direction));
> +		  status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
> +		}
>   	    }
>   	}
>       }
  
Dodji Seketeli Sept. 6, 2024, 11:18 p.m. UTC | #2
Hello Ben,

Ben Woodard <woodard@redhat.com> a écrit:

[...]

> On 9/6/24 1:56 PM, Dodji Seketeli wrote:
>> Hello,
>>
>> The libQt6Network.so.6.3.1 exhibits some interesting debug info where
>> two variables with slightly different qualified names have the same ELF
>> symbol.
>>
>> Consider the DIE for this variable:
>>   [ 1aab3]    variable             abbrev: 118
>>               name                 (GNU_strp_alt) "backendMutex"
>>               decl_file            (implicit_const) qsslsocket_p.h (42)
>>               decl_line            (data1) 196
>>               decl_column          (data1) 26
>>               linkage_name         (GNU_strp_alt) "_ZN17QSslSocketPrivate12backendMutexE"
>>               type                 (ref_udata) [ 1aa35]
>>               external             (flag_present) yes
>>               declaration          (flag_present) yes
>>               inline               (implicit_const) declared_inlined (3)
>>               location             (exprloc)
>>                [ 0] addr +0x1e3050 <_ZN17QSslSocketPrivate12backendMutexE>
>>
>> This DIE designates a variable named backendMutex in the global
>> namespace.  It's associated to the ELF symbol
>> _ZN17QSslSocketPrivate12backendMutexE.
>>
>> Then, later, there is this DIE:
>>
>>   [d05d3e]    class_type           abbrev: 260
>>               name                 (GNU_strp_alt) "QSslSocketPrivate"
>>               declaration          (flag_present) yes
>>               sibling              (ref_udata) [d05d70]
>> [...]
>>   [d05d70]    variable             abbrev: 642
>>               name                 (GNU_strp_alt) "backendMutex"
>>               decl_file            (implicit_const) qsslsocket_p.h (54)
>>               decl_line            (data1) 196
>>               decl_column          (data1) 26
>>               linkage_name         (GNU_strp_alt) "_ZN17QSslSocketPrivate12backendMutexE"
>>               type                 (ref_addr) [  fdc5]
>>               external             (flag_present) yes
>>               declaration          (flag_present) yes
>>               inline               (implicit_const) declared_inlined (3)
>>               location             (exprloc)
>>                [ 0] addr +0x1e3050 <_ZN17QSslSocketPrivate12backendMutexE>
>>
>> Here, this DIE represents a static data member named
>> QSslSocketPrivate::backendMutex, as it's member of the
>> QSslSocketPrivate class.  Note how it's also associated with the ELF
>> symbol _ZN17QSslSocketPrivate12backendMutexE.
>
> How does that work? How can they have the same C linkage name and work
> properly? What am I missing here?

Usually the way the declaration/definition of static data members is
done is that the definition DIE is a "concrete" instance of the
declaration, which is in the global namespace/scope.  And it's only that
definition DIE that carries the DW_AT_location attribute that points to
the ELF symbol.  Then that concrete instance DIE has a
DW_AT_abstract_origin attribute that points to the DIE that represents
the abstract instance which is the declaration DIE.  That declaration
DIE would then be a data member DIE of the QSslSocketPrivate class DIE.

Here, you see that the DW_AT_abstract_origin attribute is missing.  I
think it's a bug.  So both DIEs look like if they were for separate,
unrelated variables.  But because they points to the same ELF symbol,
libabigail later detects that they are the "same thing", even though
they look like different variables.  The purpose of this patch is
precisely to allow the different variables to be represented, so that
they can be recognized as the same "thing" later.

Another thing that could have been represented is if both variables were
pointing to two ELF symbols E and E', where E' is an alias of E.  That
is what is called in GCC land, a cloned variable (or function).  Cloned
functions are used by GCC to implement the various flavors of
destructors (D0, D1 and D2 destructors:
https://stackoverflow.com/questions/6613870/gnu-gcc-g-why-does-it-generate-multiple-dtors/6614369#6614369).

But, again, in that case, a DW_AT_abstract_origin attribute should be
used on the cloned (concreted) entity to point to the (abstract) entity
it was cloned from.

So yeah, this patch is one of those work-around for these DWARF bugs (or
difficulties).  There are a number of binaries around with these issues
so we need to have heuristics to handle them as much as possible.

Oh, and this particular issue seems to have been fixed in subsequent
versions of GCC, unless I messed-up something in my testing.

I hope this helps.

[...]

Cheers,
  

Patch

diff --git a/include/abg-corpus.h b/include/abg-corpus.h
index 2cebad2e..644dc627 100644
--- a/include/abg-corpus.h
+++ b/include/abg-corpus.h
@@ -230,8 +230,11 @@  public:
   const std::unordered_set<function_decl*>*
   lookup_functions(const char* id) const;
 
-  const var_decl_sptr
-  lookup_variable(const interned_string& id) const;
+  const std::unordered_set<var_decl_sptr>*
+  lookup_variables(const interned_string& id) const;
+
+  const std::unordered_set<var_decl_sptr>*
+  lookup_variables(const char* id) const;
 
   void
   sort_functions();
diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
index 100eaee7..5d5a9230 100644
--- a/src/abg-corpus-priv.h
+++ b/src/abg-corpus-priv.h
@@ -63,6 +63,12 @@  typedef unordered_map<interned_string,
 		      var_decl_sptr,
 		      hash_interned_string> istr_var_ptr_map_type;
 
+/// Convenience typedef for a hash map which key is an interned_string
+/// and which data is a set of abigail::ir::var_decl_sptr
+typedef unordered_map<interned_string,
+		      std::unordered_set<var_decl_sptr>,
+		      hash_interned_string> istr_var_ptr_set_map_type;
+
 /// The type of the private data of @ref
 /// corpus::exported_decls_builder type.
 class corpus::exported_decls_builder::priv
@@ -75,7 +81,7 @@  class corpus::exported_decls_builder::priv
   functions&		fns_;
   variables&		vars_;
   // A map that associates a function ID (function symbol and its
-  // version) to to a vector of functions with that ID.  Normally, one
+  // version) to a vector of functions with that ID.  Normally, one
   // would think that in the corpus, there must only one function for
   // a given ID.  Actually, in c++, there can be two function template
   // instantiations that produce the same function ID because the
@@ -83,7 +89,7 @@  class corpus::exported_decls_builder::priv
   // of the first instantiation, for instance.  So there can be cases
   // where one ID appertains to more than one function.
   istr_fn_ptr_set_map_type	id_fns_map_;
-  istr_var_ptr_map_type	id_var_map_;
+  istr_var_ptr_set_map_type	id_vars_map_;
   strings_type&	fns_suppress_regexps_;
   regex_t_sptrs_type	compiled_fns_suppress_regexp_;
   strings_type&	vars_suppress_regexps_;
@@ -223,12 +229,11 @@  public:
   /// Getter for a map of the IDs of the functions that are present in
   /// the set of exported functions.
   ///
-  /// This map is useful during the construction of the set of
-  /// exported functions, at least to ensure that every function is
-  /// present only once in that set.
+  /// The map associates the ID of a function with the set of
+  /// functions having the symbol that matches the ID.
   ///
-  /// @return a map which key is a string and which data is a pointer
-  /// to a function.
+  /// @return a map which key is a string and which data is a set of
+  /// functions.
   istr_fn_ptr_set_map_type&
   id_fns_map()
   {return id_fns_map_;}
@@ -236,28 +241,26 @@  public:
   /// Getter for a map of the IDs of the variables that are present in
   /// the set of exported variables.
   ///
-  /// This map is useful during the construction of the set of
-  /// exported variables, at least to ensure that every function is
-  /// present only once in that set.
+  /// The map associates the ID of a variable with the set of variables
+  /// having the symbol that matches the ID.
   ///
-  /// @return a map which key is a string and which data is a pointer
-  /// to a function.
-  const istr_var_ptr_map_type&
-  id_var_map() const
-  {return id_var_map_;}
+  /// @return a map which key is a string and which data is a set of
+  /// variables.
+  istr_var_ptr_set_map_type&
+  id_vars_map()
+  {return id_vars_map_;}
 
   /// Getter for a map of the IDs of the variables that are present in
   /// the set of exported variables.
   ///
-  /// This map is useful during the construction of the set of
-  /// exported variables, at least to ensure that every function is
-  /// present only once in that set.
+  /// The map associates the ID of a variable with the set of variables
+  /// having the symbol that matches the ID.
   ///
-  /// @return a map which key is a string and which data is a pointer
-  /// to a function.
-  istr_var_ptr_map_type&
-  id_var_map()
-  {return id_var_map_;}
+  /// @return a map which key is a string and which data is a set of
+  /// variables.
+  const istr_var_ptr_set_map_type&
+  id_vars_map() const
+  {return id_vars_map_;}
 
   /// Returns an ID for a given function.
   ///
@@ -428,34 +431,123 @@  public:
     while (sym && !sym->is_main_symbol());
   }
 
-  /// Test if a given (ID of a) varialble is present in the variable
+  /// Test if a given (ID of a) variable is present in the variable
   /// map.  In other words, it tests if a given variable is present in
   /// the set of exported variables.
   ///
   /// @param fn_id the ID of the variable to consider.
   ///
-  /// @return true iff the variable designated by @p fn_id is present
-  /// in the set of exported variables.
-  bool
-  var_id_is_in_id_var_map(const interned_string& var_id) const
+  /// @return a pointer to the set of variables that have the same ID
+  /// as @p var_id.
+  std::unordered_set<var_decl_sptr>*
+  var_id_is_in_id_vars_map(const interned_string& var_id)
   {
-    const istr_var_ptr_map_type& m = id_var_map();
+    istr_var_ptr_set_map_type& m = id_vars_map();
     auto i = m.find(var_id);
-    return i != m.end();
+    if (i != m.end())
+      return &i->second;
+    return nullptr;
   }
 
-  /// Add a given variable to the map of functions that are present in
-  /// the set of exported functions.
+  /// Test if a given (ID of a) variable is present in the variable
+  /// map.  In other words, it tests if a given variable is present in
+  /// the set of exported variables.
+  ///
+  /// @param fn_id the ID of the variable to consider.
+  ///
+  /// @return a pointer to the set of variables that have the same ID
+  /// as @p var_id.
+  const std::unordered_set<var_decl_sptr>*
+  var_id_is_in_id_vars_map(const interned_string& var_id) const
+  {
+    return const_cast<corpus::exported_decls_builder::priv*>(this)->
+      var_id_is_in_id_vars_map(var_id);
+  }
+
+  /// Test if a given variable is present in a set of variables.
+  ///
+  /// The variable compares the ID and the qualified name of
+  /// variables.
   ///
-  /// @param id the variable to add to the map.
+  /// @param fn the variable to consider.
+  ///
+  /// @parm fns the set of variables to consider.
+  static bool
+  var_is_in_vars(const var_decl_sptr& var,
+		 const std::unordered_set<var_decl_sptr>& vars)
+  {
+    if (vars.empty())
+      return false;
+
+    if (vars.find(var) != vars.end())
+      return true;
+
+    const string var_id = var->get_id();
+    for (const auto& v: vars)
+      if (v->get_id() == var_id
+	  && v->get_qualified_name() == var->get_qualified_name())
+	return true;
+
+    return false;
+  }
+
+  /// Test if a given variable is present in a set of variables.
+  ///
+  /// The variable compares the ID and the qualified name of
+  /// variables.
+  ///
+  /// @param fn the variable to consider.
+  ///
+  /// @parm fns the set of variables to consider.
+  bool
+  var_is_in_id_vars_map(const var_decl_sptr& var)
+  {
+    if (!var)
+      return false;
+
+    interned_string var_id = var->get_id();
+    const std::unordered_set<var_decl_sptr>* vars =
+      var_id_is_in_id_vars_map(var_id);
+    if (vars && var_is_in_vars(var, *vars))
+      return true;
+    return false;
+  }
+
+  /// Add a given variable to the map of variables that are present in
+  /// the set of exported variables.
+  ///
+  /// @param fn the variable to add to the map.
   void
-  add_var_to_map(const var_decl_sptr& var)
+  add_var_to_id_vars_map(const var_decl_sptr& var)
   {
-    if (var)
+    if (!var)
+      return;
+
+    // First associate the var id to the variable.
+    interned_string var_id = var->get_id();
+    std::unordered_set<var_decl_sptr>* vars = var_id_is_in_id_vars_map(var_id);
+    if (!vars)
+      vars = &(id_vars_map()[var_id] = std::unordered_set<var_decl_sptr>());
+    vars->insert(var);
+
+    // Now associate all aliases of th underlying symbol to the
+    // variable too.
+    elf_symbol_sptr sym = var->get_symbol();
+    ABG_ASSERT(sym);
+    string sym_id;
+    do
       {
-	const interned_string& var_id = get_id(*var);
-	id_var_map()[var_id] = var;
+	sym_id = sym->get_id_string();
+	if (sym_id == var_id)
+	  goto loop;
+	vars = var_id_is_in_id_vars_map(var_id);
+	if (!vars)
+	  vars = &(id_vars_map()[var_id] = std::unordered_set<var_decl_sptr>());
+	vars->insert(var);
+      loop:
+	sym = sym->get_next_alias();
       }
+    while (sym && !sym->is_main_symbol());
   }
 
   /// Add a function to the set of exported functions.
@@ -477,11 +569,10 @@  public:
   void
   add_var_to_exported(const var_decl_sptr& var)
   {
-    const interned_string& id = get_id(*var);
-    if (!var_id_is_in_id_var_map(id))
+    if (!var_is_in_id_vars_map(var))
       {
 	vars_.push_back(var);
-	add_var_to_map(var);
+	add_var_to_id_vars_map(var);
       }
   }
 
diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc
index 61c9b63d..d3102982 100644
--- a/src/abg-corpus.cc
+++ b/src/abg-corpus.cc
@@ -198,7 +198,7 @@  corpus::exported_decls_builder::maybe_add_var_to_exported_vars(const var_decl_sp
   const interned_string& var_id = priv_->get_id(*var);
   ABG_ASSERT(!var_id.empty());
 
-  if (priv_->var_id_is_in_id_var_map(var_id))
+  if (priv_->var_is_in_id_vars_map(var))
     return false;
 
   if (priv_->keep_wrt_id_of_vars_to_keep(var)
@@ -1409,20 +1409,36 @@  corpus::lookup_functions(const char* id) const
   return lookup_functions(string_id);
 }
 
-/// Lookup the exported variable which has a given variable ID.
+/// Lookup the exported variables which all have a given variable ID.
 ///
 /// @param id the ID of the variable to look up.
 ///
-/// @return the variable with ID @p id that was found or nil if none
-/// was found.
-const var_decl_sptr
-corpus::lookup_variable(const interned_string& id) const
+/// @return a pointer to the set of variables with ID @p id, or
+/// nullptr if no variable was found with that ID.
+const std::unordered_set<var_decl_sptr>*
+corpus::lookup_variables(const interned_string& id) const
 {
   exported_decls_builder_sptr b = get_exported_decls_builder();
-  auto i = b->priv_->id_var_map_.find(id);
-  if (i == b->priv_->id_var_map_.end())
+  auto i = b->priv_->id_vars_map_.find(id);
+  if (i == b->priv_->id_vars_map_.end())
     return nullptr;
-  return i->second;
+  return &i->second;
+}
+
+/// Lookup the exported variables which all have a given variable ID.
+///
+/// @param id the ID of the variable to look up.
+///
+/// @return a pointer to the set of variables with ID @p id, or
+/// nullptr if no variable was found with that ID.
+const std::unordered_set<var_decl_sptr>*
+corpus::lookup_variables(const char* id) const
+{
+  if (!id)
+    return nullptr;
+
+  interned_string string_id = priv_->env.intern(id);
+  return lookup_variables(string_id);
 }
 
 /// Sort the set of functions exported by this corpus.
diff --git a/tools/abicompat.cc b/tools/abicompat.cc
index 05ac2bdb..b14185dd 100644
--- a/tools/abicompat.cc
+++ b/tools/abicompat.cc
@@ -542,27 +542,30 @@  compare_expected_against_provided_variables(diff_context_sptr&		ctxt,
     {
       interned_string var_id = expected_var->get_id();
       // ... against the variables exported by the library!
-      const var_decl_sptr exported_var =
+      const std::unordered_set<var_decl_sptr>* exported_vars =
 	reverse_direction
-	? app_corpus->lookup_variable(var_id)
-	: lib_corpus->lookup_variable(var_id);
-      if (exported_var)
+	? app_corpus->lookup_variables(var_id)
+	: lib_corpus->lookup_variables(var_id);
+      if (exported_vars)
 	{
-	  // OK here is where we compare the variable expected by
-	  // the application against the variable exported by the
-	  // library.
-	  diff_sptr type_diff =
-	    compute_diff(expected_var->get_type(),
-			 exported_var->get_type(),
-			 ctxt);
-	  if (type_diff && type_diff->to_be_reported())
+	  for (auto exported_var : *exported_vars)
 	    {
-	      // So there is a type change between the variable
-	      // expected by the application and the variable
-	      // exported by the library.  Let's record that
-	      // change so that we can report about it later.
-	      var_changes.push_back(var_change(expected_var, type_diff, reverse_direction));
-	      status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
+	      // OK here is where we compare the variable expected by
+	      // the application against the variable exported by the
+	      // library.
+	      diff_sptr type_diff =
+		compute_diff(expected_var->get_type(),
+			     exported_var->get_type(),
+			     ctxt);
+	      if (type_diff && type_diff->to_be_reported())
+		{
+		  // So there is a type change between the variable
+		  // expected by the application and the variable
+		  // exported by the library.  Let's record that
+		  // change so that we can report about it later.
+		  var_changes.push_back(var_change(expected_var, type_diff, reverse_direction));
+		  status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
+		}
 	    }
 	}
     }