From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28185 invoked by alias); 3 Mar 2015 01:37:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 28173 invoked by uid 89); 3 Mar 2015 01:37:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 03 Mar 2015 01:37:49 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t231bjT3001915 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 2 Mar 2015 20:37:46 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t231bhu7020296; Mon, 2 Mar 2015 20:37:44 -0500 Message-ID: <54F51067.5030806@redhat.com> Date: Tue, 03 Mar 2015 01:37:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: "Breazeal, Don" , gdb-patches@sourceware.org Subject: Re: [PATCH] follow-exec: handle targets that don't have thread exit events References: <1415905375-29865-1-git-send-email-palves@redhat.com> <5482541B.40701@codesourcery.com> In-Reply-To: <5482541B.40701@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00056.txt.bz2 On 12/06/2014 12:55 AM, Breazeal, Don wrote: > I walked through this, and it makes sense to me. We know that > on entry to follow_exec inferior_thread() is the event thread, > which is also the leader thread, right? Thanks for digging into this. Yep. Funny thing, while looking at your fork series today (need to play with it a bit more), and recalled this patch. And then tonight I rebased my all-stop-on-top-of-non-stop series on current mainline and noticed that GDB crashed while running thread-execl.exp. Running the test in (real) non-stop mode with current mainline under Valgrind shows the same exact same invalid reads... So this isn't specific to remote debugging only and can be triggered today. Given that, I've now extended the thread-execl.exp test to run in non-stop mode too, tweaked the comment in infrun a bit, and pushed it in, as below. --- >From 95e50b2723eba05ca34e9ea69c1de63e65ce9578 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 3 Mar 2015 01:25:17 +0000 Subject: [PATCH] follow-exec: delete all non-execing threads This fixes invalid reads Valgrind first caught when debugging against a GDBserver patched with a series that adds exec events to the remote protocol. Like these, using the gdb.threads/thread-execl.exp test: $ valgrind ./gdb -data-directory=data-directory ./testsuite/gdb.threads/thread-execl -ex "tar extended-remote :9999" -ex "b thread_execler" -ex "c" -ex "set scheduler-locking on" ... Breakpoint 1, thread_execler (arg=0x0) at src/gdb/testsuite/gdb.threads/thread-execl.c:29 29 if (execl (image, image, NULL) == -1) (gdb) n Thread 32509.32509 is executing new program: build/gdb/testsuite/gdb.threads/thread-execl [New Thread 32509.32532] ==32510== Invalid read of size 4 ==32510== at 0x5AA7D8: delete_breakpoint (breakpoint.c:13989) ==32510== by 0x6285D3: delete_thread_breakpoint (thread.c:100) ==32510== by 0x628603: delete_step_resume_breakpoint (thread.c:109) ==32510== by 0x61622B: delete_thread_infrun_breakpoints (infrun.c:2928) ==32510== by 0x6162EF: for_each_just_stopped_thread (infrun.c:2958) ==32510== by 0x616311: delete_just_stopped_threads_infrun_breakpoints (infrun.c:2969) ==32510== by 0x616C96: fetch_inferior_event (infrun.c:3267) ==32510== by 0x63A2DE: inferior_event_handler (inf-loop.c:57) ==32510== by 0x4E0E56: remote_async_serial_handler (remote.c:11877) ==32510== by 0x4AF620: run_async_handler_and_reschedule (ser-base.c:137) ==32510== by 0x4AF6F0: fd_event (ser-base.c:182) ==32510== by 0x63806D: handle_file_event (event-loop.c:762) ==32510== Address 0xcf333e0 is 16 bytes inside a block of size 200 free'd ==32510== at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==32510== by 0x77CB74: xfree (common-utils.c:98) ==32510== by 0x5AA954: delete_breakpoint (breakpoint.c:14056) ==32510== by 0x5988BD: update_breakpoints_after_exec (breakpoint.c:3765) ==32510== by 0x61360F: follow_exec (infrun.c:1091) ==32510== by 0x6186FA: handle_inferior_event (infrun.c:4061) ==32510== by 0x616C55: fetch_inferior_event (infrun.c:3261) ==32510== by 0x63A2DE: inferior_event_handler (inf-loop.c:57) ==32510== by 0x4E0E56: remote_async_serial_handler (remote.c:11877) ==32510== by 0x4AF620: run_async_handler_and_reschedule (ser-base.c:137) ==32510== by 0x4AF6F0: fd_event (ser-base.c:182) ==32510== by 0x63806D: handle_file_event (event-loop.c:762) ==32510== [Switching to Thread 32509.32532] Breakpoint 1, thread_execler (arg=0x0) at src/gdb/testsuite/gdb.threads/thread-execl.c:29 29 if (execl (image, image, NULL) == -1) (gdb) The breakpoint in question is the step-resume breakpoint of the non-main thread, the one that was "next"ed. The exact same issue can be seen on mainline with native debugging, by running the thread-execl.exp test in non-stop mode, because the kernel doesn't report a thread exit event for the execing thread. Tested on x86_64 Fedora 20. gdb/ChangeLog: 2015-03-02 Pedro Alves * infrun.c (follow_exec): Delete all threads of the process except the event thread. Extended comments. gdb/testsuite/ChangeLog: 2015-03-02 Pedro Alves * gdb.threads/thread-execl.exp (do_test): Handle non-stop. (top level): Call do_test with non-stop as well. --- gdb/ChangeLog | 5 ++++ gdb/testsuite/ChangeLog | 5 ++++ gdb/infrun.c | 48 ++++++++++++++++++++++-------- gdb/testsuite/gdb.threads/thread-execl.exp | 24 +++++++++++++-- 4 files changed, 67 insertions(+), 15 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 922b1d9..68c55c1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2015-03-02 Pedro Alves + + * infrun.c (follow_exec): Delete all threads of the process except + the event thread. Extended comments. + 2015-03-02 Joel Brobecker * contrib/ari/gdb_ari.sh: Reinstate checks for "true" and "false". diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 76ed5e2..3b7bf7d 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,5 +1,10 @@ 2015-03-02 Pedro Alves + * gdb.threads/thread-execl.exp (do_test): Handle non-stop. + (top level): Call do_test with non-stop as well. + +2015-03-02 Pedro Alves + * lib/gdb.exp (gdb_test_multiple) : Set result to -1. diff --git a/gdb/infrun.c b/gdb/infrun.c index 15589b6..f87ed4c 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1060,10 +1060,11 @@ show_follow_exec_mode_string (struct ui_file *file, int from_tty, /* EXECD_PATHNAME is assumed to be non-NULL. */ static void -follow_exec (ptid_t pid, char *execd_pathname) +follow_exec (ptid_t ptid, char *execd_pathname) { - struct thread_info *th = inferior_thread (); + struct thread_info *th, *tmp; struct inferior *inf = current_inferior (); + int pid = ptid_get_pid (ptid); /* This is an exec event that we actually wish to pay attention to. Refresh our symbol table to the newly exec'd program, remove any @@ -1088,24 +1089,47 @@ follow_exec (ptid_t pid, char *execd_pathname) mark_breakpoints_out (); - update_breakpoints_after_exec (); - - /* If there was one, it's gone now. We cannot truly step-to-next - statement through an exec(). */ + /* The target reports the exec event to the main thread, even if + some other thread does the exec, and even if the main thread was + stopped or already gone. We may still have non-leader threads of + the process on our list. E.g., on targets that don't have thread + exit events (like remote); or on native Linux in non-stop mode if + there were only two threads in the inferior and the non-leader + one is the one that execs (and nothing forces an update of the + thread list up to here). When debugging remotely, it's best to + avoid extra traffic, when possible, so avoid syncing the thread + list with the target, and instead go ahead and delete all threads + of the process but one that reported the event. Note this must + be done before calling update_breakpoints_after_exec, as + otherwise clearing the threads' resources would reference stale + thread breakpoints -- it may have been one of these threads that + stepped across the exec. We could just clear their stepping + states, but as long as we're iterating, might as well delete + them. Deleting them now rather than at the next user-visible + stop provides a nicer sequence of events for user and MI + notifications. */ + ALL_NON_EXITED_THREADS_SAFE (th, tmp) + if (ptid_get_pid (th->ptid) == pid && !ptid_equal (th->ptid, ptid)) + delete_thread (th->ptid); + + /* We also need to clear any left over stale state for the + leader/event thread. E.g., if there was any step-resume + breakpoint or similar, it's gone now. We cannot truly + step-to-next statement through an exec(). */ + th = inferior_thread (); th->control.step_resume_breakpoint = NULL; th->control.exception_resume_breakpoint = NULL; th->control.single_step_breakpoints = NULL; th->control.step_range_start = 0; th->control.step_range_end = 0; - /* The target reports the exec event to the main thread, even if - some other thread does the exec, and even if the main thread was - already stopped --- if debugging in non-stop mode, it's possible - the user had the main thread held stopped in the previous image - --- release it now. This is the same behavior as step-over-exec - with scheduler-locking on in all-stop mode. */ + /* The user may have had the main thread held stopped in the + previous image (e.g., schedlock on, or non-stop). Release + it now. */ th->stop_requested = 0; + update_breakpoints_after_exec (); + /* What is this a.out's name? */ printf_unfiltered (_("%s is executing new program: %s\n"), target_pid_to_str (inferior_ptid), diff --git a/gdb/testsuite/gdb.threads/thread-execl.exp b/gdb/testsuite/gdb.threads/thread-execl.exp index 4a8016c..a598ad0 100644 --- a/gdb/testsuite/gdb.threads/thread-execl.exp +++ b/gdb/testsuite/gdb.threads/thread-execl.exp @@ -34,9 +34,18 @@ if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \ proc do_test { schedlock } { global binfile - with_test_prefix "schedlock $schedlock" { + if {$schedlock == "non-stop"} { + set prefix $schedlock + } else { + set prefix "schedlock $schedlock" + } + with_test_prefix "$prefix" { clean_restart ${binfile} + if {$schedlock == "non-stop"} { + gdb_test_no_output "set non-stop 1" + } + if ![runto_main] { return 0 } @@ -45,16 +54,25 @@ proc do_test { schedlock } { gdb_breakpoint "thread_execler" gdb_test "continue" ".*thread_execler.*" "continue to thread start" + if {$schedlock == "non-stop"} { + gdb_test "thread 2" \ + "Switching to .*thread_execler.*" \ + "switch to event thread" + } + # Now set a breakpoint at `main', and step over the execl call. The # breakpoint at main should be reached. GDB should not try to revert # back to the old thread from the old image and resume stepping it # (since it is gone). gdb_breakpoint "main" - gdb_test_no_output "set scheduler-locking $schedlock" + + if {$schedlock != "non-stop"} { + gdb_test_no_output "set scheduler-locking $schedlock" + } gdb_test "next" ".*main.*" "get to main in new image" } } -foreach schedlock {"off" "step" "on"} { +foreach schedlock {"off" "step" "on" "non-stop"} { do_test $schedlock } -- 1.9.3