From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com [IPv6:2607:f8b0:4864:20::d2c]) by sourceware.org (Postfix) with ESMTPS id DE10D3858D3C for ; Tue, 17 May 2022 13:52:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DE10D3858D3C Received: by mail-io1-xd2c.google.com with SMTP id m6so19292594iob.4 for ; Tue, 17 May 2022 06:52:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=yYRizxyO0eJAkf2+EB6bkl+o8JtfYcb6qeVEKOGa3wE=; b=ROR2KDspiZkMvxj03h9SLl75VJZGqNXrmLABI687xkajRvbOv/4BSF0px7pRaOgdKx np+TVuNTdC6Qsi8WygiJIsYgv+5kd/9p3Ic9wXQvIQYUOYzzDvJfzPrpyoDdhAPiquLK 9p/Irz8klxf+o9e6nt67gm2cMjgyfguj44+66t5wzj9JCl8+iORXnharOsw+hYZ18S6X 4hCnrpr/SzsCQX5IjwgZgEqdsWE4hhXv/kxYVj3uTXzG6V9Qx3F5GvCN7kWZ+v59RaWg umXcbTLqHXXQnUIIRnTEr00lM0FhTw8tDeFgaufaSZYZjjoJTf7L7wPOFjZt4iDPCsVI JUtA== X-Gm-Message-State: AOAM5324CxNHgTL2LzwuOTgZ8SYPE1t13bAQvCH0tWD0slYOlmTGUFv8 u9AVCyOx3jMpa2xfOPf6x4mSKK1PsG12YIUSVuC3sw== X-Google-Smtp-Source: ABdhPJwnoaC5CSmc+ncoprRwhQD4Mwslw8mK8ihbvge+91nXQKrYJcP95pEvPlO583/kdOgyI4OEbkJM5QuFRtp7P0g= X-Received: by 2002:a05:6638:16c7:b0:32b:cc95:2f42 with SMTP id g7-20020a05663816c700b0032bcc952f42mr11757234jat.92.1652795540938; Tue, 17 May 2022 06:52:20 -0700 (PDT) MIME-Version: 1.0 References: <20220516143423.097F8581C5E@localhost> In-Reply-To: From: Giuliano Procida Date: Tue, 17 May 2022 14:51:44 +0100 Message-ID: Subject: Re: [PATCH] symtab-reader: Setup aliases before checking ppc64 opd function entries To: Mark Wielaard Cc: Dodji Seketeli , libabigail@sourceware.org, =?UTF-8?Q?Matthias_M=C3=A4nnich?= Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-27.1 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL 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: Tue, 17 May 2022 13:52:24 -0000 Hi. On Tue, 17 May 2022 at 10:59, Mark Wielaard 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 > > 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 > > 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 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.