From: Sergio Durigan Junior <sergiodj@redhat.com>
To: Pedro Alves <palves@redhat.com>
Cc: GDB Patches <gdb-patches@sourceware.org>,
Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: Re: [PATCH v4] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages
Date: Fri, 06 Apr 2018 15:56:00 -0000 [thread overview]
Message-ID: <876054l45b.fsf@redhat.com> (raw)
In-Reply-To: <4477e049-87f5-5615-533e-c9bde580f935@redhat.com> (Pedro Alves's message of "Fri, 6 Apr 2018 16:39:19 +0100")
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<char> 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<char>
>> + parent_pid (xstrdup (target_pid_to_str (parent_ptid)));
>> + gdb::unique_xmalloc_ptr<char>
>> + 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/
next prev parent reply other threads:[~2018-04-06 15:56 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-24 19:47 [PATCH] Always print "Detaching after fork from child..." Sergio Durigan Junior
2018-01-24 20:43 ` Jan Kratochvil
2018-01-24 20:56 ` Sergio Durigan Junior
2018-01-25 15:59 ` Pedro Alves
2018-01-25 20:21 ` Sergio Durigan Junior
2018-01-25 22:39 ` Pedro Alves
2018-01-31 16:57 ` [PATCH v2] Enable 'set print inferior-events' and cleanup attach/detach messages Sergio Durigan Junior
2018-02-01 17:17 ` Pedro Alves
2018-03-06 1:44 ` Sergio Durigan Junior
2018-03-09 21:56 ` [PATCH v3] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages Sergio Durigan Junior
2018-03-20 19:24 ` Sergio Durigan Junior
2018-03-26 10:58 ` Pedro Alves
2018-03-26 11:43 ` Pedro Alves
2018-04-03 0:15 ` Sergio Durigan Junior
2018-04-02 21:51 ` Sergio Durigan Junior
2018-04-05 18:47 ` [PATCH v4] " Sergio Durigan Junior
2018-04-05 21:32 ` Sergio Durigan Junior
2018-04-06 15:39 ` Pedro Alves
2018-04-06 15:56 ` Sergio Durigan Junior [this message]
2018-04-06 16:41 ` Pedro Alves
2018-04-10 16:22 ` Sergio Durigan Junior
2018-04-11 18:46 ` [PATCH v5] " Sergio Durigan Junior
2018-04-11 19:05 ` Pedro Alves
2018-04-11 19:08 ` Sergio Durigan Junior
2018-04-16 20:04 ` [PATCH v6] " Sergio Durigan Junior
2018-04-17 15:57 ` Pedro Alves
2018-04-17 20:07 ` Sergio Durigan Junior
2018-04-19 19:54 ` [PATCH v7] " Sergio Durigan Junior
2018-04-24 13:33 ` Pedro Alves
2018-04-24 19:49 ` Sergio Durigan Junior
2018-04-25 17:41 ` [PATCH] Fix new inferior events output (Re: [PATCH v7] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages) Pedro Alves
2018-04-25 17:53 ` Sergio Durigan Junior
2018-04-25 18:07 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=876054l45b.fsf@redhat.com \
--to=sergiodj@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.kratochvil@redhat.com \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).