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

[Dodji Seketeli <dodji@seketeli.org>]

Hello Mark & Giuliano and Matthias!

Mark Wielaard <mark@klomp.org> a écrit:

[...]

> 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.

Thank you for the review!  I have updated the patch accordingly and I am
posting an updated version below.

[...]

>> 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?

Yes it is.  Fixed.

>> 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.

Hmmh, that one is 70 columns wide, so less than 80, as far as I can
tell.

[...]

>> @@ -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.

Thanks for commenting on this.  We'll see how Giuliano thinks.

[...]

>
>> +  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.

Absolutely.  Fixed.  Thanks.

[...]

> Looks good.

Thanks!

[...]

Giuliano Procida <gprocida@google.com> a écrit:

> Hi.

Hey Giuliano,

> I have some minor comments. Other than that, it looks good to me.
>
> I'm CCing Matthias as he did a lot of the reworking into the current
> structure, I think.

Thank you for the review.  I have updated the patch according to your
comments.  Let's look at them once again.

[...]

>> > @@ -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
>
> and and / and

Spot on.  Thanks.  Fixed.

[...]

>> > +/// only once during the execution of the former.
>
> I've lost a bit of the context, but where comments say "only once"
> should that be "only once per symbol"?

You are right.  Fixed.

[...]

>> >  /// @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.
>>
>
> The two ARM adjustments belong together. I wasn't sure about PPC, so
> I'm glad that it's getting moved to the right place.

Great, thanks to both of you.

[...]

>
>> > +  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)
>
> IIRC, someone said that this should apply to PPC32 as well, perhaps you, Mark?

[...]

Mark Wielaard <mark@klomp.org> a écrit:

> Yes, in theory both ppc32 and ppc64 use function descriptors (and both
> are big endian). But I haven't seen a real ppc32 setup in years.
>
> If you want to support that then it should be as simple as defining a
> architecture_is_ppc32 which is precisely like architecture_is_ppc64
> with s/EM_PPC64/EM_PPC/

OK, thanks Giuliano and Mark.

I have added support for ppc32 as well, as outlined above.

Please find below the updated patch that I am proposing.

Thanks!

From 8d44b582529ccd02689043918ef3ec3dd85155bc 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.

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.

This fixes runtestslowselfcompare.sh on ppc64 (ELFv1) with
ENABLE_SLOW_TEST=yes

	* src/abg-elf-helpers.h (architecture_is_ppc32): Declare new
	function.
	* src/abg-elf-helpers.cc (architecture_is_ppc32): Define it.
	* 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 ppc{32,64} 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-elf-helpers.cc   | 13 +++++++++
 src/abg-elf-helpers.h    |  3 ++
 src/abg-symtab-reader.cc | 62 ++++++++++++++++++++++------------------
 src/abg-symtab-reader.h  |  6 ++--
 4 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/src/abg-elf-helpers.cc b/src/abg-elf-helpers.cc
index c41e339e..2a2cc6c2 100644
--- a/src/abg-elf-helpers.cc
+++ b/src/abg-elf-helpers.cc
@@ -918,6 +918,19 @@ architecture_is_ppc64(Elf* elf_handle)
   return (elf_header && elf_header->e_machine == EM_PPC64);
 }
 
+/// Test if the architecture of the current binary is ppc32.
+///
+/// @param elf_handle the ELF handle to consider.
+///
+/// @return true iff the architecture of the current binary is ppc32.
+bool
+architecture_is_ppc32(Elf* elf_handle)
+{
+  GElf_Ehdr  eh_mem;
+  GElf_Ehdr* elf_header = gelf_getehdr(elf_handle, &eh_mem);
+  return (elf_header && elf_header->e_machine == EM_PPC);
+}
+
 /// Test if the architecture of the current binary is arm32.
 ///
 /// @param elf_handle the ELF handle to consider.
diff --git a/src/abg-elf-helpers.h b/src/abg-elf-helpers.h
index ee406863..0dc987d4 100644
--- a/src/abg-elf-helpers.h
+++ b/src/abg-elf-helpers.h
@@ -147,6 +147,9 @@ get_version_for_symbol(Elf*			elf_handle,
 bool
 architecture_is_ppc64(Elf* elf_handle);
 
+bool
+architecture_is_ppc32(Elf* elf_handle);
+
 bool
 architecture_is_arm32(Elf* elf_handle);
 
diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
index 3740cb7a..0c2f8ccc 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 must be called only
+/// once (per symbol) during the execution of the former.
+///
 /// @param elf_handle the ELF handle
 ///
 /// @param elf_symbol the ELF symbol
@@ -476,35 +470,49 @@ 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);
   const bool is_ppc64 = elf_helpers::architecture_is_ppc64(elf_handle);
+  const bool is_ppc32 = elf_helpers::architecture_is_ppc32(elf_handle);
 
   GElf_Addr symbol_value =
     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 (is_arm32 && symbol_sptr->is_function())
+    // 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);
 
+  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);
+    }
+
+  // Please note that update_function_entry_address_symbol_map depends
+  // on the symbol aliases been setup.  This is why, the
+  // elf_symbol::add_alias call is done above BEFORE this point.
+  if ((is_ppc64 || is_ppc32) && symbol_sptr->is_function())
+    update_function_entry_address_symbol_map(elf_handle, elf_symbol,
+					     symbol_sptr);
+
   return symbol_value;
 }
 
@@ -656,9 +664,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,
-- 
2.36.1

Cheers,

-- 
		Dodji