From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id CBBCA3858C50 for ; Sat, 14 Jan 2023 11:00:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CBBCA3858C50 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673694037; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=TfZ6G6Qp3/3AL3F6vBfFOfuLJsMV4HDp8HnOpsq7nT0=; b=BvzIxb5NRpQwMKcgusNdbgU8wk4VCy0c1hmBfo9KdZx5U5VfxCTjwFGvvMFaJy1/42cVuz FjgDNPMbeiRIqbeoGvsLsFVUmtCmjLr5KriyoN2OcLpLv0izwHe+qoEHw28HI4PrK/U/zV FCLKqmvQTQ5/FW2IRDZpYizTVv+yGV8= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-550-FSM4J-NNNH2EeBQGo2ehoQ-1; Sat, 14 Jan 2023 06:00:36 -0500 X-MC-Unique: FSM4J-NNNH2EeBQGo2ehoQ-1 Received: by mail-wm1-f72.google.com with SMTP id n18-20020a05600c4f9200b003d993e08485so12294474wmq.2 for ; Sat, 14 Jan 2023 03:00:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=TfZ6G6Qp3/3AL3F6vBfFOfuLJsMV4HDp8HnOpsq7nT0=; b=s2KrUwozt2R0w7uRBxICCdFWqwWZBVV2Yr4br4IjJhMH7ho0VZ+sMZoC755MY24EXV oo3i5sg3rZZ2Z2vTTVsaBUt0S35BehgHgIfNZRJDIhiTFVvu14BwNxHMu70Rn1l1JLLU iQp2Q5vCJDHJA36DHIvJ2IM9Q89UOeb5tMCE1gvxyK0KH+y8+w+OHnCNJpuYhVAKEWFL BC1Lj1usAWZMhR0gUr7kDcFpC4wtGCjNRRsY6GXjnuTk7LHKanllADRfhZ6cjMp2fmIq 1kQAShl4iTSlxpsypDaNQgOJeO/BXXIGy4/tvQNN8ck4NqXz1Z69MwkkePEKs7tvHZQy h3jg== X-Gm-Message-State: AFqh2kqQyFVxmDolE5A4VdYaxwaBJPb4VkWUgBk7OR+O1wVaPLroy6T2 XJlF22jd+/Bgtkk9Y5YOUDG+n+ZZ3+EfhXids0B11dpvJGkFEXbpBXvbXJ6OkZRLWUuYKwNMVgz DcfNgN5scXKhGgQTNQfanyA== X-Received: by 2002:adf:eb41:0:b0:2bd:e7de:477b with SMTP id u1-20020adfeb41000000b002bde7de477bmr2623138wrn.51.1673694034779; Sat, 14 Jan 2023 03:00:34 -0800 (PST) X-Google-Smtp-Source: AMrXdXuWH+iAeYeiZwOo2S+MgIO8xan5OzBaUVPeJftgMCVIvY8qrlXj1b0GKCzuZhp2OI13/T9/Vw== X-Received: by 2002:adf:eb41:0:b0:2bd:e7de:477b with SMTP id u1-20020adfeb41000000b002bde7de477bmr2623117wrn.51.1673694034298; Sat, 14 Jan 2023 03:00:34 -0800 (PST) Received: from localhost (92.40.219.187.threembb.co.uk. [92.40.219.187]) by smtp.gmail.com with ESMTPSA id w4-20020a5d4b44000000b002366dd0e030sm20941422wrs.68.2023.01.14.03.00.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 14 Jan 2023 03:00:33 -0800 (PST) From: Andrew Burgess To: Eli Zaretskii Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 11/12] gdb: add timeouts for inferior function calls In-Reply-To: <83a65pwi1h.fsf@gnu.org> References: <83a65pwi1h.fsf@gnu.org> Date: Sat, 14 Jan 2023 11:00:32 +0000 Message-ID: <87ilh9fkj3.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Eli, Thanks for all your feedback, sorry it has taken me so long to get back to this series. I'm working through most of your points, but I there was one issue that I wanted to follow up on, see inline below. Eli Zaretskii writes: >> Date: Fri, 21 Oct 2022 09:43:47 +0100 >> From: Andrew Burgess via Gdb-patches >> >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -115,6 +115,22 @@ set debug infcall on|off >> show debug infcall >> Print additional debug messages about inferior function calls. >> >> +set direct-call-timeout SECONDS >> +show direct-call-timeout >> +set indirect-call-timeout SECONDS >> +show indirect-call-timeout >> + These new settings can be used to limit how long GDB will wait for >> + an inferior function call to complete. The direct timeout is used >> + for inferior function calls from e.g. 'call' and 'print' commands, >> + while the indirect timeout is used for inferior function calls from >> + within a conditional breakpoint expression. >> + >> + The default for the direct timeout is unlimited, while the default >> + for the indirect timeout is 300 seconds. > > IMO, 300 seconds is way too long a wait for a breakpoint condition. > It should be a matter of a few seconds at most, IMO. > >> + These timeouts will only have an effect for targets that are >> + operating in async mode. > > This should tell what happens on targets that don't support async > mode. > >> +When calling a function within a program, it is possible that the >> +program could enter a state from which the called function may never >> +return. If this happens then @value{GDBN} will appear to hang. >> +Should this happen then it is possible to interrupt the running >> +program by typing the interrupt character (often @kbd{Ctrl-c}). >> + >> +On some targets @value{GDBN} can also place a timeout on any function >> +calls made into the program. > > Instead of "some targets", which leaves those targets unspecified, we > should say "targets that support async mode", with a cross-reference > to where that is described. We should give the user a means to > determine whether the particular target does or doesn't need/support > this timeout feature. > >> If the timeout expires and the function >> +call is still going, then @value{GDBN} will interrupt the program >> +automatically. > > And what does this mean for the value returned by the interrupted > call? This is important for the breakpoint condition use case, for > example. > >> + >> +@table @code >> +@item set direct-call-timeout @var{seconds} >> +@kindex set direct-call-timeout >> +@cindex timeout for called functions >> +Set the timeout used when calling functions in the program to >> +@var{seconds}, which should be an integer greater than zero, or the >> +special value @code{unlimited}, which indicates no timeout should be >> +used. The default for this setting is @code{unlimited}. > > Why integer in seconds? don't we want to be able to support shorter > timeouts, like 100 msec? Most inferior calls should take much less > than a second, so a second resolution is not the best idea, IMO. It > could, for example, make running a program with such a breakpoint > unbearably slow. Remember, this timeout is a safety net intended to catch situations where either due to a bug in the inferior, or due to user error, the breakpoint condition is never going to complete. As such I don't think we should be trying to trim this value down as low as possible, I just don't see any additional value, in fact, I see doing so adding more risk that the user will hit invalid timeouts. If you wanted to be super aggressive with the timeout, and you set it to 1 second, but you still expect your inferior calls to complete in 100msec, then the timeout being 1 second will not slow you down in any way. But, if for some reason your inferior call deadlocks, you'll end up waiting that complete second before GDB gives up. I can't imagine a use case where a user will be debugging by having an inferior function call repeatedly timeout, and that's the only way that having such a large timeout would be unbearable That all said, the underlying timer mechanism does use msec, so if you feel strongly that we should be able to have sub-second timeouts, then it's trivial to switch over, though I do think that the default should be multiple seconds (I've reduced the default to 30 seconds based on your other feedback), as I'd like GDB's default behaviour to allow for debugging slower, remote targets, where that timeout has to allow for packets sent to the remote target, and slower remote targets. Thanks, Andrew > >> +This setting only works for targets that support asynchronous >> +execution (@pxref{Background Execution}), for any other target the >> +setting is treated as @code{unlimited}. > > This should be moved to the beginning of the description, as mentioned > above. In addition, saying "treated as 'unlimited'" is not clear > enough in this context, because actually no timeout is applicable at > all, and GDB will wait indefinitely for the call to return. We should > tell this explicitly. > >> +It is also possible to call functions within the program from the >> +condition of a conditional breakpoint (@pxref{Conditions, ,Break >> +Conditions}). A different setting controls the timeout used for >> +functions made from a breakpoint condition. > ^^^^^^^^^^^^^^ > "function calls made..." > >> +@item set indirect-call-timeout @var{seconds} >> +@kindex set indirect-call-timeout >> +@cindex timeout for called functions >> +Set the timeout used when calling functions in the program from a >> +breakpoint condition to @var{seconds}, which should be an integer >> +greater than zero, or the special value @code{unlimited}, which >> +indicates no timeout should be used. The default for this setting is >> +@code{300} seconds. > > Here 300 seconds is definitely too long. > >> + add_setshow_uinteger_cmd ("direct-call-timeout", no_class, >> + &direct_call_timeout, _("\ >> +Set the timeout, for direct calls to inferior function calls."), _("\ >> +Show the timeout, for direct calls to inferior function calls."), _("\ >> +If running on a target that supports, and is running in, async mode\n\ >> +then this timeout is used for any inferior function calls triggered\n\ >> +directly from the prompt, e.g. from a 'call' or 'print' command. The\n\ >> +timeout is specified in seconds."), >> + nullptr, >> + show_direct_call_timeout, >> + &setlist, &showlist); >> + >> + add_setshow_uinteger_cmd ("indirect-call-timeout", no_class, >> + &indirect_call_timeout, _("\ >> +Set the timeout, for indirect calls to inferior function calls."), _("\ >> +Show the timeout, for indirect calls to inferior function calls."), _("\ >> +If running on a target that supports, and is running in, async mode\n\ >> +then this timeout is used for any inferior function calls triggered\n\ >> +indirectly, e.g. when evaluating a conditional breakpoint expression.\n\ >> +The timeout is specified in seconds."), > > These doc strings explain what is a "direct" vs "indirect" call by way > of "e.g.". But that leaves the issue not well-defined, because it > begs the question: what are the other cases that are considered > "direct" or "indirect"?