From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [IPv6:2001:4b98:dc4:8::231]) by sourceware.org (Postfix) with ESMTPS id 452583839C5F for ; Mon, 16 May 2022 14:34:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 452583839C5F 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 A146E100011; Mon, 16 May 2022 14:34:23 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 097F8581C5E; Mon, 16 May 2022 11:43:57 +0200 (CEST) From: Dodji Seketeli To: Mark Wielaard , gprocida@google.com Cc: libabigail@sourceware.org Subject: Re: [PATCH] symtab-reader: Setup aliases before checking ppc64 opd function entries Organization: Me, myself and I X-Operating-System: Fedora 36 X-URL: http://www.seketeli.net/~dodji Message-Id: <20220516143423.097F8581C5E@localhost> Date: Mon, 16 May 2022 11:43:57 +0200 (CEST) X-Spam-Status: No, score=-9.1 required=5.0 tests=BAYES_00, DATE_IN_PAST_03_06, 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: Mon, 16 May 2022 14:34:27 -0000 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 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 Signed-off-by: Dodji Seketeli --- 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