From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55021 invoked by alias); 6 Apr 2018 15:56:55 -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 55012 invoked by uid 89); 6 Apr 2018 15:56:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=cancelled, weekend X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Apr 2018 15:56:52 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2550477067 for ; Fri, 6 Apr 2018 15:56:51 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id A6E2410F1BF5; Fri, 6 Apr 2018 15:56:48 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Jan Kratochvil Subject: Re: [PATCH v4] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages References: <20180124194714.26222-1-sergiodj@redhat.com> <20180405184648.3055-1-sergiodj@redhat.com> <4477e049-87f5-5615-533e-c9bde580f935@redhat.com> Date: Fri, 06 Apr 2018 15:56:00 -0000 In-Reply-To: <4477e049-87f5-5615-533e-c9bde580f935@redhat.com> (Pedro Alves's message of "Fri, 6 Apr 2018 16:39:19 +0100") Message-ID: <876054l45b.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-04/txt/msg00095.txt.bz2 On Friday, April 06 2018, Pedro Alves wrote: > On 04/05/2018 07:46 PM, Sergio Durigan Junior wrote: >> Changes from v3: >> >> - Revisited commit log and fixed wrong copy&paste from GDB output. >> >> - Use target_pid_to_str where applicable. Use >> gdb::unique_xmalloc_ptr to save results from >> target_pid_to_str in some cases. > > Arguably target_pid_to_str implementations should be > using get_print_cell instead of a single static buffer, avoiding > the problem. But that's a larger change, so local copy is fine > with me. I can send a patch for this later. >> @@ -2598,8 +2598,16 @@ kill_command (const char *arg, int from_tty) >> error (_("The program is not being run.")); >> if (!query (_("Kill the program being debugged? "))) >> error (_("Not confirmed.")); >> + >> + const char *pid_str = target_pid_to_str (inferior_ptid); >> + int infnum = current_inferior ()->num; >> + >> target_kill (); >> >> + if (print_inferior_events) >> + printf_unfiltered (_("[Inferior %d (process %s) has been killed]\n"), >> + infnum, pid_str); > > This still seem risky -- Target backends use target_pid_to_str inside > target_kill, e.g., when logging is enabled. E.g., > > linux_nat_kill -> -> stop_callback -> target_pid_to_str > > ISTM a deep copy like: > > std::string pid_str = target_pid_to_str (inferior_ptid); > > would be safer/better. Fair enough, I'll change it. >> /* The Current Inferior. This is a strong reference. I.e., whenever >> an inferior is the current inferior, its refcount is >> @@ -123,7 +122,8 @@ add_inferior (int pid) >> struct inferior *inf = add_inferior_silent (pid); >> >> if (print_inferior_events) >> - printf_unfiltered (_("[New inferior %d]\n"), pid); >> + printf_unfiltered (_("[New inferior %d (process %d)]\n"), >> + inf->num, pid); > > As discussed in a previous revision, using hardcoded "(process %d)" > doesn't work properly, because PID can be a fake PID number, or the > target may have no concept of processes, e.g., when debugging remote > targets. ISTRM this should use target_pid_to_str as well. That's true, I apologize for overlooking this one. And I also noticed the problem with including the "process" string before, that's why I sent the message "cancelling" this patch. > >> @@ -266,7 +263,8 @@ detach_inferior (inferior *inf) >> exit_inferior_1 (inf, 0); >> >> if (print_inferior_events) >> - printf_unfiltered (_("[Inferior %d detached]\n"), pid); >> + printf_unfiltered (_("[Inferior %d (process %d) detached]\n"), >> + inf->num, pid); > > Ditto. Got it. >> - if (info_verbose || debug_infrun) >> + if (print_inferior_events) >> { >> + gdb::unique_xmalloc_ptr >> + parent_pid (xstrdup (target_pid_to_str (parent_ptid))); >> + gdb::unique_xmalloc_ptr >> + child_pid (xstrdup (target_pid_to_str (child_ptid))); > > std::string would be simpler than gdb::unique_xmalloc_ptr here: > > std::string parent_pid = target_pid_to_str (parent_ptid)); > std::string child_pid = target_pid_to_str (child_ptid)); > > gdb::unique_xmalloc_ptr is handy when you have no way around > malloc/free, but here you're in charge of the dup yourself. That's true. I guess I wasn't thinking in C++ terms. >> diff --git a/gdb/remote.c b/gdb/remote.c >> index 68c43f8312..4cb4badd8a 100644 >> --- a/gdb/remote.c >> +++ b/gdb/remote.c >> @@ -5137,7 +5137,12 @@ remote_detach_1 (int from_tty, inferior *inf) >> /* If doing detach-on-fork, we don't mourn, because that will delete >> breakpoints that should be available for the followed inferior. */ >> if (!is_fork_parent) >> - target_mourn_inferior (inferior_ptid); >> + { >> + target_mourn_inferior (inferior_ptid); >> + if (print_inferior_events) >> + printf_unfiltered (_("[Inferior %d (process %d) detached]\n"), >> + inf->num, pid); > > Hardcoded "(process %d)" here too. > > Fixing this issue in the several spots may affect your > testsuite changes -- please be sure to rerun tests with > "target remote" afterwards. I'm doing it, thanks. > >> + } >> else >> { > >> } >> >> diff --git a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp >> index e05acb1711..616b6cf7a4 100644 >> --- a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp >> +++ b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp >> @@ -81,8 +81,15 @@ proc detach_and_expect_exit {inf_output_re test} { >> global inferior_spawn_id >> global gdb_prompt >> >> + set saw_inf_exit 0 >> return_if_fail [gdb_test_multiple "detach" $test { >> - -re "Detaching from .*, process $decimal" { >> + -re "Detaching from .*, process $decimal\r\n\\\[Inferior $decimal \\(process $decimal\\) detached\\\]" { >> + } >> + # inf_output_re can also appear in the middle, so we catch >> + # this case here in order to avoid racy results. >> + -re "Detaching from .*, process $decimal\r\n${inf_output_re}\r\n\\\[Inferior $decimal \\(process $decimal\\) detached\\\]" { >> + verbose -log "saw inferior exit" >> + set saw_inf_exit 1 >> } > > I'm not sure I understand the need for this. If you left this > gdb_test_multiple exactly as it was before your patch, wouldn't it all > work the same? As I said in the other message, the problem here is that ${inf_output_re} can happen between the two messages. For example: Detaching from program: .../gdb/testsuite/outputs/gdb.threads/process-dies-while-detaching/process-dies-while-detaching-1-detach, process 7440 exited, status=0 [Inferior 1 (process 7440) detached] In this case, leave gdb_test_multiple as it was before doesn't catch this case, which leads to a racy failure. However, I noticed that my patch also doesn't fix the failure (I thought it did, but then I saw it happening again on the BuildBot). That's another reason why I "cancelled" this version of the patch. I'm trying to come up with another solution for this race, but so far I haven't had much success. Of course, if you have any ideas feel free to suggest them. Otherwise, I'll see what I can do during the weekend (I'm on PTO today). Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/