From patchwork Wed Nov 29 19:07:54 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 24605 Received: (qmail 110505 invoked by alias); 29 Nov 2017 19:08:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 110482 invoked by uid 89); 29 Nov 2017 19:08:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, KB_WAM_FROM_NAME_SINGLEWORD, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=BE, sk:get_obj, sal, offending X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Nov 2017 19:07:59 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CC33680469; Wed, 29 Nov 2017 19:07:58 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 01A80600D5; Wed, 29 Nov 2017 19:07:54 +0000 (UTC) Subject: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors) (was: Re: ppc64 regression: [PATCH 1/2] Fix "list ambiguous_variable") To: Ulrich Weigand , Jan Kratochvil References: <20171126163756.1515ED802F9@oc3748833570.ibm.com> Cc: Simon Marchi , Keith Seitz , gdb-patches@sourceware.org From: Pedro Alves Message-ID: <0bea6805-b8eb-da2c-07f6-0f1ee917c7b5@redhat.com> Date: Wed, 29 Nov 2017 19:07:54 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20171126163756.1515ED802F9@oc3748833570.ibm.com> On 11/26/2017 04:37 PM, Ulrich Weigand wrote: > I now see this as well on Cell/B.E. This is a serious regression that > causes "start" to always fail for me ... > > The problem seems to be that GDB sets a breakpoint into the function > descriptor for main, which is not a good idea. > > Looking at the commit identified above, it seems that GDB now only > runs the address through gdbarch_convert_from_func_ptr_addr if > msymbol_is_text returns true. However, if the symbol points to > a function descriptor, msymbol_is_text would be false since this > is in fact outside the text section. > > So I think probably we need to still run the address through > gdbarch_convert_from_func_ptr_addr, and if that detects that it > was indeed a function descriptor, always treat the resulting > address as a function. Thanks for the suggestion. Here's a patch that implements something like that. WDYT? I've pushed this to the users/palves/fix-ppc64-func-descriptors branch as well. From d1e2a9dc61a645ff29606f01f96a02b477761c23 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 29 Nov 2017 18:06:33 +0000 Subject: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors) The recent-ish commit e5f25bc5d6db ('Fix "list ambiguous_variable"') caused a serious regression on PPC64. See . Basically, after that patch, GDB sets breakpoints in function descriptors instead of where the descriptors point to, which is incorrect. The problem is that GDB now only runs a minsym's address through gdbarch_convert_from_func_ptr_addr if msymbol_is_text returns true. However, if the symbol points to a function descriptor, msymbol_is_text is false since function descriptors are in fact outside the text section. The fix is to also run a non-text address through gdbarch_convert_from_func_ptr_addr, and if that detects that it was indeed a function descriptor, treat the resulting address as a function. While implementing that directly in linespec.c:minsym_found (where the bad msymbol_is_text check is) fixes the issue, I noticed that linespec.c:add_minsym has some code that also basically needs to do the same checks, however it's implemented differently. Also, add_minsym is calling find_pc_sect_line on non-function symbols, which also doesn't look right. So I introduced msymbol_is_function, so that we have a simple place to consider minsyms and function descriptors. And then, the only other use of msymbol_is_text is in find_function_alias_target, which turns out to also be incorrect. Changing that one to use msymbol_is_function, i.e., to consider function descriptors too fixes (on PPC64): -FAIL: gdb.base/symbol-alias.exp: p func_alias -FAIL: gdb.base/symbol-alias.exp: p *func_alias() +PASS: gdb.base/symbol-alias.exp: p func_alias +PASS: gdb.base/symbol-alias.exp: p *func_alias() And then after that, msymbol_is_text is no longer used anywhere, so it can be removed. Tested on x86_64 GNU/Linux, no regressions. Tested on PPC64 GNU/Linux and results compared to a testrun of e5f25bc5d6db^ (before the offending commit), also no regressions. (there's a couple new FAILs and some new symbol name matching unit tests are crashing, but that looks unrelated). gdb/ChangeLog: yyyy-mm-dd Pedro Alves * linespec.c (minsym_found, add_minsym): Use msymbol_is_function. * minsyms.c (msymbol_is_text): Delete. (msymbol_is_function): New function. * minsyms.h (msymbol_is_text): Delete. (msymbol_is_function): New declaration. * symtab.c (find_function_alias_target): Use msymbol_is_function. --- gdb/linespec.c | 83 ++++++++++++++++------------------------------------------ gdb/minsyms.c | 34 ++++++++++++++++++------ gdb/minsyms.h | 11 +++++--- gdb/symtab.c | 8 +++--- 4 files changed, 61 insertions(+), 75 deletions(-) diff --git a/gdb/linespec.c b/gdb/linespec.c index 3f7f171..83f9bbf 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -4309,22 +4309,16 @@ minsym_found (struct linespec_state *self, struct objfile *objfile, struct minimal_symbol *msymbol, std::vector *result) { - struct gdbarch *gdbarch = get_objfile_arch (objfile); - CORE_ADDR pc; struct symtab_and_line sal; - if (msymbol_is_text (msymbol)) - { - sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol), - (struct obj_section *) 0, 0); - sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol); + /* The minimal symbol might point to a function descriptor, which is + not a text symbol; try resolving it to the actual code + address. */ - /* The minimal symbol might point to a function descriptor; - resolve it to the actual code address instead. */ - pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, - ¤t_target); - if (pc != sal.pc) - sal = find_pc_sect_line (pc, NULL, 0); + CORE_ADDR func_addr; + if (msymbol_is_function (objfile, msymbol, &func_addr)) + { + sal = find_pc_sect_line (func_addr, NULL, 0); if (self->funfirstline) { @@ -4332,14 +4326,9 @@ minsym_found (struct linespec_state *self, struct objfile *objfile, && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab)) || SYMTAB_LANGUAGE (sal.symtab) == language_asm)) { - /* If gdbarch_convert_from_func_ptr_addr does not apply then - sal.SECTION, sal.LINE&co. will stay correct from above. - If gdbarch_convert_from_func_ptr_addr applies then - sal.SECTION is cleared from above and sal.LINE&co. will - stay correct from the last find_pc_sect_line above. */ - sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol); - sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, - ¤t_target); + struct gdbarch *gdbarch = get_objfile_arch (objfile); + + sal.pc = func_addr; if (gdbarch_skip_entrypoint_p (gdbarch)) sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc); } @@ -4424,52 +4413,26 @@ static void add_minsym (struct minimal_symbol *minsym, void *d) { struct collect_minsyms *info = (struct collect_minsyms *) d; - bound_minimal_symbol_d mo; - - mo.minsym = minsym; - mo.objfile = info->objfile; if (info->symtab != NULL) { - CORE_ADDR pc; - struct symtab_and_line sal; - struct gdbarch *gdbarch = get_objfile_arch (info->objfile); - - sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (info->objfile, minsym), - NULL, 0); - sal.section = MSYMBOL_OBJ_SECTION (info->objfile, minsym); - pc - = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, ¤t_target); - if (pc != sal.pc) - sal = find_pc_sect_line (pc, NULL, 0); + /* We're looking for a label for which we don't have debug + info. */ + CORE_ADDR func_addr; + if (msymbol_is_function (info->objfile, minsym, &func_addr)) + { + symtab_and_line sal = find_pc_sect_line (func_addr, NULL, 0); - if (info->symtab != sal.symtab) - return; + if (info->symtab != sal.symtab) + return; + } } - /* Exclude data symbols when looking for breakpoint locations. */ - if (!info->list_mode) - switch (minsym->type) - { - case mst_slot_got_plt: - case mst_data: - case mst_bss: - case mst_abs: - case mst_file_data: - case mst_file_bss: - { - /* Make sure this minsym is not a function descriptor - before we decide to discard it. */ - struct gdbarch *gdbarch = get_objfile_arch (info->objfile); - CORE_ADDR addr = gdbarch_convert_from_func_ptr_addr - (gdbarch, BMSYMBOL_VALUE_ADDRESS (mo), - ¤t_target); - - if (addr == BMSYMBOL_VALUE_ADDRESS (mo)) - return; - } - } + /* Exclude data symbols when looking for breakpoint locations. */ + if (!info->list_mode && !msymbol_is_function (info->objfile, minsym)) + return; + bound_minimal_symbol_d mo = {minsym, info->objfile}; VEC_safe_push (bound_minimal_symbol_d, info->msyms, &mo); } diff --git a/gdb/minsyms.c b/gdb/minsyms.c index 4898da1..d68fb65 100644 --- a/gdb/minsyms.c +++ b/gdb/minsyms.c @@ -57,17 +57,35 @@ /* See minsyms.h. */ bool -msymbol_is_text (minimal_symbol *msymbol) +msymbol_is_function (struct objfile *objfile, minimal_symbol *minsym, + CORE_ADDR *func_address_p) { - switch (MSYMBOL_TYPE (msymbol)) + CORE_ADDR msym_addr = MSYMBOL_VALUE_ADDRESS (objfile, minsym); + + switch (minsym->type) { - case mst_text: - case mst_text_gnu_ifunc: - case mst_solib_trampoline: - case mst_file_text: - return true; + case mst_slot_got_plt: + case mst_data: + case mst_bss: + case mst_abs: + case mst_file_data: + case mst_file_bss: + { + struct gdbarch *gdbarch = get_objfile_arch (objfile); + CORE_ADDR pc = gdbarch_convert_from_func_ptr_addr (gdbarch, msym_addr, + ¤t_target); + if (pc != msym_addr) + { + if (func_address_p != NULL) + *func_address_p = pc; + return true; + } + return false; + } default: - return false; + if (func_address_p != NULL) + *func_address_p = msym_addr; + return true; } } diff --git a/gdb/minsyms.h b/gdb/minsyms.h index 5c0dde4..baa87f0 100644 --- a/gdb/minsyms.h +++ b/gdb/minsyms.h @@ -159,9 +159,14 @@ void terminate_minimal_symbol_table (struct objfile *objfile); -/* Return whether MSYMBOL is a function/method. */ - -bool msymbol_is_text (minimal_symbol *msymbol); +/* Return whether MSYMBOL is a function/method. If FUNC_ADDRESS_P is + non-NULL, and the MSYMBOL is a function, then *FUNC_ADDRESS_P is + set to the function's address, already resolved if MINSYM points to + a function descriptor. */ + +bool msymbol_is_function (struct objfile *objfile, + minimal_symbol *minsym, + CORE_ADDR *func_address_p = NULL); /* Compute a hash code for the string argument. */ diff --git a/gdb/symtab.c b/gdb/symtab.c index 3d59367..a249a8d 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -3927,14 +3927,14 @@ skip_prologue_using_sal (struct gdbarch *gdbarch, CORE_ADDR func_addr) symbol * find_function_alias_target (bound_minimal_symbol msymbol) { - if (!msymbol_is_text (msymbol.minsym)) + CORE_ADDR func_addr; + if (!msymbol_is_function (msymbol.objfile, msymbol.minsym, &func_addr)) return NULL; - CORE_ADDR addr = BMSYMBOL_VALUE_ADDRESS (msymbol); - symbol *sym = find_pc_function (addr); + symbol *sym = find_pc_function (func_addr); if (sym != NULL && SYMBOL_CLASS (sym) == LOC_BLOCK - && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == addr) + && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == func_addr) return sym; return NULL;