From patchwork Tue Jul 11 02:36:41 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Seitz X-Patchwork-Id: 21535 Received: (qmail 78224 invoked by alias); 11 Jul 2017 02:36:52 -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 78176 invoked by uid 89); 11 Jul 2017 02:36:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Forget, concurrently, keith, bsp 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; Tue, 11 Jul 2017 02:36:43 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E6C7D80F63 for ; Tue, 11 Jul 2017 02:36:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E6C7D80F63 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=keiths@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com E6C7D80F63 Received: from valrhona.Home (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id BC0325C54F for ; Tue, 11 Jul 2017 02:36:41 +0000 (UTC) From: Keith Seitz To: gdb-patches@sourceware.org Subject: [PATCH 2/2] Report stop locations in inlined functions. Date: Mon, 10 Jul 2017 19:36:41 -0700 Message-Id: <1499740601-15957-2-git-send-email-keiths@redhat.com> In-Reply-To: <1499740601-15957-1-git-send-email-keiths@redhat.com> References: <1499740601-15957-1-git-send-email-keiths@redhat.com> X-IsSubscribed: yes This is a patch for a very related inline function problem. Using the test case from breakpoints/17534, 3 static inline void NVIC_EnableIRQ(int IRQn) 4 { 5 volatile int y; 6 y = IRQn; 7 } 8 9 __attribute__( ( always_inline ) ) static inline void __WFI(void) 10 { 11 __asm volatile ("nop"); 12 } 13 14 int main(void) { 15 16 x= 42; 17 18 if (x) 19 NVIC_EnableIRQ(16); 20 else 21 NVIC_EnableIRQ(18); (gdb) b NVIC_EnableIRQ Breakpoint 1 at 0x4003e4: NVIC_EnableIRQ. (2 locations) (gdb) r Starting program: 17534 Breakpoint 1, main () at 17534.c:19 19 NVIC_EnableIRQ(16); This happens because skip_inline_frames automatically skips every inlined frame. Based on a suggestion by Jan, this patch introduces a new function, breakpoint_for_stop, which attempts to ascertain which breakpoint, if any, caused a particular stop in the inferior. That breakpoint is then passed to skip_inline_frames so that it can decide if a particular inlined frame should be skipped. I've had to separate the bpstat chain building from bpstat_stop_status -- py-finish-breakpoint.exp did not like me calling bpstat_stop_status multiple times. So I've added the ability to allocate the chain separately and optionally pass it to bpstat_stop_status, which remains otherwise unchanged. With this patch, GDB now correctly reports that the inferior has stopped inside the inlined function: (gdb) r Starting program: 17534 Breakpoint 1, NVIC_EnableIRQ (IRQn=16) at 17534.c:6 6 y = IRQn; I don't quite like this, though. This solution involves calling decode_line_full, and that is really expensive, so I would be grateful if maintaienrs could offer advice on how to better tackle this. gdb/ChangeLog: 2017-MM-DD Keith Seitz * breakpoint.c (bpstat_explains_signal): Add output parameter for breakpoint and save the breakpoint if one is found to explain the signal. All callers updated. (build_bpstat_chain): New function, moved from bpstat_stop_status. (breakpoint_for_stop): New function. (bpstat_stop_status): Add new optional parameter for the bpstat chain. If this new parameter is NULL, call build_bpstat_chain. All callers updated. * breakpoint.h (breakpoint_for_stop): Declare. (bpstat_explains_signal): Update declaration. * infrun.c (handle_signal_stop): Before calling skip_inline_frames, use breakpoint_for_stop to find the breakpoint that caused us to stop. Save the bpstat chain for later invocation of bpstat_stop_status. * inline-frame.c: Include linespec.h. (skip_inline_frames): Add struct breakpoint parameter. Re-parse the location of the breakpoint causing the stop, if any, and only skip frames that did not cause the stop. * inline-frame.h (skip_inline_frames): Update declaration. gdb/testsuite/ChangeLog: 2017-MM-DD Keith Seitz * gdb.opt/inline-break.c (inline_func1, not_inline_func1) (inline_func2, not_inline_func2, inline_func3, not_inline_func3): New functions. (main): Call not_inline_func3. * gdb.opt/inline-break.exp: Start inferior and set breakpoints at inline_func1, inline_func2, and inline_func3. Test that when each breakpoint is hit, GDB properly reports both the stop location and the backtrace. --- gdb/breakpoint.c | 128 +++++++++++++++++++++------------ gdb/breakpoint.h | 19 ++++- gdb/infrun.c | 22 +++--- gdb/inline-frame.c | 46 +++++++++++- gdb/inline-frame.h | 2 +- gdb/testsuite/gdb.opt/inline-break.c | 50 +++++++++++++ gdb/testsuite/gdb.opt/inline-break.exp | 35 +++++++++ 7 files changed, 244 insertions(+), 58 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index c283ec0..0a087ec 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -4547,7 +4547,7 @@ bpstat_find_breakpoint (bpstat bsp, struct breakpoint *breakpoint) /* See breakpoint.h. */ int -bpstat_explains_signal (bpstat bsp, enum gdb_signal sig) +bpstat_explains_signal (bpstat bsp, enum gdb_signal sig, struct breakpoint **bp) { for (; bsp != NULL; bsp = bsp->next) { @@ -4556,13 +4556,21 @@ bpstat_explains_signal (bpstat bsp, enum gdb_signal sig) /* A moribund location can never explain a signal other than GDB_SIGNAL_TRAP. */ if (sig == GDB_SIGNAL_TRAP) - return 1; + { + if (bp != NULL) + *bp = NULL; + return 1; + } } else { if (bsp->breakpoint_at->ops->explains_signal (bsp->breakpoint_at, sig)) - return 1; + { + if (bp != NULL) + *bp = bsp->breakpoint_at;; + return 1; + } } } @@ -5597,47 +5605,16 @@ need_moribund_for_location_type (struct bp_location *loc) && !target_supports_stopped_by_hw_breakpoint ())); } +/* Build the bstat chain for the stop information given by ASPACE, + BP_ADDR, and WS. BS_LINK should point to storage which may be subsequently + passed to bpstat_stop_status to avoid rebuilding the stop chain. */ -/* Get a bpstat associated with having just stopped at address - BP_ADDR in thread PTID. - - Determine whether we stopped at a breakpoint, etc, or whether we - don't understand this stop. Result is a chain of bpstat's such - that: - - if we don't understand the stop, the result is a null pointer. - - if we understand why we stopped, the result is not null. - - Each element of the chain refers to a particular breakpoint or - watchpoint at which we have stopped. (We may have stopped for - several reasons concurrently.) - - Each element of the chain has valid next, breakpoint_at, - commands, FIXME??? fields. */ - -bpstat -bpstat_stop_status (struct address_space *aspace, - CORE_ADDR bp_addr, ptid_t ptid, - const struct target_waitstatus *ws) +static void +build_bpstat_chain (bpstat *bs_link, struct address_space *aspace, + CORE_ADDR bp_addr, const struct target_waitstatus *ws) { - struct breakpoint *b = NULL; + struct breakpoint *b; struct bp_location *bl; - struct bp_location *loc; - /* First item of allocated bpstat's. */ - bpstat bs_head = NULL, *bs_link = &bs_head; - /* Pointer to the last thing in the chain currently. */ - bpstat bs; - int ix; - int need_remove_insert; - int removed_any; - - /* First, build the bpstat chain with locations that explain a - target stop, while being careful to not set the target running, - as that may invalidate locations (in particular watchpoint - locations are recreated). Resuming will happen here with - breakpoint conditions or watchpoint expressions that include - inferior function calls. */ ALL_BREAKPOINTS (b) { @@ -5663,8 +5640,8 @@ bpstat_stop_status (struct address_space *aspace, /* Come here if it's a watchpoint, or if the break address matches. */ - bs = bpstat_alloc (bl, &bs_link); /* Alloc a bpstat to - explain stop. */ + bpstat bs = bpstat_alloc (bl, &bs_link); /* Alloc a bpstat to + explain stop. */ /* Assume we stop. Should we find a watchpoint that is not actually triggered, or if the condition of the breakpoint @@ -5689,12 +5666,15 @@ bpstat_stop_status (struct address_space *aspace, if (!target_supports_stopped_by_sw_breakpoint () || !target_supports_stopped_by_hw_breakpoint ()) { - for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix) + struct bp_location *loc; + + for (int ix = 0; + VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix) { if (breakpoint_location_address_match (loc, aspace, bp_addr) && need_moribund_for_location_type (loc)) { - bs = bpstat_alloc (loc, &bs_link); + bpstat bs = bpstat_alloc (loc, &bs_link); /* For hits of moribund locations, we should just proceed. */ bs->stop = 0; bs->print = 0; @@ -5702,6 +5682,64 @@ bpstat_stop_status (struct address_space *aspace, } } } +} + +/* See breakpoint.h. */ + +struct breakpoint * +breakpoint_for_stop (struct address_space *aspace, CORE_ADDR stop_pc, + enum gdb_signal stop_signal, + const struct target_waitstatus *ws, + bpstat *storage) +{ + build_bpstat_chain (storage, aspace, stop_pc, ws); + + struct breakpoint *bpt = NULL; + if (bpstat_explains_signal (*storage, stop_signal, &bpt)) + return bpt; + + return NULL; +} + +/* Get a bpstat associated with having just stopped at address + BP_ADDR in thread PTID. + + Determine whether we stopped at a breakpoint, etc, or whether we + don't understand this stop. Result is a chain of bpstat's such + that: + + if we don't understand the stop, the result is a null pointer. + + if we understand why we stopped, the result is not null. + + Each element of the chain refers to a particular breakpoint or + watchpoint at which we have stopped. (We may have stopped for + several reasons concurrently.) + + Each element of the chain has valid next, breakpoint_at, + commands, FIXME??? fields. */ + +bpstat +bpstat_stop_status (struct address_space *aspace, + CORE_ADDR bp_addr, ptid_t ptid, + const struct target_waitstatus *ws, + bpstat opt_chain) +{ + struct breakpoint *b = NULL; + /* First item of allocated bpstat's. */ + bpstat bs_head = opt_chain; + bpstat bs; + int need_remove_insert; + int removed_any; + + /* First, build the bpstat chain with locations that explain a + target stop, while being careful to not set the target running, + as that may invalidate locations (in particular watchpoint + locations are recreated). Resuming will happen here with + breakpoint conditions or watchpoint expressions that include + inferior function calls. */ + if (bs_head == NULL) + build_bpstat_chain (&bs_head, aspace, bp_addr, ws); /* A bit of special processing for shlib breakpoints. We need to process solib loading here, so that the lists of loaded and diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 2f10c3b..1d7ed48 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -918,7 +918,21 @@ extern bpstat bpstat_copy (bpstat); extern bpstat bpstat_stop_status (struct address_space *aspace, CORE_ADDR pc, ptid_t ptid, - const struct target_waitstatus *ws); + const struct target_waitstatus *ws, + bpstat opt_chain); + +/* If the stop described by ASPACE, STOP_PC, STOP_SIGNAL, and WS was + the result of a breakpoint, return that breakpoint; otherwise return + NULL. + + STORAGE will hold the bpstat chain and may be passed to bpstat_stop_status + to avoid rebuilding the entire stop chain multiple times. */ + +extern struct breakpoint * + breakpoint_for_stop (struct address_space *aspace, CORE_ADDR stop_pc, + enum gdb_signal stop_signal, + const struct target_waitstatus *ws, + bpstat *storage); /* This bpstat_what stuff tells wait_for_inferior what to do with a breakpoint (a challenging task). @@ -1028,7 +1042,8 @@ bpstat bpstat_find_breakpoint (bpstat, struct breakpoint *); /* Nonzero if a signal that we got in target_wait() was due to circumstances explained by the bpstat; the signal is therefore not random. */ -extern int bpstat_explains_signal (bpstat, enum gdb_signal); +extern int bpstat_explains_signal (bpstat, enum gdb_signal, + struct breakpoint **); /* Nonzero is this bpstat causes a stop. */ extern int bpstat_causes_stop (bpstat); diff --git a/gdb/infrun.c b/gdb/infrun.c index 5e4cd51..7efafa6 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4276,7 +4276,7 @@ handle_syscall_event (struct execution_control_state *ecs) ecs->event_thread->control.stop_bpstat = bpstat_stop_status (get_regcache_aspace (regcache), - stop_pc, ecs->ptid, &ecs->ws); + stop_pc, ecs->ptid, &ecs->ws, NULL); if (handle_stop_requested (ecs)) return 0; @@ -4979,7 +4979,7 @@ handle_inferior_event_1 (struct execution_control_state *ecs) ecs->event_thread->control.stop_bpstat = bpstat_stop_status (get_regcache_aspace (regcache), - stop_pc, ecs->ptid, &ecs->ws); + stop_pc, ecs->ptid, &ecs->ws, NULL); if (handle_stop_requested (ecs)) return; @@ -5233,7 +5233,7 @@ Cannot fill $_exitsignal with the correct signal number.\n")); ecs->event_thread->control.stop_bpstat = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()), - stop_pc, ecs->ptid, &ecs->ws); + stop_pc, ecs->ptid, &ecs->ws, NULL); if (handle_stop_requested (ecs)) return; @@ -5345,7 +5345,7 @@ Cannot fill $_exitsignal with the correct signal number.\n")); ecs->event_thread->control.stop_bpstat = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()), - stop_pc, ecs->ptid, &ecs->ws); + stop_pc, ecs->ptid, &ecs->ws, NULL); /* Note that this may be referenced from inside bpstat_stop_status above, through inferior_has_execd. */ @@ -5884,6 +5884,7 @@ handle_signal_stop (struct execution_control_state *ecs) ecs->event_thread->control.stop_step = 0; stop_print_frame = 1; stopped_by_random_signal = 0; + bpstat stop_chain = NULL; /* Hide inlined functions starting here, unless we just performed stepi or nexti. After stepi and nexti, always show the innermost frame (not any @@ -5915,7 +5916,12 @@ handle_signal_stop (struct execution_control_state *ecs) ecs->event_thread->prev_pc, &ecs->ws))) { - skip_inline_frames (ecs->ptid); + struct breakpoint *bpt + = breakpoint_for_stop (aspace, stop_pc, + ecs->event_thread->suspend.stop_signal, + &ecs->ws, &stop_chain); + + skip_inline_frames (ecs->ptid, bpt); /* Re-fetch current thread's frame in case that invalidated the frame cache. */ @@ -5964,7 +5970,7 @@ handle_signal_stop (struct execution_control_state *ecs) handles this event. */ ecs->event_thread->control.stop_bpstat = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()), - stop_pc, ecs->ptid, &ecs->ws); + stop_pc, ecs->ptid, &ecs->ws, stop_chain); /* Following in case break condition called a function. */ @@ -5981,7 +5987,7 @@ handle_signal_stop (struct execution_control_state *ecs) if (debug_infrun && ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP && !bpstat_explains_signal (ecs->event_thread->control.stop_bpstat, - GDB_SIGNAL_TRAP) + GDB_SIGNAL_TRAP, NULL) && stopped_by_watchpoint) fprintf_unfiltered (gdb_stdlog, "infrun: no user watchpoint explains " @@ -6010,7 +6016,7 @@ handle_signal_stop (struct execution_control_state *ecs) /* See if the breakpoints module can explain the signal. */ random_signal = !bpstat_explains_signal (ecs->event_thread->control.stop_bpstat, - ecs->event_thread->suspend.stop_signal); + ecs->event_thread->suspend.stop_signal, NULL); /* Maybe this was a trap for a software breakpoint that has since been removed. */ diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index b6154cd..006ae0d 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -27,6 +27,7 @@ #include "symtab.h" #include "vec.h" #include "frame.h" +#include "linespec.h" /* We need to save a few variables for every thread stopped at the virtual call site of an inlined function. If there was always a @@ -301,13 +302,34 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block) user steps into them. */ void -skip_inline_frames (ptid_t ptid) +skip_inline_frames (ptid_t ptid, struct breakpoint *bpt) { CORE_ADDR this_pc; const struct block *frame_block, *cur_block; struct symbol *last_sym = NULL; int skip_count = 0; struct inline_state *state; + struct linespec_result canonical; + + canonical.sals = NULL; + if (bpt != NULL) + { + const struct event_location *location = bpt->location.get (); + + if (location != NULL && event_location_type (location) != PROBE_LOCATION) + { + TRY + { + decode_line_full (location, DECODE_LINE_FUNFIRSTLINE, bpt->pspace, + NULL, 0, &canonical, multiple_symbols_all, + NULL); + } + CATCH (e, RETURN_MASK_ERROR) + { + } + END_CATCH + } + } /* This function is called right after reinitializing the frame cache. We try not to do more unwinding than absolutely @@ -327,7 +349,27 @@ skip_inline_frames (ptid_t ptid) if (BLOCK_START (cur_block) == this_pc || block_starting_point_at (this_pc, cur_block)) { - skip_count++; + int lsal_i; + struct linespec_sals *lsal; + bool skip_this_frame = true; + + for (lsal_i = 0; + VEC_iterate (linespec_sals, canonical.sals, + lsal_i, lsal); lsal_i++) + { + struct symtabs_and_lines &sals = lsal->sals; + + for (int sals_i = 0; sals_i < sals.nelts; sals_i++) + { + struct symtab_and_line &sal = sals.sals[sals_i]; + + if (sal.pc == this_pc) + skip_this_frame = false; + } + } + + if (skip_this_frame) + skip_count++; last_sym = BLOCK_FUNCTION (cur_block); } else diff --git a/gdb/inline-frame.h b/gdb/inline-frame.h index e6fe29d..8afaf6c 100644 --- a/gdb/inline-frame.h +++ b/gdb/inline-frame.h @@ -31,7 +31,7 @@ extern const struct frame_unwind inline_frame_unwind; Frames for the hidden functions will not appear in the backtrace until the user steps into them. */ -void skip_inline_frames (ptid_t ptid); +void skip_inline_frames (ptid_t ptid, struct breakpoint *bpt); /* Forget about any hidden inlined functions in PTID, which is new or about to be resumed. If PTID is minus_one_ptid, forget about all diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c index 2616a0e..a42bc0b 100644 --- a/gdb/testsuite/gdb.opt/inline-break.c +++ b/gdb/testsuite/gdb.opt/inline-break.c @@ -128,6 +128,54 @@ func8a (int x) return func8b (x * 31); } +static inline ATTR int +inline_func1 (int x) +{ + int y = 1; /* inline_func1 */ + + return y + x; +} + +static int +not_inline_func1 (int x) +{ + int y = 2; /* not_inline_func1 */ + + return y + inline_func1 (x); +} + +inline ATTR int +inline_func2 (int x) +{ + int y = 3; /* inline_func2 */ + + return y + not_inline_func1 (x); +} + +int +not_inline_func2 (int x) +{ + int y = 4; /* not_inline_func2 */ + + return y + inline_func2 (x); +} + +static inline ATTR int +inline_func3 (int x) +{ + int y = 5; /* inline_func3 */ + + return y + not_inline_func2 (x); +} + +static int +not_inline_func3 (int x) +{ + int y = 6; /* not_inline_func3 */ + + return y + inline_func3 (x); +} + /* Entry point. */ int @@ -155,5 +203,7 @@ main (int argc, char *argv[]) x = func8a (x) + func8b (x); + x = not_inline_func3 (-21); + return x; } diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp index 91766d4..7965c77 100644 --- a/gdb/testsuite/gdb.opt/inline-break.exp +++ b/gdb/testsuite/gdb.opt/inline-break.exp @@ -211,4 +211,39 @@ for {set i 1} {$i <= [llength [array names results]]} {incr i} { gdb_test "interpreter-exec mi -break-info 1" \ ".*,call-site-func=\"main\",call-site-file=\".*\",call-site-fullname=\".*\",call-site-line=\".*\".*" +# Start us running. +if {![runto main]} { + untested "could not run to main" + return -1 +} + +# Insert breakpoints for all inline_func? and not_inline_func? and check +# that we actually stop where we think we should. + +for {set i 1} {$i < 4} {incr i} { + foreach inline {"not_inline" "inline"} { + gdb_breakpoint "${inline}_func$i" message + } +} + +set ws {[\r\n\t ]+} +set backtrace [list "(in|at)? main"] +for {set i 3} {$i > 0} {incr i -1} { + + foreach inline {"not_inline" "inline"} { + + # Check that we stop at the correct location and print out + # the (possibly) inlined frames. + set num [gdb_get_line_number "/* ${inline}_func$i */"] + set pattern ".*/$srcfile:$num${ws}.*$num${ws}int y = $decimal;" + append pattern "${ws}/\\\* ${inline}_func$i \\\*/" + send_log "Expecting $pattern\n" + gdb_continue_to_breakpoint "${inline}_func$i" $pattern + + # Also check for the correct backtrace. + set backtrace [linsert $backtrace 0 "(in|at)?${ws}${inline}_func$i"] + gdb_test_sequence "bt" "bt stopped in ${inline}_func$i" $backtrace + } +} + unset -nocomplain results