From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by sourceware.org (Postfix) with ESMTPS id 7CFFF3851C0A for ; Fri, 27 May 2022 16:33:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7CFFF3851C0A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by mail.gandi.net (Postfix) with ESMTPSA id 627C5C0008; Fri, 27 May 2022 16:33:08 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 7576A5802B4; Fri, 27 May 2022 18:33:07 +0200 (CEST) From: Dodji Seketeli To: Mark Wielaard Cc: gprocida@google.com, maennich@google.com, libabigail@sourceware.org Subject: Re: [PATCH] symtab-reader: Setup aliases before checking ppc64 opd function entries Organization: Me, myself and I References: <20220516143423.097F8581C5E@localhost> X-Operating-System: Fedora 36 X-URL: http://www.seketeli.net/~dodji Date: Fri, 27 May 2022 18:33:07 +0200 In-Reply-To: (Mark Wielaard's message of "Tue, 17 May 2022 11:59:50 +0200") Message-ID: <877d67szp8.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 May 2022 16:33:14 -0000 Hello Mark & Giuliano and Matthias! Mark Wielaard a =C3=A9crit: [...] > 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 =3D elf_helpers::architecture_is_arm32(elf_handle= ); >> const bool is_arm64 =3D 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); >>=20=20 >> - 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 &=3D ~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 &=3D ~1; >> + >> if (is_arm64) >> // Copy bit 55 over bits 56 to 63 which may be tag information. >> symbol_value =3D 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 =3D >> + 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 a =C3=A9crit: > 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 co= rrect >> > /// interpretation (one that matches DWARF addresses) of raw symbol v= alues. >> > /// >> > +/// 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 =3D elf_helpers::architecture_is_arm32(elf_hand= le); >> > const bool is_arm64 =3D elf_helpers::architecture_is_arm64(elf_hand= le); >> > @@ -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 &=3D ~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 &=3D ~1; >> > + >> > if (is_arm64) >> > // Copy bit 55 over bits 56 to 63 which may be tag information. >> > symbol_value =3D 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 =3D >> > + 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 a =C3=A9crit: > 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 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=3Dyes * 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 Signed-off-by: Dodji Seketeli --- 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 =3D=3D EM_PPC64); } =20 +/// 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 =3D gelf_getehdr(elf_handle, &eh_mem); + return (elf_header && elf_header->e_machine =3D=3D 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); =20 +bool +architecture_is_ppc32(Elf* elf_handle); + bool architecture_is_arm32(Elf* elf_handle); =20 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 =3D - get_symbol_value(elf_handle, sym, symbol_sptr); - const auto result =3D - 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); } =20 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 =3D elf_helpers::architecture_is_arm32(elf_handle); const bool is_arm64 =3D elf_helpers::architecture_is_arm64(elf_handle); const bool is_ppc64 =3D elf_helpers::architecture_is_ppc64(elf_handle); + const bool is_ppc32 =3D elf_helpers::architecture_is_ppc32(elf_handle); =20 GElf_Addr symbol_value =3D elf_helpers::maybe_adjust_et_rel_sym_addr_to_abs_addr(elf_handle, elf_symbol); =20 - 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 &=3D ~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 &=3D ~1; + if (is_arm64) // Copy bit 55 over bits 56 to 63 which may be tag information. symbol_value =3D symbol_value & (1ULL<<55) ? symbol_value | (0xffULL<<56) : symbol_value &~ (0xffULL<<56); =20 + if (symbol_sptr->is_defined()) + { + const auto result =3D + 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; } =20 @@ -656,9 +664,7 @@ symtab::add_alternative_address_lookups(Elf* elf_handle) if (symbols.size() =3D=3D 1) { const auto& symbol_sptr =3D symbols[0]; - const GElf_Addr symbol_value =3D - 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); =20 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); =20 void update_function_entry_address_symbol_map(Elf* elf_handle, --=20 2.36.1 Cheers, --=20 Dodji