From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66128 invoked by alias); 12 Feb 2019 13:27:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 66119 invoked by uid 89); 12 Feb 2019 13:27:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.4 required=5.0 tests=BAYES_00,GIT_PATCH_2,KAM_SHORT,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=blogs X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Feb 2019 13:27:02 +0000 Received: by mail-wr1-f65.google.com with SMTP id v16so2638441wrn.11 for ; Tue, 12 Feb 2019 05:27:01 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:75e6:857f:3506:a1f4? ([2001:8a0:f913:f700:75e6:857f:3506:a1f4]) by smtp.gmail.com with ESMTPSA id c1sm16613415wrf.20.2019.02.12.05.26.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 12 Feb 2019 05:26:57 -0800 (PST) Subject: Re: [RFAv2 3/3] Make symtab.c better styled. To: Philippe Waroquiers , gdb-patches@sourceware.org, Ulrich Weigand References: <20190112222835.16932-1-philippe.waroquiers@skynet.be> <20190112222835.16932-4-philippe.waroquiers@skynet.be> <393f2825-8cc8-b71a-4e61-1edfd944cb84@redhat.com> <1549708558.12345.4.camel@skynet.be> From: Pedro Alves Message-ID: Date: Tue, 12 Feb 2019 13:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1549708558.12345.4.camel@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2019-02/txt/msg00133.txt.bz2 On 02/09/2019 10:35 AM, Philippe Waroquiers wrote: > On Thu, 2019-02-07 at 18:58 +0000, Pedro Alves wrote: >> On 01/12/2019 10:28 PM, Philippe Waroquiers wrote: >> >>> @@ -4667,8 +4685,15 @@ print_msymbol_info (struct bound_minimal_symbol msymbol) >>> else >>> tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol), >>> 16); >>> - printf_filtered ("%s %s\n", >>> - tmp, MSYMBOL_PRINT_NAME (msymbol.minsym)); >>> + fputs_styled (tmp, address_style.style (), gdb_stdout); >>> + fputs_filtered (" ", gdb_stdout); >>> + if (msymbol_type_text_p (msymbol)) >>> + fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym), >>> + function_name_style.style (), >>> + gdb_stdout); >>> + else >>> + fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout); >>> + fputs_filtered ("\n", gdb_stdout); >> >> Should this use the existing msymbol_is_function instead? > That is a good question. Note that there was a v3 where > if (msymbol_type_text_p (msymbol)) > is replaced by > if (msymbol.minsym->text_p ()) > > The below summarizes the behavior difference if we change the patch > to rather use msymbol_is_function instead of msymbol.minsym->text_p (): > >                             patch   msymbol_is_function >                             -----   ------------------- >   mst_unknown >   mst_text,     text_p  function >   mst_text_gnu_ifunc,       text_p  function >   mst_data_gnu_ifunc,     text_p  maybe >   mst_slot_got_plt,     text_p  maybe >   mst_data,                 data_p  maybe >   mst_bss,     data_p  maybe >   mst_abs,     data_p  maybe >   mst_solib_trampoline,     text_p  function >   mst_file_text,     text_p  function >   mst_file_data,     data_p  maybe >   mst_file_bss,     data_p  maybe >   nr_minsym_types > > The 'patch' first colummn indicates how the patch classifies a > msymbol type.  The 'msymbol_is_function' describes the behaviour > if msymbol_is_function is used instead of 'text_p'. > maybe indicates msymbol_is_function returns True if the msymbol address > can be converted to a different address using > gdbarch_convert_from_func_ptr_addr. > > When trying to use msymbol_is_function, I however do not see > any difference of behavior on debian/amd64with a small executable. > > The logic I used for the patch was: there are already 2 places > that define the set of msymbol types that are data, so I assumed > the rest was text, and the resulting type set looked plausible. > > It is however somewhat bizarre to have msymbol_is_function that > will sometime say that data is a function (and then 'gives back' > another address than the msymbol address). > > So, not very clear which one is better/correct, as this msymbol > concept is not very clear to me, and I see no > difference of behavior to decide which one looks better. On some platforms (PPC64 v1), functions in the symbol tables are actually function descriptors, which are data symbols. Addresses of functions (and thus C function pointers) are addresses of those function descriptors. See here: https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/deeply_understand_64_bit_powerpc_elf_abi_function_descriptors?lang=en So even though function descriptors are data symbols, they're basically treated as text/code symbols. The msymbol_is_function function returns true for them, and optionally returns the resolved entry address (which is where we place a breakpoint). Since these function descriptor symbols represent functions, even though they're data symbols in the elf tables, it would seem natural to me to print them using the function name style. What I'm not sure is whether we'll reach print_msymbol_info with data descriptor symbols when we do "info functions". Seems like search_symbols won't consider those symbols when looking for functions for "info functions". So we'd find those symbols only with "info variables", and then we'd print them as functions? I guess it could be argued that we should fix search_symbols / "info functions" to consider function descriptors, and use msymbol_is_function in print_msymbol_info to decide whether to use function style. I'm not 100% sure it needs fixing, and whether making "info functions" find the descriptors like that is the desired behavior. It wouldn't be fair to impose "info functions" on PPC64 on you, though. So if we don't fix that (and assuming it actually needs fixing, I'm not sure), then I guess the question is whether we should still print function descriptor symbols in function style, via "info variable" or whatever other paths could end up calling print_msymbol_info in the future. I'm not 100% sure what is preferred here. Ulrich, thoughts? Thanks, Pedro Alves