From 4024cf2b8d864279ff87af1a2ade77ab6d710d50 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 29 Nov 2017 19:25:58 +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: 2017-11-29 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/ChangeLog | 9 ++++++ gdb/linespec.c | 79 ++++++++++++-------------------------------------- gdb/minsyms.c | 34 +++++++++++++++++----- gdb/minsyms.h | 11 +++++-- gdb/symtab.c | 8 ++--- 5 files changed, 66 insertions(+), 75 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index bd1eccfb687..c8f8e2671ac 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2017-11-29 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. + 2017-11-29 Tom Tromey * Makefile.in (distclean): Handle the case where rmdir fails. diff --git a/gdb/linespec.c b/gdb/linespec.c index 3f7f1713589..f1e1ea97749 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -4309,22 +4309,12 @@ 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)) + CORE_ADDR func_addr; + if (msymbol_is_function (objfile, msymbol, &func_addr)) { - 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; - 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); + sal = find_pc_sect_line (func_addr, NULL, 0); if (self->funfirstline) { @@ -4332,14 +4322,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 +4409,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 4898da17aa7..d68fb65a9e0 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 5c0dde4de15..baa87f038af 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 3d5936774db..a249a8d98ea 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; -- 2.30.2