Re: [PATCH] symtab-reader: Setup aliases before checking ppc64 opd function entries

[Mark Wielaard <mark@klomp.org>]

Hi Dodji,

On Mon, 2022-05-16 at 11:43 +0200, Dodji Seketeli wrote:
> I have thus adjusted your patch to keep its spirit while making it
> apply to the current code base.
> 
> Giuliano, Mark, would you guys please review the patch, when you have
> time, and tell me what you think?

It looks good to me, some small comments below. I don't have access to
a ppc64[be] setup anymore, but the buildbot has:
https://builder.sourceware.org/buildbot/#/builders/libabigail-debian-ppc64
So hopefully after this patch that will turn green.

> From 768d13cb21a0d6c8ebd8ed0530a095a036c4c9b5 Mon Sep 17 00:00:00 2001
> From: Mark Wielaard <mark@klomp.org>
> Date: Mon, 16 May 2022 10:53:55 +0200
> Subject: [PATCH] symtab-reader: Setup aliases before checking ppc64 opd function entries
> 
> The update_function_entry_address_symbol_map function checks whether
> the given symbol is an alias of another symbol or a special ppc64
> ELFv1 function entry symbol. This requires the symbol aliases to
> already been setup. But the alias entries were only setup after
> calling update_function_entry_address_symbol_map. Make sure that
> the symbol aliases have been setup and only then call the special
> ppc64 update_function_entry_address_symbol_map function. But make
> sure the arm32 function symbol entry address cleanup is done before
> checking for aliases.
> 
> This patch renames symtab::get_symbol_value into
> symtab::setup_symbol_lookup_tables to better reflect what it does.
> 
> It is in that function that the call to
> update_function_entry_address_symbol_map is performed, the arm32 symbol address
> cleanup is done and the ppc64 ELFv1 function address plumbing
> is done.

Is that second line too long?

> So arranging for update_function_entry_address_symbol_map to be called
> after symbol aliases are setup for ppc64 is also done in that
> function.  That way, symtab::load_ only have to call
> symtab::setup_symbol_lookup_tables to have aliases setup.

Same for the first line here.

