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

[Giuliano Procida <gprocida@google.com>]

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.