| Message ID | 20190608195434.26512-2-kevinb@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 6/8/19 8:54 PM, Kevin Buettner wrote: > The discussion on gdb-patches which led to this patch may be found > here: > > https://www.sourceware.org/ml/gdb-patches/2019-05/msg00018.html > > Here's a brief synopsis/analysis: > > Eli Zaretskii, while debugging a Windows emacs executable, found > that functions comprised of more than one (non-contiguous) > address range were not being displayed correctly in a backtrace. This > is the example that Eli provided: > > (gdb) bt > #0 0x76a63227 in KERNELBASE!DebugBreak () > from C:\Windows\syswow64\KernelBase.dll > #1 0x012e7b89 in emacs_abort () at w32fns.c:10768 > #2 0x012e1f3b in print_vectorlike.cold () at print.c:1824 > #3 0x011d2dec in print_object (obj=<optimized out>, printcharfun=XIL(0), > escapeflag=true) at print.c:2150 > > The function print_vectorlike consists of two address ranges, one of > which contains "cold" code which is expected to not execute very often. > There is a minimal symbol, print_vectorlike.cold.65, which is the address > of the "cold" range. > > GDB is prefering this minsym over the the name provided by the > DWARF info due to some really old code in GDB which handles > "certain pathological cases". See the first big block comment > in find_frame_funname for more information. Yuck! > > I considered removing the code for this corner case entirely, but it > seems as though it might still be useful, so I left it intact. > Yeah, I'd be inclined to try removing it too. The comment smells of a.out or stabs limitations. But I'm not 100% sure, and I'm sympathetic with forward incremental progress. > That code is already disabled for inline functions. I added a > condition which disables it for non-contiguous functions as well. > > The change to find_frame_funname in stack.c fixes this problem for > stack traces. A similar change to print_address_symbolic in > printcmd.c fixes this problem for the "x" command and other commands > which use print_address_symbolic(). > > gdb/ChangeLog: > > * stack.c (find_frame_funname): Disable use of minsym for function > name in functions comprised of non-contiguous blocks. > * printcmd.c (print_address_symbolic): Likewise. > --- > gdb/printcmd.c | 4 +++- > gdb/stack.c | 15 ++++++++++++--- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 9e84594fe6..e00a9c671a 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -627,7 +627,9 @@ build_address_symbolic (struct gdbarch *gdbarch, > > if (msymbol.minsym != NULL) > { > - if (BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location || symbol == NULL) > + if (symbol == NULL > + || (BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (symbol)) > + && BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location)) I think this warrants a comment somewhere. Maybe a bit higher up, where it reads: /* First try to find the address in the symbol table, then in the minsyms. Take the closest one. */ Or better yet, abstract out the relevant bits in build_address_symbolic and find_frame_funname to a separate function, and then use that function in both places? IIUC, in both cases, we're looking to see if there's a minimal symbol that would be better than the debug symbol we start with. > @@ -1067,9 +1067,18 @@ find_frame_funname (struct frame_info *frame, enum language *funlang, > > struct bound_minimal_symbol msymbol; > > - /* Don't attempt to do this for inlined functions, which do not > - have a corresponding minimal symbol. */ > - if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func))) > + /* Don't attempt to do this for two cases: > + > + 1) Inlined functions, which do not have a corresponding minimal > + symbol. > + > + 2) Functions which are comprised of non-contiguous blocks. > + Such functions often contain a minimal symbol for a > + "cold" range, i.e. code which is not expected to execute > + very often. It is incorrect to use the minimal symbol > + associated with this range. */ I don't find this "incorrect" here very useful -- why is it incorrect is my immediate question when I read this. Maybe that old comment: So look in the minimal symbol tables as well, and if it comes up with a larger address for the function use that instead. I don't think this can ever cause any problems; there shouldn't be any minimal symbols in the middle of a function; should be updated. Thanks, Pedro Alves
>> GDB is prefering this minsym over the the name provided by the >> DWARF info due to some really old code in GDB which handles >> "certain pathological cases". See the first big block comment >> in find_frame_funname for more information. Pedro> Yuck! >> I considered removing the code for this corner case entirely, but it >> seems as though it might still be useful, so I left it intact. Pedro> Yeah, I'd be inclined to try removing it too. The comment Pedro> smells of a.out or stabs limitations. But I'm not 100% sure, Pedro> and I'm sympathetic with forward incremental progress. That code dates to the creation of the sourceware repository. I think it could be deleted. And, if it's still a bug somehow, it seems better to fix it some other way, and not let stabs and/or a.out weirdness into the generic code. Tom
On Wed, 26 Jun 2019 11:30:51 -0600 Tom Tromey <tom@tromey.com> wrote: > >> GDB is prefering this minsym over the the name provided by the > >> DWARF info due to some really old code in GDB which handles > >> "certain pathological cases". See the first big block comment > >> in find_frame_funname for more information. > > Pedro> Yuck! > > >> I considered removing the code for this corner case entirely, but it > >> seems as though it might still be useful, so I left it intact. > > Pedro> Yeah, I'd be inclined to try removing it too. The comment > Pedro> smells of a.out or stabs limitations. But I'm not 100% sure, > Pedro> and I'm sympathetic with forward incremental progress. > > That code dates to the creation of the sourceware repository. > > I think it could be deleted. And, if it's still a bug somehow, it seems > better to fix it some other way, and not let stabs and/or a.out > weirdness into the generic code. Okay, for v2 of this patch series, I'll get rid of that code. Thanks, Kevin
diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 9e84594fe6..e00a9c671a 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -627,7 +627,9 @@ build_address_symbolic (struct gdbarch *gdbarch, if (msymbol.minsym != NULL) { - if (BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location || symbol == NULL) + if (symbol == NULL + || (BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (symbol)) + && BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location)) { /* If this is a function (i.e. a code address), strip out any non-address bits. For instance, display a pointer to the diff --git a/gdb/stack.c b/gdb/stack.c index 408c795e38..d511690a14 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1067,9 +1067,18 @@ find_frame_funname (struct frame_info *frame, enum language *funlang, struct bound_minimal_symbol msymbol; - /* Don't attempt to do this for inlined functions, which do not - have a corresponding minimal symbol. */ - if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func))) + /* Don't attempt to do this for two cases: + + 1) Inlined functions, which do not have a corresponding minimal + symbol. + + 2) Functions which are comprised of non-contiguous blocks. + Such functions often contain a minimal symbol for a + "cold" range, i.e. code which is not expected to execute + very often. It is incorrect to use the minimal symbol + associated with this range. */ + if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func)) + && BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (func))) msymbol = lookup_minimal_symbol_by_pc (get_frame_address_in_block (frame)); else
The discussion on gdb-patches which led to this patch may be found here: https://www.sourceware.org/ml/gdb-patches/2019-05/msg00018.html Here's a brief synopsis/analysis: Eli Zaretskii, while debugging a Windows emacs executable, found that functions comprised of more than one (non-contiguous) address range were not being displayed correctly in a backtrace. This is the example that Eli provided: (gdb) bt #0 0x76a63227 in KERNELBASE!DebugBreak () from C:\Windows\syswow64\KernelBase.dll #1 0x012e7b89 in emacs_abort () at w32fns.c:10768 #2 0x012e1f3b in print_vectorlike.cold () at print.c:1824 #3 0x011d2dec in print_object (obj=<optimized out>, printcharfun=XIL(0), escapeflag=true) at print.c:2150 The function print_vectorlike consists of two address ranges, one of which contains "cold" code which is expected to not execute very often. There is a minimal symbol, print_vectorlike.cold.65, which is the address of the "cold" range. GDB is prefering this minsym over the the name provided by the DWARF info due to some really old code in GDB which handles "certain pathological cases". See the first big block comment in find_frame_funname for more information. I considered removing the code for this corner case entirely, but it seems as though it might still be useful, so I left it intact. That code is already disabled for inline functions. I added a condition which disables it for non-contiguous functions as well. The change to find_frame_funname in stack.c fixes this problem for stack traces. A similar change to print_address_symbolic in printcmd.c fixes this problem for the "x" command and other commands which use print_address_symbolic(). gdb/ChangeLog: * stack.c (find_frame_funname): Disable use of minsym for function name in functions comprised of non-contiguous blocks. * printcmd.c (print_address_symbolic): Likewise. --- gdb/printcmd.c | 4 +++- gdb/stack.c | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-)