> This fixes runtestslowselfcompare.sh on ppc64 (ELFv1) with
> ENABLE_SLOW_TEST=yes
> 
> 	* src/abg-symtab-reader.cc
> 	(symtab::setup_symbol_lookup_tables): Rename
> 	symtab::get_symbol_value into this. Setup symbol aliases before
> 	setting up function entry address maps for ppc64 ELFv1 and after
> 	fixing up arm32/64 addresses.
> 	(symtab::load_): Invoke the new setup_symbol_lookup_tables.
> 	update_function_entry_address_symbol_map after setting up aliases.
> 	No need to setup symbol aliases anymore.
> 	(symtab::add_alternative_address_lookups): Invoke the new
> 	setup_symbol_lookup_tables.
> 
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
>  src/abg-symtab-reader.cc | 58 +++++++++++++++++++++-------------------
>  src/abg-symtab-reader.h  |  6 ++---
>  2 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
> index 3740cb7a..05be4d6b 100644
> --- a/src/abg-symtab-reader.cc
> +++ b/src/abg-symtab-reader.cc
> @@ -343,17 +343,7 @@ symtab::load_(Elf*	       elf_handle,
>  	    }
>  	}
>        else if (symbol_sptr->is_defined())
> -	{
> -	  const GElf_Addr symbol_value =
> -	    get_symbol_value(elf_handle, sym, symbol_sptr);
> -	  const auto result =
> -	    addr_symbol_map_.emplace(symbol_value, symbol_sptr);
> -	  if (!result.second)
> -	    // A symbol with the same address already exists.  This
> -	    // means this symbol is an alias of the main symbol with
> -	    // that address.  So let's register this new alias as such.
> -	    result.first->second->get_main_symbol()->add_alias(symbol_sptr);
> -	}
> +	setup_symbol_lookup_tables(elf_handle, sym, symbol_sptr);
>      }
>  
>    add_alternative_address_lookups(elf_handle);
> @@ -468,6 +458,10 @@ symtab::update_main_symbol(GElf_Addr addr, const std::string& name)
>  /// Various adjustments and bookkeeping may be needed to provide a correct
>  /// interpretation (one that matches DWARF addresses) of raw symbol values.
>  ///
> +/// This is a sub-routine for symtab::load_and
> +/// symtab::add_alternative_address_lookups and and must be called
> +/// only once during the execution of the former.
> +///
>  /// @param elf_handle the ELF handle
>  ///
>  /// @param elf_symbol the ELF symbol
> @@ -476,9 +470,9 @@ symtab::update_main_symbol(GElf_Addr addr, const std::string& name)
>  ///
>  /// @return a possibly-adjusted symbol value
>  GElf_Addr
> -symtab::get_symbol_value(Elf* elf_handle,
> -			 GElf_Sym* elf_symbol,
> -			 const elf_symbol_sptr& symbol_sptr)
> +symtab::setup_symbol_lookup_tables(Elf* elf_handle,
> +				   GElf_Sym* elf_symbol,
> +				   const elf_symbol_sptr& symbol_sptr)
>  {
>    const bool is_arm32 = elf_helpers::architecture_is_arm32(elf_handle);
>    const bool is_arm64 = elf_helpers::architecture_is_arm64(elf_handle);
> @@ -488,23 +482,33 @@ symtab::get_symbol_value(Elf* elf_handle,
>      elf_helpers::maybe_adjust_et_rel_sym_addr_to_abs_addr(elf_handle,
>  							  elf_symbol);
>  
> -  if (symbol_sptr->is_function())
> -    {
> -      if (is_arm32)
> -	// Clear bit zero of ARM32 addresses as per "ELF for the Arm
> -	// Architecture" section 5.5.3.
> -	// https://static.docs.arm.com/ihi0044/g/aaelf32.pdf
> -	symbol_value &= ~1;
> -      else if (is_ppc64)
> -	update_function_entry_address_symbol_map(elf_handle, elf_symbol,
> -						 symbol_sptr);
> -    }
> +  if (symbol_sptr->is_function() && is_arm32)
> +    // Clear bit zero of ARM32 addresses as per "ELF for the Arm
> +    // Architecture" section 5.5.3.
> +    // https://static.docs.arm.com/ihi0044/g/aaelf32.pdf
> +    symbol_value &= ~1;
> +
>    if (is_arm64)
>      // Copy bit 55 over bits 56 to 63 which may be tag information.
>      symbol_value = symbol_value & (1ULL<<55)
>  		   ? symbol_value | (0xffULL<<56)
>  		   : symbol_value &~ (0xffULL<<56);

This part is new. I think it is correct to do this, before setting up
the aliases, just like we mask off the zero bit for arm32. But it would
be good if Giuliano could confirm.

> +  if (symbol_sptr->is_defined())
> +    {
> +      const auto result =
> +	addr_symbol_map_.emplace(symbol_value, symbol_sptr);
> +      if (!result.second)
> +	// A symbol with the same address already exists.  This
> +	// means this symbol is an alias of the main symbol with
> +	// that address.  So let's register this new alias as such.
> +	result.first->second->get_main_symbol()->add_alias(symbol_sptr);
> +    }
> +
> +  if (symbol_sptr->is_function() && is_ppc64)
> +    update_function_entry_address_symbol_map(elf_handle, elf_symbol,
> +					     symbol_sptr);

I would add a comment here to say something like:

// Note, update_function_entry_address_symbol_map depends
// on the symbol aliases been setup.

So it is clear why this needs to be done after the add_alias call
above.

>    return symbol_value;
>  }
>  
> @@ -656,9 +660,7 @@ symtab::add_alternative_address_lookups(Elf* elf_handle)
>  	  if (symbols.size() == 1)
>  	    {
>  	      const auto& symbol_sptr = symbols[0];
> -	      const GElf_Addr symbol_value =
> -		get_symbol_value(elf_handle, sym, symbol_sptr);
> -	      addr_symbol_map_.emplace(symbol_value, symbol_sptr);
> +		setup_symbol_lookup_tables(elf_handle, sym, symbol_sptr);
>  	    }
>  	}
>      }
> diff --git a/src/abg-symtab-reader.h b/src/abg-symtab-reader.h
> index bddde2f6..948fc270 100644
> --- a/src/abg-symtab-reader.h
> +++ b/src/abg-symtab-reader.h
> @@ -290,9 +290,9 @@ private:
>         string_elf_symbols_map_sptr variables_symbol_map);
>  
>    GElf_Addr
> -  get_symbol_value(Elf* elf_handle,
> -		   GElf_Sym* elf_symbol,
> -		   const elf_symbol_sptr& symbol_sptr);
> +  setup_symbol_lookup_tables(Elf* elf_handle,
> +			     GElf_Sym* elf_symbol,
> +			     const elf_symbol_sptr& symbol_sptr);
>  
>    void
>    update_function_entry_address_symbol_map(Elf*	     elf_handle,
> -- 

Looks good.

Thanks,

Mark