* [PATCH] Ensure captured_main has unique address @ 2018-06-12 15:06 Tom de Vries 2018-06-12 17:38 ` Change inline frame breakpoint skipping logic (Re: [PATCH] Ensure captured_main has unique address) Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Tom de Vries @ 2018-06-12 15:06 UTC (permalink / raw) To: gdb-patches Hi, atm selftest.exp fails for me. One of the reasons is that after setting a breakpoint in captured_main, we stop at: ... Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 ... while selftest_setup expects to stop at captured_main. The problem is that captured_main_1 has been inlined into captured_main, and captured_main has been inlined into gdb_main: ... $ nm ./build/gdb/gdb | egrep ' [tT] .*captured_main|gdb_main' | c++filt 000000000061b950 T gdb_main(captured_main_args*) ... The reason that we seem to be stopping at inline function captured_main_1 has probably something to do with commit "Don't elide all inlined frames", which shows us the frames of inlined functions as if they were not inlined. Indeed, the two inlined functions show up in the backtrace: ... (gdb) bt #0 captured_main_1 (context=<optimized out>) at main.c:492 #1 captured_main (data=<optimized out>) at main.c:1147 #2 gdb_main (args=args@entry=0x7fffffffdb80) at main.c:1173 #3 0x000000000040fea5 in main (argc=<optimized out>, argv=<optimized out>) at gdb.c:32 ... [ For contrast, If I use my distro gdb to debug build/gdb/gdb instead, we just get: ... Breakpoint 1, gdb_main (args=args@entry=0x7fffffffdb80) at src/gdb/main.c:1173 1173 captured_main (args); ... ] Either way, this patch fixes the problem by ensuring that captured_main has a unique address: ... $ nm ./build/gdb/gdb | egrep ' [tT] .*captured_main|gdb_main' | c++filt 000000000061ca20 T gdb_main(captured_main_args*) 000000000061c980 t captured_main(void*) 000000000061b950 t captured_main_1(captured_main_args*) ... Tested selftest.exp (with two other selftest.exp related fixes applied). OK for trunk? Thanks, - Tom [gdb] Ensure captured_main has unique address 2018-06-12 Tom de Vries <tdevries@suse.de> * main.c (captured_main, captured_main_1): Add __attribute__((noinline)). --- gdb/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/main.c b/gdb/main.c index 9694af2426..f35dffd428 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -447,7 +447,7 @@ struct cmdarg char *string; }; -static void +static void __attribute__((noinline)) captured_main_1 (struct captured_main_args *context) { int argc = context->argc; @@ -1139,7 +1139,7 @@ captured_main_1 (struct captured_main_args *context) } } -static void +static void __attribute__((noinline)) captured_main (void *data) { struct captured_main_args *context = (struct captured_main_args *) data; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Change inline frame breakpoint skipping logic (Re: [PATCH] Ensure captured_main has unique address) 2018-06-12 15:06 [PATCH] Ensure captured_main has unique address Tom de Vries @ 2018-06-12 17:38 ` Pedro Alves 2018-06-14 13:22 ` Tom de Vries 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2018-06-12 17:38 UTC (permalink / raw) To: Tom de Vries, gdb-patches; +Cc: Keith Seitz On 06/12/2018 04:06 PM, Tom de Vries wrote: > Hi, > > atm selftest.exp fails for me. > > One of the reasons is that after setting a breakpoint in captured_main, we > stop at: > ... > Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 > ... > while selftest_setup expects to stop at captured_main. > > The problem is that captured_main_1 has been inlined into captured_main, and > captured_main has been inlined into gdb_main: > ... > $ nm ./build/gdb/gdb | egrep ' [tT] .*captured_main|gdb_main' | c++filt > 000000000061b950 T gdb_main(captured_main_args*) > ... > > The reason that we seem to be stopping at inline function captured_main_1 has > probably something to do with commit "Don't elide all inlined frames", Yes, sounds like it. But the selftest.exp explicitly asks to stop at "captured_main", not "captured_main_1", so I'm thinking that it's gdb's behavior that might be wrong: (top-gdb) b captured_main Breakpoint 3 at 0x792f99: file src/gdb/main.c, line 492. (top-gdb) r Starting program: build/gdb/gdb Breakpoint 3, captured_main_1 (context=<optimized out>) at /home/pedro/gdb/binutils-gdb/src/gdb/main.c:492 492 lim_at_start = (char *) sbrk (0); (top-gdb) With the patch below, we instead get: (top-gdb) b captured_main Breakpoint 6 at 0x791339: file src/gdb/main.c, line 492. (top-gdb) r Starting program: build/gdb/gdb Breakpoint 6, captured_main (data=<optimized out>) at src/gdb/main.c:1147 1147 captured_main_1 (context); (top-gdb) and: (top-gdb) b captured_main_1 Breakpoint 7 at 0x791339: file src/gdb/main.c, line 492. (top-gdb) r Starting program: build/gdb/gdb Breakpoint 7, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 492 lim_at_start = (char *) sbrk (0); (top-gdb) Note that both captured_main and captured_main_1 resolved to the same address, 0x791339. The gdb.base/inline-break.exp testcase currently does not exercise that, but the new test added by the patch below does. That new test fails without the patch and passes with the patch. No regressions on x86-64 GNU/Linux. WDYT? Also, while looking at stopped_by_user_bp_inline_frame I noticed that comparing the THIS_PC is basically a nop -- if a software or hardware breakpoint explains the stop, then it must be that it was installed at the current PC. From cf6924a2cfca397b80c7c935e048771de77d3105 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Tue, 12 Jun 2018 18:22:25 +0100 Subject: [PATCH] Inline breakpoints gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC parameter with a block parameter. Compare location's block symbol with the frame's block instead of addresses. (skip_inline_frames): Pass the current block instead of the frame's address. Break out as soon as we determine the frame should not be skipped. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * gdb.opt/inline-break.c (func_callee, func_caller): New. (main): Call func_caller. --- gdb/inline-frame.c | 23 +++++++++++------------ gdb/testsuite/gdb.opt/inline-break.c | 21 +++++++++++++++++++++ gdb/testsuite/gdb.opt/inline-break.exp | 17 +++++++++++++++++ 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 1ac5835438d..3edd5b2b20b 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -286,11 +286,10 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block) } /* Loop over the stop chain and determine if execution stopped in an - inlined frame because of a user breakpoint. THIS_PC is the current - frame's PC. */ + inlined frame because of a user breakpoint set at FRAME_BLOCK. */ static bool -stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain) +stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain) { for (bpstat s = stop_chain; s != NULL; s = s->next) { @@ -301,9 +300,9 @@ stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain) bp_location *loc = s->bp_location_at; enum bp_loc_type t = loc->loc_type; - if (loc->address == this_pc - && (t == bp_loc_software_breakpoint - || t == bp_loc_hardware_breakpoint)) + if ((t == bp_loc_software_breakpoint + || t == bp_loc_hardware_breakpoint) + && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) return true; } } @@ -340,12 +339,12 @@ skip_inline_frames (ptid_t ptid, bpstat stop_chain) { /* Do not skip the inlined frame if execution stopped in an inlined frame because of a user - breakpoint. */ - if (!stopped_by_user_bp_inline_frame (this_pc, stop_chain)) - { - skip_count++; - last_sym = BLOCK_FUNCTION (cur_block); - } + breakpoint for this inline function. */ + if (stopped_by_user_bp_inline_frame (cur_block, stop_chain)) + break; + + skip_count++; + last_sym = BLOCK_FUNCTION (cur_block); } else break; diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c index 922102debb6..6ad331fcc63 100644 --- a/gdb/testsuite/gdb.opt/inline-break.c +++ b/gdb/testsuite/gdb.opt/inline-break.c @@ -176,6 +176,25 @@ not_inline_func3 (int x) return y + inline_func3 (x); } +/* A static inlined function that is called by another static inlined + function. */ + +static inline ATTR int +func_callee (int x) +{ + return x * 23; +} + +/* A static inlined function that calls another static inlined + function. The body of the function is a simple as possible so that + both functions are inlined to the same PC address. */ + +static int +func_caller (int x) +{ + return func_callee (x); +} + /* Entry point. */ int @@ -205,5 +224,7 @@ main (int argc, char *argv[]) x = not_inline_func3 (-21); + func_caller (1); + return x; } diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp index 008ff1ac33a..f1ab3d23715 100644 --- a/gdb/testsuite/gdb.opt/inline-break.exp +++ b/gdb/testsuite/gdb.opt/inline-break.exp @@ -231,4 +231,21 @@ foreach_with_prefix cmd [list "break" "tbreak"] { } } +# func_caller and func_callee are both inline functions, and one calls +# the other. Test that setting a breakpoint on the caller reports the +# stop at the caller, and that setting a breakpoint at the callee +# reports a stop at the callee. +foreach_with_prefix func {"func_callee" "func_caller"} { + clean_restart $binfile + + if {![runto main]} { + untested "could not run to main" + continue + } + + gdb_breakpoint $func + gdb_test "continue" "Breakpoint .* $func .*at .*$srcfile.*" \ + "continue to inline function" +} + unset -nocomplain results -- 2.14.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Change inline frame breakpoint skipping logic (Re: [PATCH] Ensure captured_main has unique address) 2018-06-12 17:38 ` Change inline frame breakpoint skipping logic (Re: [PATCH] Ensure captured_main has unique address) Pedro Alves @ 2018-06-14 13:22 ` Tom de Vries 2018-06-19 16:36 ` [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Tom de Vries @ 2018-06-14 13:22 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: Keith Seitz On 06/12/2018 07:38 PM, Pedro Alves wrote: > On 06/12/2018 04:06 PM, Tom de Vries wrote: >> Hi, >> >> atm selftest.exp fails for me. >> >> One of the reasons is that after setting a breakpoint in captured_main, we >> stop at: >> ... >> Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 >> ... >> while selftest_setup expects to stop at captured_main. >> >> The problem is that captured_main_1 has been inlined into captured_main, and >> captured_main has been inlined into gdb_main: >> ... >> $ nm ./build/gdb/gdb | egrep ' [tT] .*captured_main|gdb_main' | c++filt >> 000000000061b950 T gdb_main(captured_main_args*) >> ... >> >> The reason that we seem to be stopping at inline function captured_main_1 has >> probably something to do with commit "Don't elide all inlined frames", > > Yes, sounds like it. But the selftest.exp explicitly asks to stop > at "captured_main", not "captured_main_1", so I'm thinking that > it's gdb's behavior that might be wrong: > > (top-gdb) b captured_main > Breakpoint 3 at 0x792f99: file src/gdb/main.c, line 492. > (top-gdb) r > Starting program: build/gdb/gdb > > Breakpoint 3, captured_main_1 (context=<optimized out>) at /home/pedro/gdb/binutils-gdb/src/gdb/main.c:492 > 492 lim_at_start = (char *) sbrk (0); > (top-gdb) > > With the patch below, we instead get: > > (top-gdb) b captured_main > Breakpoint 6 at 0x791339: file src/gdb/main.c, line 492. > (top-gdb) r > Starting program: build/gdb/gdb > > Breakpoint 6, captured_main (data=<optimized out>) at src/gdb/main.c:1147 > 1147 captured_main_1 (context); > (top-gdb) > > and: > > (top-gdb) b captured_main_1 > Breakpoint 7 at 0x791339: file src/gdb/main.c, line 492. > (top-gdb) r > Starting program: build/gdb/gdb > Breakpoint 7, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 > 492 lim_at_start = (char *) sbrk (0); > (top-gdb) > Agreed, that's a better solution. > Note that both captured_main and captured_main_1 resolved to the > same address, 0x791339. Right. I played around a bit with this, and set breakpoints on captured_main and captured_main_1. If I set a breakpoint on captured_main_1, we have captured_main unknown: ... Breakpoint 2, captured_main_1 (context=<optimized out>) at /home/vries/gdb_versions/devel/src/gdb/main.c:492 492 lim_at_start = (char *) sbrk (0); (gdb) p captured_main No symbol "captured_main" in current context. (gdb) p captured_main_1 $1 = {void (captured_main_args *)} 0x61b959 <gdb_main(captured_main_args*)+25> ... But If I set a breakpoint on captured_main instead, we have captured_main_1 unknown: ... Breakpoint 3, captured_main (data=<optimized out>) at /home/vries/gdb_versions/devel/src/gdb/main.c:1147 1147 captured_main_1 (context); (gdb) p captured_main $2 = {void (void *)} 0x61b959 <gdb_main(captured_main_args*)+25> (gdb) p captured_main_1 No symbol "captured_main_1" in current context. ... And if I set a breakpoint on both, captured_main_1 seems to take precedence (independent of the order used to set the breakpoint): ... Breakpoint 1, captured_main_1 (context=<optimized out>) at /home/vries/gdb_versions/devel/src/gdb/main.c:492 492 lim_at_start = (char *) sbrk (0); (gdb) p captured_main_1 $1 = {void (captured_main_args *)} 0x61b959 <gdb_main(captured_main_args*)+25> (gdb) p captured_main No symbol "captured_main" in current context. ... I don't understand the underlying mechanisms well enough to decide whether this is a problem or not, but I thought I just mention it. > The gdb.base/inline-break.exp testcase > currently does not exercise that, but the new test added by the > patch below does. That new test fails without the patch and passes > with the patch. No regressions on x86-64 GNU/Linux. WDYT? > AFAICT, the patch looks ok (just one nit below). > +/* A static inlined function that is called by another static inlined > + function. */ > + > +static inline ATTR int > +func_callee (int x) > +{ > + return x * 23; > +} > + > +/* A static inlined function that calls another static inlined > + function. The body of the function is a simple as possible so that > + both functions are inlined to the same PC address. */ > + > +static int inline ATTR ? > +func_caller (int x) > +{ > + return func_callee (x); > +} > + Thanks, - Tom ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) 2018-06-14 13:22 ` Tom de Vries @ 2018-06-19 16:36 ` Pedro Alves 2018-06-25 21:04 ` Joel Brobecker 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2018-06-19 16:36 UTC (permalink / raw) To: Tom de Vries, gdb-patches; +Cc: Keith Seitz On 06/14/2018 02:22 PM, Tom de Vries wrote: > On 06/12/2018 07:38 PM, Pedro Alves wrote: >> Yes, sounds like it. But the selftest.exp explicitly asks to stop >> at "captured_main", not "captured_main_1", so I'm thinking that >> it's gdb's behavior that might be wrong: >> > Agreed, that's a better solution. > >> Note that both captured_main and captured_main_1 resolved to the >> same address, 0x791339. > > Right. I played around a bit with this, and set breakpoints on > captured_main and captured_main_1. > > If I set a breakpoint on captured_main_1, we have captured_main unknown: > ... > Breakpoint 2, captured_main_1 (context=<optimized out>) > at /home/vries/gdb_versions/devel/src/gdb/main.c:492 > 492 lim_at_start = (char *) sbrk (0); > (gdb) p captured_main > No symbol "captured_main" in current context. > (gdb) p captured_main_1 > $1 = {void (captured_main_args *)} 0x61b959 > <gdb_main(captured_main_args*)+25> > ... > > But If I set a breakpoint on captured_main instead, we have > captured_main_1 unknown: > ... > Breakpoint 3, captured_main (data=<optimized out>) > at /home/vries/gdb_versions/devel/src/gdb/main.c:1147 > 1147 captured_main_1 (context); > (gdb) p captured_main > $2 = {void (void *)} 0x61b959 <gdb_main(captured_main_args*)+25> > (gdb) p captured_main_1 > No symbol "captured_main_1" in current context. > ... > > And if I set a breakpoint on both, captured_main_1 seems to take > precedence (independent of the order used to set the breakpoint): > ... > Breakpoint 1, captured_main_1 (context=<optimized out>) > at /home/vries/gdb_versions/devel/src/gdb/main.c:492 > 492 lim_at_start = (char *) sbrk (0); > (gdb) p captured_main_1 > $1 = {void (captured_main_args *)} 0x61b959 > <gdb_main(captured_main_args*)+25> > (gdb) p captured_main > No symbol "captured_main" in current context. > ... > > I don't understand the underlying mechanisms well enough to decide > whether this is a problem or not, but I thought I just mention it. Can't pinpoint offhand where the problem is, but sounds like something in the dwarf or elf symbol readers, maybe the breakpoint you set first changes the order in which symbols are read and are added to search hashes etc., or something like that. It's most certainly unrelated to this change though. > >> The gdb.base/inline-break.exp testcase >> currently does not exercise that, but the new test added by the >> patch below does. That new test fails without the patch and passes >> with the patch. No regressions on x86-64 GNU/Linux. WDYT? >> > > AFAICT, the patch looks ok (just one nit below). > >> +/* A static inlined function that is called by another static inlined >> + function. */ >> + >> +static inline ATTR int >> +func_callee (int x) >> +{ >> + return x * 23; >> +} >> + >> +/* A static inlined function that calls another static inlined >> + function. The body of the function is a simple as possible so that >> + both functions are inlined to the same PC address. */ >> + >> +static int > > inline ATTR ? Hmm, indeed. If I do that however gcc (7.3) seemingly optimizes out the functions more aggressively and we can't set a breakpoint anymore: (gdb) b func_inline_caller Function "func_inline_caller" not defined. Breakpoint 1 (func_inline_caller) pending. A quick look at the debug info reveals that the debug info does mention the functions, so this may be another gdb bug: <1><2c3>: Abbrev Number: 12 (DW_TAG_subprogram) <2c4> DW_AT_name : (indirect string, offset: 0x16f): func_inline_caller <2c8> DW_AT_decl_file : 1 <2c9> DW_AT_decl_line : 197 <2ca> DW_AT_prototyped : 1 <2ca> DW_AT_type : <0x2a4> <2ce> DW_AT_inline : 3 (declared as inline and inlined) <2cf> DW_AT_sibling : <0x2dd> ... <1><2dd>: Abbrev Number: 12 (DW_TAG_subprogram) <2de> DW_AT_name : (indirect string, offset: 0x112): func_inline_callee <2e2> DW_AT_decl_file : 1 <2e3> DW_AT_decl_line : 187 <2e4> DW_AT_prototyped : 1 <2e4> DW_AT_type : <0x2a4> <2e8> DW_AT_inline : 3 (declared as inline and inlined) <2e9> DW_AT_sibling : <0x2f7> I haven't investigated that one. In order to move forward with the frame skipping patch, I'm adding a non-inline caller level in the testcase instead. Below's what I've now merged. From 4de1f9e76460db0f6b97762ff368e5b8f0da16b0 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Tue, 19 Jun 2018 16:30:13 +0100 Subject: [PATCH] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) Currently, gdb.gdb/selftest.exp fails if you build GDB with optimization (-O2, etc.). The reason is that after setting a breakpoint in captured_main, we stop at: ... Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 ... while selftest_setup expects a stop at captured_main. Here, captured_main_1 has been inlined into captured_main, and captured_main has been inlined into gdb_main: ... $ nm ./build/gdb/gdb | egrep ' [tT] .*captured_main|gdb_main' | c++filt 000000000061b950 T gdb_main(captured_main_args*) ... Indeed, the two inlined functions show up in the backtrace: ... (gdb) bt #0 captured_main_1 (context=<optimized out>) at main.c:492 #1 captured_main (data=<optimized out>) at main.c:1147 #2 gdb_main (args=args@entry=0x7fffffffdb80) at main.c:1173 #3 0x000000000040fea5 in main (argc=<optimized out>, argv=<optimized out>) at gdb.c:32 ... We're now stopping at captured_main_1 because commit ddfe970e6bec ("Don't elide all inlined frames") makes GDB present a stop at the innermost inlined frame if the program stopped by a user breakpoint. Now, the selftest.exp testcase explicitly asks to stop at "captured_main", not "captured_main_1", so I'm thinking that it's GDB'S behavior that should be improved. That is what this commit does, by only showing a stop at an inline frame if the user breakpoint was set in that frame's block. Before this commit: (top-gdb) b captured_main Breakpoint 1 at 0x792f99: file src/gdb/main.c, line 492. (top-gdb) r Starting program: build/gdb/gdb Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 492 lim_at_start = (char *) sbrk (0); (top-gdb) After this commit, we now instead get: (top-gdb) b captured_main Breakpoint 1 at 0x791339: file src/gdb/main.c, line 492. (top-gdb) r Starting program: build/gdb/gdb Breakpoint 1, captured_main (data=<optimized out>) at src/gdb/main.c:1147 1147 captured_main_1 (context); (top-gdb) and: (top-gdb) b captured_main_1 Breakpoint 2 at 0x791339: file src/gdb/main.c, line 492. (top-gdb) r Starting program: build/gdb/gdb Breakpoint 2, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492 492 lim_at_start = (char *) sbrk (0); (top-gdb) Note that both captured_main and captured_main_1 resolved to the same address, 0x791339. That is necessary to trigger the issue in question. The gdb.base/inline-break.exp testcase currently does not exercise that, but the new test added by this commit does. That new test fails without the GDB fix and passes with the fix. No regressions on x86-64 GNU/Linux. While at it, the THIS_PC comparison in stopped_by_user_bp_inline_frame is basically a nop, so just remove it -- if a software or hardware breakpoint explains the stop, then it must be that it was installed at the current PC. gdb/ChangeLog: 2018-06-19 Pedro Alves <palves@redhat.com> * inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC parameter with a block parameter. Compare location's block symbol with the frame's block instead of addresses. (skip_inline_frames): Pass the current block instead of the frame's address. Break out as soon as we determine the frame should not be skipped. gdb/testsuite/ChangeLog: 2018-06-19 Pedro Alves <palves@redhat.com> * gdb.opt/inline-break.c (func_inline_callee, func_inline_caller) (func_extern_caller): New. (main): Call func_extern_caller. * gdb.opt/inline-break.exp: Add tests for inline frame skipping logic change. --- gdb/inline-frame.c | 23 +++++++++++------------ gdb/testsuite/gdb.opt/inline-break.c | 34 ++++++++++++++++++++++++++++++++++ gdb/testsuite/gdb.opt/inline-break.exp | 25 +++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 1ac5835438d..3edd5b2b20b 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -286,11 +286,10 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block) } /* Loop over the stop chain and determine if execution stopped in an - inlined frame because of a user breakpoint. THIS_PC is the current - frame's PC. */ + inlined frame because of a user breakpoint set at FRAME_BLOCK. */ static bool -stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain) +stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain) { for (bpstat s = stop_chain; s != NULL; s = s->next) { @@ -301,9 +300,9 @@ stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain) bp_location *loc = s->bp_location_at; enum bp_loc_type t = loc->loc_type; - if (loc->address == this_pc - && (t == bp_loc_software_breakpoint - || t == bp_loc_hardware_breakpoint)) + if ((t == bp_loc_software_breakpoint + || t == bp_loc_hardware_breakpoint) + && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) return true; } } @@ -340,12 +339,12 @@ skip_inline_frames (ptid_t ptid, bpstat stop_chain) { /* Do not skip the inlined frame if execution stopped in an inlined frame because of a user - breakpoint. */ - if (!stopped_by_user_bp_inline_frame (this_pc, stop_chain)) - { - skip_count++; - last_sym = BLOCK_FUNCTION (cur_block); - } + breakpoint for this inline function. */ + if (stopped_by_user_bp_inline_frame (cur_block, stop_chain)) + break; + + skip_count++; + last_sym = BLOCK_FUNCTION (cur_block); } else break; diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c index 922102debb6..f64a81af939 100644 --- a/gdb/testsuite/gdb.opt/inline-break.c +++ b/gdb/testsuite/gdb.opt/inline-break.c @@ -176,6 +176,38 @@ not_inline_func3 (int x) return y + inline_func3 (x); } +/* The following three functions serve to exercise GDB's inline frame + skipping logic when setting a user breakpoint on an inline function + by name. */ + +/* A static inlined function that is called by another static inlined + function. */ + +static inline ATTR int +func_inline_callee (int x) +{ + return x * 23; +} + +/* A static inlined function that calls another static inlined + function. The body of the function is as simple as possible so + that both functions are inlined to the same PC address. */ + +static inline ATTR int +func_inline_caller (int x) +{ + return func_inline_callee (x); +} + +/* An extern not-inline function that calls a static inlined + function. */ + +int +func_extern_caller (int x) +{ + return func_inline_caller (x); +} + /* Entry point. */ int @@ -205,5 +237,7 @@ main (int argc, char *argv[]) x = not_inline_func3 (-21); + func_extern_caller (1); + return x; } diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp index 008ff1ac33a..bae76254905 100644 --- a/gdb/testsuite/gdb.opt/inline-break.exp +++ b/gdb/testsuite/gdb.opt/inline-break.exp @@ -231,4 +231,29 @@ foreach_with_prefix cmd [list "break" "tbreak"] { } } +# func_extern_caller calls func_inline_caller which calls +# func_inline_callee. The latter two are both inline functions. Test +# that setting a breakpoint on each of the functions reports a stop at +# that function. This exercises the inline frame skipping logic. If +# we set a breakpoint at function A, we want to present the stop at A, +# even if A's entry code is an inlined call to another inline function +# B. + +foreach_with_prefix func { + "func_extern_caller" + "func_inline_caller" + "func_inline_callee" +} { + clean_restart $binfile + + if {![runto main]} { + untested "could not run to main" + continue + } + + gdb_breakpoint $func + gdb_test "continue" "Breakpoint .* $func .*at .*$srcfile.*" \ + "breakpoint hit presents stop at breakpointed function" +} + unset -nocomplain results -- 2.14.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) 2018-06-19 16:36 ` [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) Pedro Alves @ 2018-06-25 21:04 ` Joel Brobecker 2018-06-26 19:02 ` Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Joel Brobecker @ 2018-06-25 21:04 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom de Vries, gdb-patches, Keith Seitz Hello, > gdb/ChangeLog: > 2018-06-19 Pedro Alves <palves@redhat.com> > > * inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC > parameter with a block parameter. Compare location's block symbol > with the frame's block instead of addresses. > (skip_inline_frames): Pass the current block instead of the > frame's address. Break out as soon as we determine the frame > should not be skipped. > > gdb/testsuite/ChangeLog: > 2018-06-19 Pedro Alves <palves@redhat.com> > > * gdb.opt/inline-break.c (func_inline_callee, func_inline_caller) > (func_extern_caller): New. > (main): Call func_extern_caller. > * gdb.opt/inline-break.exp: Add tests for inline frame skipping > logic change. it looks like this patch is causing a crash with the following example program: $ cat -n r.h 1 /* r.h */ 2 int counter = 42; 3 4 inline void 5 callee () { 6 counter = 0; /* break here */ 7 } $ cat -n r.c 1 /* r.c */ 2 #include "r.h" 3 4 int 5 main () 6 { 7 callee (); 8 } I compiled it using the following commands: $ gcc -c -g -O2 r.c $ gcc -o r r.o Then, trying to put a breakpoint on r.h:6 (inside "callee") causes a SEGV for me: $ gdb -q r Reading symbols from r...done. (gdb) b r.h:6 Breakpoint 1 at 0x4003c0: file r.h, line 6. (gdb) run Starting program: /[...]/r [1] 75618 segmentation fault /[...]/gdb -q r Prior to this commit, the behavior is the following for the "run" command: (gdb) run Starting program: /[...]/r Breakpoint 1, callee () at r.h:6 6 counter = 0; /* break here */ The problem occurs because we apparently assume that a bp_location's symbols is not NULL: Program received signal SIGSEGV, Segmentation fault. 0x00000000006f42bb in stopped_by_user_bp_inline_frame ( stop_chain=<optimized out>, frame_block=<optimized out>) at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305 305 && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) (gdb) p loc->symbol $1 = (const symbol *) 0x0 I don't know yet whether that's a valid assumption or something occurred earlier in the process. Any thoughts on this before I start looking deeper? I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to reproduce. -- Joel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) 2018-06-25 21:04 ` Joel Brobecker @ 2018-06-26 19:02 ` Pedro Alves 2018-06-26 22:02 ` Joel Brobecker 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2018-06-26 19:02 UTC (permalink / raw) To: Joel Brobecker; +Cc: Tom de Vries, gdb-patches, Keith Seitz On 06/25/2018 10:04 PM, Joel Brobecker wrote: > Hello, > >> gdb/ChangeLog: >> 2018-06-19 Pedro Alves <palves@redhat.com> >> >> * inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC >> parameter with a block parameter. Compare location's block symbol >> with the frame's block instead of addresses. >> (skip_inline_frames): Pass the current block instead of the >> frame's address. Break out as soon as we determine the frame >> should not be skipped. >> >> gdb/testsuite/ChangeLog: >> 2018-06-19 Pedro Alves <palves@redhat.com> >> >> * gdb.opt/inline-break.c (func_inline_callee, func_inline_caller) >> (func_extern_caller): New. >> (main): Call func_extern_caller. >> * gdb.opt/inline-break.exp: Add tests for inline frame skipping >> logic change. > > it looks like this patch is causing a crash with the following > example program: > > $ cat -n r.h > 1 /* r.h */ > 2 int counter = 42; > 3 > 4 inline void > 5 callee () { > 6 counter = 0; /* break here */ > 7 } > $ cat -n r.c > 1 /* r.c */ > 2 #include "r.h" > 3 > 4 int > 5 main () > 6 { > 7 callee (); > 8 } > > I compiled it using the following commands: > > $ gcc -c -g -O2 r.c > $ gcc -o r r.o > > Then, trying to put a breakpoint on r.h:6 (inside "callee") causes > a SEGV for me: > > $ gdb -q r > Reading symbols from r...done. > (gdb) b r.h:6 > Breakpoint 1 at 0x4003c0: file r.h, line 6. > (gdb) run > Starting program: /[...]/r > [1] 75618 segmentation fault /[...]/gdb -q r > > Prior to this commit, the behavior is the following for the "run" > command: > > (gdb) run > Starting program: /[...]/r > > Breakpoint 1, callee () at r.h:6 > 6 counter = 0; /* break here */ > > The problem occurs because we apparently assume that a bp_location's > symbols is not NULL: > > Program received signal SIGSEGV, Segmentation fault. > 0x00000000006f42bb in stopped_by_user_bp_inline_frame ( > stop_chain=<optimized out>, frame_block=<optimized out>) > at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305 > 305 && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) > (gdb) p loc->symbol > $1 = (const symbol *) 0x0 Whoops. I remember actually thinking about loc->symbol potentially being null, but somehow convinced myself that SYMBOL_BLOCK_VALUE would return null in that case... :-P > > I don't know yet whether that's a valid assumption or something > occurred earlier in the process. Any thoughts on this before I start > looking deeper? > > I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to > reproduce. If we just add a "loc->symbol != NULL" check, then we end up presenting the stop at the caller of the inline function, where the inline function was inlined, which is not what we want, since that's not where the user set the breakpoint. Recording the symbol in the location (it is copied from the sal that linespec.c creates into the location by add_location_to_breakpoint), like in the patch below, makes gdb present the stop at the "break here" line successfully. I tried to come up with a more complicated testcase that included more nested blocks inside the inlined function, to see if we would need to record the inner inlined block in the sal instead of the function's symbol (does that actually make sense for inlined functions?), but all I came up with worked with this patch as is. So maybe we can defer thinking about that until we find a testcase? From ac746927c3a8078098729dc3256010b2c1e617f8 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Tue, 26 Jun 2018 19:14:41 +0100 Subject: [PATCH] inline --- gdb/inline-frame.c | 1 + gdb/linespec.c | 1 + 2 files changed, 2 insertions(+) diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 896b0004e4a..3d07f8d0970 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -302,6 +302,7 @@ stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain) if ((t == bp_loc_software_breakpoint || t == bp_loc_hardware_breakpoint) + && loc->symbol != nullptr && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) return true; } diff --git a/gdb/linespec.c b/gdb/linespec.c index ae0200b8133..93e66c389f7 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -2196,6 +2196,7 @@ create_sals_line_offset (struct linespec_state *self, if (self->funfirstline) skip_prologue_sal (&intermediate_results[i]); + intermediate_results[i].symbol = sym; add_sal_to_sals (self, &values, &intermediate_results[i], sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0); } -- 2.14.4 Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) 2018-06-26 19:02 ` Pedro Alves @ 2018-06-26 22:02 ` Joel Brobecker 2018-06-27 16:28 ` Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Joel Brobecker @ 2018-06-26 22:02 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom de Vries, gdb-patches, Keith Seitz Hi Pedro, > > Program received signal SIGSEGV, Segmentation fault. > > 0x00000000006f42bb in stopped_by_user_bp_inline_frame ( > > stop_chain=<optimized out>, frame_block=<optimized out>) > > at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305 > > 305 && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) > > (gdb) p loc->symbol > > $1 = (const symbol *) 0x0 > > Whoops. I remember actually thinking about loc->symbol potentially > being null, but somehow convinced myself that SYMBOL_BLOCK_VALUE > would return null in that case... :-P > > > > > I don't know yet whether that's a valid assumption or something > > occurred earlier in the process. Any thoughts on this before I start > > looking deeper? > > > > I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to > > reproduce. > > If we just add a "loc->symbol != NULL" check, then we end up > presenting the stop at the caller of the inline function, where > the inline function was inlined, which is not what we want, since that's > not where the user set the breakpoint. > > Recording the symbol in the location (it is copied from the sal > that linespec.c creates into the location by > add_location_to_breakpoint), like in the patch below, makes gdb present > the stop at the "break here" line successfully. > > I tried to come up with a more complicated testcase that included > more nested blocks inside the inlined function, to see if we would > need to record the inner inlined block in the sal instead of the > function's symbol (does that actually make sense for inlined functions?), > but all I came up with worked with this patch as is. So maybe we > can defer thinking about that until we find a testcase? Just a quick message that the patch makes sense to me, and that I was just able to run it through AdaCore's testsuite with succes. Or, I should qualify that - there is one tiny change that I haven't had time to analyze, but from the surface, it is exactly what you explained about why you need the second hunk. I haven't had a chance to run it through the official testsuite, however, as I have to go ... I am so laaaaate! I can do that tomorrow, or if you prefer to just finish the patch up and push it, it'd be perfect. I think the patch is good. Thanks again! > > >From ac746927c3a8078098729dc3256010b2c1e617f8 Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Tue, 26 Jun 2018 19:14:41 +0100 > Subject: [PATCH] inline > > --- > gdb/inline-frame.c | 1 + > gdb/linespec.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c > index 896b0004e4a..3d07f8d0970 100644 > --- a/gdb/inline-frame.c > +++ b/gdb/inline-frame.c > @@ -302,6 +302,7 @@ stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain) > > if ((t == bp_loc_software_breakpoint > || t == bp_loc_hardware_breakpoint) > + && loc->symbol != nullptr > && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) > return true; > } > diff --git a/gdb/linespec.c b/gdb/linespec.c > index ae0200b8133..93e66c389f7 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -2196,6 +2196,7 @@ create_sals_line_offset (struct linespec_state *self, > > if (self->funfirstline) > skip_prologue_sal (&intermediate_results[i]); > + intermediate_results[i].symbol = sym; > add_sal_to_sals (self, &values, &intermediate_results[i], > sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0); > } > -- > 2.14.4 > > Thanks, > Pedro Alves -- Joel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) 2018-06-26 22:02 ` Joel Brobecker @ 2018-06-27 16:28 ` Pedro Alves 2018-06-28 14:48 ` Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2018-06-27 16:28 UTC (permalink / raw) To: Joel Brobecker; +Cc: Tom de Vries, gdb-patches, Keith Seitz Hi Joel, On 06/26/2018 11:02 PM, Joel Brobecker wrote: > Just a quick message that the patch makes sense to me, and that > I was just able to run it through AdaCore's testsuite with succes. > Or, I should qualify that - there is one tiny change that I haven't > had time to analyze, but from the surface, it is exactly what you > explained about why you need the second hunk. > > I haven't had a chance to run it through the official testsuite, > however, as I have to go ... I am so laaaaate! > > I can do that tomorrow, or if you prefer to just finish the patch > up and push it, it'd be perfect. I think the patch is good. > > Thanks again! FYI, I'm starting to look at this now. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) 2018-06-27 16:28 ` Pedro Alves @ 2018-06-28 14:48 ` Pedro Alves 2018-06-28 14:50 ` [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output Pedro Alves 2018-06-28 14:50 ` [PATCH 1/2] Fix running to breakpoint set in inline function by lineno/address Pedro Alves 0 siblings, 2 replies; 14+ messages in thread From: Pedro Alves @ 2018-06-28 14:48 UTC (permalink / raw) To: Joel Brobecker; +Cc: Tom de Vries, gdb-patches, Keith Seitz On 06/27/2018 05:28 PM, Pedro Alves wrote: > Hi Joel, > > On 06/26/2018 11:02 PM, Joel Brobecker wrote: > >> Just a quick message that the patch makes sense to me, and that >> I was just able to run it through AdaCore's testsuite with succes. >> Or, I should qualify that - there is one tiny change that I haven't >> had time to analyze, but from the surface, it is exactly what you >> explained about why you need the second hunk. >> >> I haven't had a chance to run it through the official testsuite, >> however, as I have to go ... I am so laaaaate! >> >> I can do that tomorrow, or if you prefer to just finish the patch >> up and push it, it'd be perfect. I think the patch is good. >> >> Thanks again! > > FYI, I'm starting to look at this now. So while poking some more at this, noticed that setting a breakpoint by address crashes in the same way, like "b *ADDRESS". So I thought that maybe it would be better to make stopped_by_user_bp_inline_frame return true if the location has no symbol instead of returning false like in the version I sent before. That preserves the previous behavior of showing the stop at the inline function if we miss setting the sal's symbol somewhere. However, playing with that made me notice something else unrelated to my "Change inline frame breakpoint skipping" patch: (gdb) b *0x40062f Breakpoint 2 at 0x40062f: file inline-break.c, line 32. (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x000000000040062f in main at inline-break.c:32 (gdb) r .... Breakpoint 2, func1 (x=1) at inline-break.c:32 32 return x * 23; /* break here */ Notice that above "info break" says "in main": in main at inline-break.c:32 ^^^^ Since we say "inline-break.c:32" everywhere, and present the stop at the inline function, I think that "info break" should say instead: in func1 at inline-break.c:32 ^^^^^ Fixing that ends up going back to setting the symbol in the sal again, but I decided to do that in a separate patch, and still make "loc->symbol == nullptr" in stopped_by_user_bp_inline_frame return true, unlike the previous version of the patch. I'll be sending two patches in response to this email. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output 2018-06-28 14:48 ` Pedro Alves @ 2018-06-28 14:50 ` Pedro Alves 2018-06-28 17:42 ` Joel Brobecker 2018-06-28 14:50 ` [PATCH 1/2] Fix running to breakpoint set in inline function by lineno/address Pedro Alves 1 sibling, 1 reply; 14+ messages in thread From: Pedro Alves @ 2018-06-28 14:50 UTC (permalink / raw) To: gdb-patches While experimenting with the previous patch, I noticed this inconsistency in GDB's output: (gdb) b 32 Breakpoint 1 at 0x40062f: file inline-break.c, line 32. (1) (gdb) r .... Breakpoint 1, func1 (x=1) at inline-break.c:32 (2) 32 return x * 23; /* break here */ (gdb) info breakpoints Num Type Disp Enb Address What 1 breakpoint keep y 0x40062f in main at inline-break.c:32 (3) breakpoint already hit 1 time (gdb) Notice that when the breakpoint as set, GDB showed "inline-break.c, line 32" (1), the same line number that was specified in the command. When we run to the breakpoint, we present the stop at the same line number, and correctly show "func1" as the function name (2). But in "info break" output (3), notice that we say "in main", not "in func1". The same thing happens if you set a breakpoint by address. I.e.: (gdb) b *0x40062f Breakpoint 2 at 0x40062f: file inline-break.c, line 32. (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x000000000040062f in main at inline-break.c:32 (gdb) r .... Breakpoint 2, func1 (x=1) at inline-break.c:32 32 return x * 23; /* break here */ The problem is that the breakpoints were set at an inline function, but when we set such a breakpoint by line number or address, we don't record the functions symbol in the sal, and as consequence the breakpoint location does not have an associated symbol either. Then, in print_breakpoint_location, if the location does not have a symbol, we call find_pc_sect_function to find one, and this is what finds "main", because find_pc_sect_function uses block_linkage_function: /* Return the symbol for the function which contains a specified lexical block, described by a struct block BL. The return value will not be an inlined function; the containing function will be returned instead. */ struct symbol * block_linkage_function (const struct block *bl) To fix this, this commit adds an alternative to find_pc_sect_function that uses block_containing_function instead: /* Return the symbol for the function which contains a specified block, described by a struct block BL. The return value will be the closest enclosing function, which might be an inline function. */ struct symbol * block_containing_function (const struct block *bl) (It seems odd to me that block_linkage_function says "the CONTAINING function will be returned", and then block_containing_function says it returns "the closest enclosing function". Something seems reversed here. Still, I've kept the same nomenclature and copied the comments, so that at least there's consistency. Maybe we should fix that up somehow.) Then I wondered, why make print_breakpoint_location look up the symbol every time it is called, instead of just always storing the symbol when the location is created, since the location already stores the symbol in some cases. So to find which cases might be missing setting the symbol in the sal which is used to create the breakpoint location, I added an assertion to print_breakpoint_location, and ran the testsuite. That caught a few places, unsurprisingly: - setting a breakpoint by line number - setting a breapoint by address - ifunc resolving Those are all fixed by this commit. I decided not to add the assertion to block_linkage_function and leave the existing "if (sym)" check in place, because it's plausible that we have symtabs with line info but no symbols. I.e., that would not be a GDB bug, but a peculiarity of debug info input. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * blockframe.c (find_pc_sect_containing_function): New function. * breakpoint.c (print_breakpoint_location): Don't call find_pc_sect_function. * linespec.c (create_sals_line_offset): Record the location's symbol in the sal. * linespec.c (convert_address_location_to_sals): Fill in sal's symbol with find_pc_sect_containing_function. * symtab.c (find_function_start_sal): Rename to ... (find_function_start_sal_1): ... this. (find_function_start_sal): Reimplement as wrapper around find_function_start_sal_1, and use find_pc_sect_containing_function to fill in the sal's symbol. (find_function_start_sal(symbol*, bool)): Adjust. * symtab.h (find_pc_function, find_pc_sect_function): Adjust comments. (find_pc_sect_containing_function): Declare. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * gdb.opt/inline-break.exp (line number, address): Add "info break" tests. --- gdb/blockframe.c | 13 +++++++++++++ gdb/breakpoint.c | 3 --- gdb/linespec.c | 2 ++ gdb/symtab.c | 32 +++++++++++++++++++++++++------- gdb/symtab.h | 15 +++++++++++++-- gdb/testsuite/gdb.opt/inline-break.exp | 6 ++++++ 6 files changed, 59 insertions(+), 12 deletions(-) diff --git a/gdb/blockframe.c b/gdb/blockframe.c index b3c9aa3bd73..6a018ccaefe 100644 --- a/gdb/blockframe.c +++ b/gdb/blockframe.c @@ -152,6 +152,19 @@ find_pc_function (CORE_ADDR pc) return find_pc_sect_function (pc, find_pc_mapped_section (pc)); } +/* See symtab.h. */ + +struct symbol * +find_pc_sect_containing_function (CORE_ADDR pc, struct obj_section *section) +{ + const block *bl = block_for_pc_sect (pc, section); + + if (bl == nullptr) + return nullptr; + + return block_containing_function (bl); +} + /* These variables are used to cache the most recent result of find_pc_partial_function. */ diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 80df193a013..82dec7d418f 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5867,9 +5867,6 @@ print_breakpoint_location (struct breakpoint *b, { const struct symbol *sym = loc->symbol; - if (sym == NULL) - sym = find_pc_sect_function (loc->address, loc->section); - if (sym) { uiout->text ("in "); diff --git a/gdb/linespec.c b/gdb/linespec.c index ae0200b8133..2a4189278ef 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -2196,6 +2196,7 @@ create_sals_line_offset (struct linespec_state *self, if (self->funfirstline) skip_prologue_sal (&intermediate_results[i]); + intermediate_results[i].symbol = sym; add_sal_to_sals (self, &values, &intermediate_results[i], sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0); } @@ -2224,6 +2225,7 @@ convert_address_location_to_sals (struct linespec_state *self, sal.pc = address; sal.section = find_pc_overlay (address); sal.explicit_pc = 1; + sal.symbol = find_pc_sect_containing_function (sal.pc, sal.section); std::vector<symtab_and_line> sals; add_sal_to_sals (self, &sals, &sal, core_addr_to_string (address), 1); diff --git a/gdb/symtab.c b/gdb/symtab.c index 3e594e76c92..d8a7a16e073 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -3575,11 +3575,12 @@ find_pc_line_pc_range (CORE_ADDR pc, CORE_ADDR *startptr, CORE_ADDR *endptr) return sal.symtab != 0; } -/* See symtab.h. */ +/* Helper for find_function_start_sal. Does most of the work, except + setting the sal's symbol. */ -symtab_and_line -find_function_start_sal (CORE_ADDR func_addr, obj_section *section, - bool funfirstline) +static symtab_and_line +find_function_start_sal_1 (CORE_ADDR func_addr, obj_section *section, + bool funfirstline) { symtab_and_line sal = find_pc_sect_line (func_addr, section, 0); @@ -3615,14 +3616,31 @@ find_function_start_sal (CORE_ADDR func_addr, obj_section *section, /* See symtab.h. */ +symtab_and_line +find_function_start_sal (CORE_ADDR func_addr, obj_section *section, + bool funfirstline) +{ + symtab_and_line sal + = find_function_start_sal_1 (func_addr, section, funfirstline); + + /* find_function_start_sal_1 does a linetable search, so it finds + the symtab and linenumber, but not a symbol. Fill in the + function symbol too. */ + sal.symbol = find_pc_sect_containing_function (sal.pc, sal.section); + + return sal; +} + +/* See symtab.h. */ + symtab_and_line find_function_start_sal (symbol *sym, bool funfirstline) { fixup_symbol_section (sym, NULL); symtab_and_line sal - = find_function_start_sal (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)), - SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym), - funfirstline); + = find_function_start_sal_1 (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)), + SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym), + funfirstline); sal.symbol = sym; return sal; } diff --git a/gdb/symtab.h b/gdb/symtab.h index 84fc8976582..0b155d06592 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -1670,14 +1670,25 @@ extern struct type *lookup_enum (const char *, const struct block *); /* from blockframe.c: */ -/* lookup the function symbol corresponding to the address. */ +/* lookup the function symbol corresponding to the address. The + return value will not be an inlined function; the containing + function will be returned instead. */ extern struct symbol *find_pc_function (CORE_ADDR); -/* lookup the function corresponding to the address and section. */ +/* lookup the function corresponding to the address and section. The + return value will not be an inlined function; the containing + function will be returned instead. */ extern struct symbol *find_pc_sect_function (CORE_ADDR, struct obj_section *); +/* lookup the function symbol corresponding to the address and + section. The return value will be the closest enclosing function, + which might be an inline function. */ + +extern struct symbol *find_pc_sect_containing_function + (CORE_ADDR pc, struct obj_section *section); + /* Find the symbol at the given address. Returns NULL if no symbol found. Only exact matches for ADDRESS are considered. */ diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp index 68b7b89ef31..183a1b927c6 100644 --- a/gdb/testsuite/gdb.opt/inline-break.exp +++ b/gdb/testsuite/gdb.opt/inline-break.exp @@ -272,6 +272,9 @@ with_test_prefix "line number" { # Set the breakpoint by line number, and check that GDB reports # the breakpoint location being the inline function. gdb_breakpoint "$srcfile:$line" + + gdb_test "info break \$bpnum" "in func1 at .*$srcfile:$line" + gdb_test "continue" "Breakpoint .*, func1 \\(x=1\\) at .*$srcfile:$line.*break here.*" \ "breakpoint hit presents stop at inlined function" @@ -294,6 +297,9 @@ with_test_prefix "address" { # Set the breakpoint by address, and check that GDB reports the # breakpoint location being the inline function. gdb_test "break *$address" ".*Breakpoint .* at $address: file .*$srcfile, line $line." + + gdb_test "info break \$bpnum" "in func1 at .*$srcfile:$line" + gdb_test "continue" "Breakpoint .*, func1 \\(x=1\\) at .*$srcfile:$line.*break here.*" \ "breakpoint hit presents stop at inlined function" } -- 2.14.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output 2018-06-28 14:50 ` [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output Pedro Alves @ 2018-06-28 17:42 ` Joel Brobecker 2018-06-29 18:43 ` Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Joel Brobecker @ 2018-06-28 17:42 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Hi Pedro, > While experimenting with the previous patch, I noticed this inconsistency > in GDB's output: [...] > When we run to the breakpoint, we present the stop at the same line > number, and correctly show "func1" as the function name (2). > > But in "info break" output (3), notice that we say "in main", not "in > func1". > (It seems odd to me that block_linkage_function says "the CONTAINING > function will be returned", and then block_containing_function says it > returns "the closest enclosing function". Something seems reversed > here. Still, I've kept the same nomenclature and copied the comments, > so that at least there's consistency. Maybe we should fix that up > somehow.) That seems opposite to me as well... > Then I wondered, why make print_breakpoint_location look up the symbol > every time it is called, instead of just always storing the symbol > when the location is created, since the location already stores the > symbol in some cases. Agreed. > So to find which cases might be missing setting > the symbol in the sal which is used to create the breakpoint location, > I added an assertion to print_breakpoint_location, and ran the > testsuite. That caught a few places, unsurprisingly: > > - setting a breakpoint by line number > - setting a breapoint by address > - ifunc resolving > > Those are all fixed by this commit. Nice approach! > I decided not to add the > assertion to block_linkage_function and leave the existing "if (sym)" > check in place, because it's plausible that we have symtabs with line > info but no symbols. I.e., that would not be a GDB bug, but > a peculiarity of debug info input. Agreed as well. > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * blockframe.c (find_pc_sect_containing_function): New function. > * breakpoint.c (print_breakpoint_location): Don't call > find_pc_sect_function. > * linespec.c (create_sals_line_offset): Record the location's > symbol in the sal. > * linespec.c (convert_address_location_to_sals): Fill in sal's > symbol with find_pc_sect_containing_function. > * symtab.c (find_function_start_sal): Rename to ... > (find_function_start_sal_1): ... this. > (find_function_start_sal): Reimplement as wrapper around > find_function_start_sal_1, and use > find_pc_sect_containing_function to fill in the sal's symbol. > (find_function_start_sal(symbol*, bool)): Adjust. > * symtab.h (find_pc_function, find_pc_sect_function): Adjust > comments. > (find_pc_sect_containing_function): Declare. I know it might be considered a small and trivial part, but I really appreciate the attention to the function's comment description. > > gdb/testsuite/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * gdb.opt/inline-break.exp (line number, address): Add "info > break" tests. I went through the patch and it looks good. Thanks again! -- Joel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output 2018-06-28 17:42 ` Joel Brobecker @ 2018-06-29 18:43 ` Pedro Alves 0 siblings, 0 replies; 14+ messages in thread From: Pedro Alves @ 2018-06-29 18:43 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches On 06/28/2018 06:42 PM, Joel Brobecker wrote: >> gdb/testsuite/ChangeLog: >> yyyy-mm-dd Pedro Alves <palves@redhat.com> >> >> * gdb.opt/inline-break.exp (line number, address): Add "info >> break" tests. > > I went through the patch and it looks good. Great, thanks. I pushed in both patches now. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] Fix running to breakpoint set in inline function by lineno/address 2018-06-28 14:48 ` Pedro Alves 2018-06-28 14:50 ` [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output Pedro Alves @ 2018-06-28 14:50 ` Pedro Alves 2018-06-28 17:32 ` Joel Brobecker 1 sibling, 1 reply; 14+ messages in thread From: Pedro Alves @ 2018-06-28 14:50 UTC (permalink / raw) To: gdb-patches Commit 61b04dd04ac2 ("Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp)") caused a GDB crash when you set a breakpoint by line number in an inline function, and then run to the breakpoint: $ gdb -q test Reading symbols from test...done. (gdb) b inline-break.c:32 Breakpoint 1 at 0x40062f: file inline-break.c, line 32. (gdb) run Starting program: /[...]/test [1] 75618 segmentation fault /[...]/gdb -q test The problem occurs because we assume that a bp_location's symbol is not NULL, which is not true when we set the breakpoint with a linespec location: Program received signal SIGSEGV, Segmentation fault. 0x00000000006f42bb in stopped_by_user_bp_inline_frame ( stop_chain=<optimized out>, frame_block=<optimized out>) at gdb/inline-frame.c:305 305 && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) (gdb) p loc->symbol $1 = (const symbol *) 0x0 The same thing happens if you run to a breakpoint set in an inline function by address: (gdb) b *0x40062f Breakpoint 3 at 0x40062f: file inline-break.c, line 32. To fix this, add a null pointer check, to avoid the crash, and make it so that if there's not symbol for the location, then we present the stop at the inline function. This preserves the previous behavior when e.g., setting a breakpoint by address, with "b *ADDRESS". gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * inline-frame.c (stopped_by_user_bp_inline_frame): Return true if the the location has no symbol. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * gdb.opt/inline-break.c (func1): Add "break here" marker. * gdb.opt/inline-break.exp: Test setting breakpoints by line number and address and running to them. --- gdb/inline-frame.c | 16 +++++++++---- gdb/testsuite/gdb.opt/inline-break.c | 2 +- gdb/testsuite/gdb.opt/inline-break.exp | 42 ++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 896b0004e4a..c6caf9d0c6e 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -300,10 +300,18 @@ stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain) bp_location *loc = s->bp_location_at; enum bp_loc_type t = loc->loc_type; - if ((t == bp_loc_software_breakpoint - || t == bp_loc_hardware_breakpoint) - && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) - return true; + if (t == bp_loc_software_breakpoint + || t == bp_loc_hardware_breakpoint) + { + /* If the location has a function symbol, check whether + the frame was for that inlined function. If it has + no function symbol, then assume it is. I.e., default + to presenting the stop at the innermost inline + function. */ + if (loc->symbol == nullptr + || frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) + return true; + } } } diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c index f64a81af939..d4779299fa2 100644 --- a/gdb/testsuite/gdb.opt/inline-break.c +++ b/gdb/testsuite/gdb.opt/inline-break.c @@ -29,7 +29,7 @@ static inline ATTR int func1 (int x) { - return x * 23; + return x * 23; /* break here */ } /* A non-static inlined function that is called once. */ diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp index bae76254905..68b7b89ef31 100644 --- a/gdb/testsuite/gdb.opt/inline-break.exp +++ b/gdb/testsuite/gdb.opt/inline-break.exp @@ -256,4 +256,46 @@ foreach_with_prefix func { "breakpoint hit presents stop at breakpointed function" } +# Test setting a breakpoint in an inline function by line number and +# by address, and that GDB presents the stop there. + +set line [gdb_get_line_number "break here"] + +with_test_prefix "line number" { + clean_restart $binfile + + if {![runto main]} { + untested "could not run to main" + continue + } + + # Set the breakpoint by line number, and check that GDB reports + # the breakpoint location being the inline function. + gdb_breakpoint "$srcfile:$line" + gdb_test "continue" "Breakpoint .*, func1 \\(x=1\\) at .*$srcfile:$line.*break here.*" \ + "breakpoint hit presents stop at inlined function" + + # Save the PC for the following by-address test. + set address [get_hexadecimal_valueof "\$pc" "0"] +} + +# Test setting a breakpoint in an inline function by address, and that +# GDB presents the stop there. + +with_test_prefix "address" { + + clean_restart $binfile + + if {![runto main]} { + untested "could not run to main" + continue + } + + # Set the breakpoint by address, and check that GDB reports the + # breakpoint location being the inline function. + gdb_test "break *$address" ".*Breakpoint .* at $address: file .*$srcfile, line $line." + gdb_test "continue" "Breakpoint .*, func1 \\(x=1\\) at .*$srcfile:$line.*break here.*" \ + "breakpoint hit presents stop at inlined function" +} + unset -nocomplain results -- 2.14.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] Fix running to breakpoint set in inline function by lineno/address 2018-06-28 14:50 ` [PATCH 1/2] Fix running to breakpoint set in inline function by lineno/address Pedro Alves @ 2018-06-28 17:32 ` Joel Brobecker 0 siblings, 0 replies; 14+ messages in thread From: Joel Brobecker @ 2018-06-28 17:32 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > Commit 61b04dd04ac2 ("Change inline frame breakpoint skipping logic > (fix gdb.gdb/selftest.exp)") caused a GDB crash when you set a > breakpoint by line number in an inline function, and then run to the > breakpoint: > > $ gdb -q test Reading symbols from test...done. > (gdb) b inline-break.c:32 > Breakpoint 1 at 0x40062f: file inline-break.c, line 32. > (gdb) run > Starting program: /[...]/test > [1] 75618 segmentation fault /[...]/gdb -q test > > The problem occurs because we assume that a bp_location's symbol is > not NULL, which is not true when we set the breakpoint with a linespec > location: > > Program received signal SIGSEGV, Segmentation fault. > 0x00000000006f42bb in stopped_by_user_bp_inline_frame ( > stop_chain=<optimized out>, frame_block=<optimized out>) > at gdb/inline-frame.c:305 > 305 && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol)) > (gdb) p loc->symbol > $1 = (const symbol *) 0x0 > > The same thing happens if you run to a breakpoint set in an inline > function by address: > > (gdb) b *0x40062f > Breakpoint 3 at 0x40062f: file inline-break.c, line 32. > > To fix this, add a null pointer check, to avoid the crash, and make it > so that if there's not symbol for the location, then we present the > stop at the inline function. This preserves the previous behavior > when e.g., setting a breakpoint by address, with "b *ADDRESS". > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * inline-frame.c (stopped_by_user_bp_inline_frame): Return > true if the the location has no symbol. > > gdb/testsuite/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * gdb.opt/inline-break.c (func1): Add "break here" marker. > * gdb.opt/inline-break.exp: Test setting breakpoints by line > number and address and running to them. Thanks, Pedro. I had a look, and FWIW, the patch looks good to me. -- Joel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-06-29 18:43 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-12 15:06 [PATCH] Ensure captured_main has unique address Tom de Vries 2018-06-12 17:38 ` Change inline frame breakpoint skipping logic (Re: [PATCH] Ensure captured_main has unique address) Pedro Alves 2018-06-14 13:22 ` Tom de Vries 2018-06-19 16:36 ` [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp) Pedro Alves 2018-06-25 21:04 ` Joel Brobecker 2018-06-26 19:02 ` Pedro Alves 2018-06-26 22:02 ` Joel Brobecker 2018-06-27 16:28 ` Pedro Alves 2018-06-28 14:48 ` Pedro Alves 2018-06-28 14:50 ` [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output Pedro Alves 2018-06-28 17:42 ` Joel Brobecker 2018-06-29 18:43 ` Pedro Alves 2018-06-28 14:50 ` [PATCH 1/2] Fix running to breakpoint set in inline function by lineno/address Pedro Alves 2018-06-28 17:32 ` Joel Brobecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).