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

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

[Dodji Seketeli]

Mark Wielaard mark@klomp.org 
Date: Mon, 16 May 2022 11:43:35 +0200
Message-ID: <87a6bhvmm0.fsf@seketeli.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain

[This is email is in reply to the patch sent by Mark at https://sourceware.org/pipermail/libabigail/2022q2/004318.html]

Hello Mark, Giuliano,

For a reason, my email account was bouncing emails from sourceware.org
around the May 1st, so mailman stopped sending me emails around that
time.  I re-enabled the service, but it seems I haven't received a
number of emails around that time.

So I am replying to the patch that Mark sent at
https://sourceware.org/pipermail/libabigail/2022q2/004318.html.

Mark, I applied two patches from Giuliano that changed the area that your
patch touches.  Those patches are:

    c96463e1 symtab: fix up 64-bit ARM address which may contain tags
    5106149c symtab: refactor ELF symbol value tweaks

Basically, those patches create a new symtab::get_symbol_value and
performs the arm{32,64} and ppc64 ELFv1 addresses adjustments.

So, your patch won't apply anymore.

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?

Thank you very much.

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.

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-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);
 
+  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);
+
   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,
-- 
2.35.0.rc2

Cheers,

-- 
		Dodji

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

[Mark Wielaard]

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

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

[Giuliano Procida]

Hi.

On Tue, 17 May 2022 at 10:59, Mark Wielaard <mark@klomp.org> wrote:
>
> 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.
>

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.

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

and and / and

> > +/// 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"?

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

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.

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

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

Cheers,
Giuliano.

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

[Mark Wielaard]

On Tue, 2022-05-17 at 14:51 +0100, Giuliano Procida wrote:
> > > +  if (symbol_sptr->is_function() && is_ppc64)
> 
> IIRC, someone said that this should apply to PPC32 as well, perhaps
> you, Mark?

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/

Cheers,

Mark

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

[Dodji Seketeli]

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

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

[Mark Wielaard]

Hi Dodji,

On Fri, May 27, 2022 at 06:33:07PM +0200, Dodji Seketeli wrote:
> I have added support for ppc32 as well, as outlined above.
> 
> Please find below the updated patch that I am proposing.
> 
> 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>

I read over it one more time and it all looks good to me.

Cheers,

Mark

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

[Dodji Seketeli]

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

[...]

>> Signed-off-by: Mark Wielaard <mark@klomp.org>
>> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>
> I read over it one more time and it all looks good to me.

Thanks a lot!

I have just applied to master.

Cheers,

-- 
		Dodji

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

[Mark Wielaard]

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 fixes runtestslowselfcompare.sh on ppc64 (ELFv1) with
ENABLE_SLOW_TEST=yes

	* src/abg-symtab-reader.cc (symtab::load_): Call
	update_function_entry_address_symbol_map after setting
	up aliases.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/abg-symtab-reader.cc | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
index b42ce87..0f7ace9 100644
--- a/src/abg-symtab-reader.cc
+++ b/src/abg-symtab-reader.cc
@@ -352,16 +352,13 @@ symtab::load_(Elf*	       elf_handle,
 								    sym);
 
 	  // See also symtab::add_alternative_address_lookups.
-	  if (symbol_sptr->is_function())
+	  // Note, do this before setting up aliases
+	  if (symbol_sptr->is_function() && is_arm32)
 	    {
-	      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, sym,
-							 symbol_sptr);
+	      // 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;
 	    }
 
 	  const auto result =
@@ -371,6 +368,14 @@ symtab::load_(Elf*	       elf_handle,
 	    // 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);
+
+	  // See also symtab::add_alternative_address_lookups.
+	  // Note, do this after setting up aliases.
+	  if (symbol_sptr->is_function() && is_ppc64)
+	    {
+	      update_function_entry_address_symbol_map(elf_handle, sym,
+						       symbol_sptr);
+	    }
 	}
     }
 
-- 
1.8.3.1