From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) by sourceware.org (Postfix) with ESMTPS id 400673858C2C for ; Mon, 27 Sep 2021 18:05:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 400673858C2C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f53.google.com with SMTP id s21so13413918wra.7 for ; Mon, 27 Sep 2021 11:05:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=GMbzKFYxlWsLfi/DwL/AHA6ok1PoVRy5mrmXPnKjvtQ=; b=gks0OHTXuKLOu+nnkX8E2HFwXylGq9JVWkeN1F3J7zRoh5XVKIdP3KSWFvPrysgTIW UXbL/T15JzuNZnvZLANe2Zw5FHV/OP+kByVWFHQdarJWhF969m3wDXO7PnqepGI3f/fK gjvd0X9jDJ+oQypJHzWT5mYklcIwHjrFmch3Jkwg9gIZjXS5eAPhNE2Pub26bugw/0FY ikt3Ibg3VqrMD9DbUsIDVaTLk6ONQ1z9OBwHhRyrET1GcGPNynzC2crg0jXxX/JoC5+I Bonf5vQvIKiJPokxEs27wQajrZirAoTlz75iWnP2lp18/gAUmcO91zj1twsVoQXwi+3I e7rw== X-Gm-Message-State: AOAM533ElbtpDXs2xLfAJ8An2t3XqD40lCKGCeBcGCuQxU10AUB1TRLz BhS5DQTwTi90xbWorFBfrTkGsKa7uHM= X-Google-Smtp-Source: ABdhPJwDmevOl/DyOOB05nJ/2JLtKwwSAIp8lmId1Z+WHBJvWxa0xbV9oqYuvvhboDYgxIn204d5Xw== X-Received: by 2002:adf:e68e:: with SMTP id r14mr1539098wrm.344.1632765928708; Mon, 27 Sep 2021 11:05:28 -0700 (PDT) Received: from ?IPv6:2001:8a0:f932:6a00:46bc:d03b:7b3a:2227? ([2001:8a0:f932:6a00:46bc:d03b:7b3a:2227]) by smtp.gmail.com with ESMTPSA id s85sm246924wme.20.2021.09.27.11.05.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 27 Sep 2021 11:05:27 -0700 (PDT) Subject: Re: [PATCH v2 3/6] Catch and (re)throw gdb_exception_quit From: Pedro Alves To: Kevin Buettner , gdb-patches@sourceware.org References: <20210822231959.184061-1-kevinb@redhat.com> <20210822231959.184061-4-kevinb@redhat.com> Message-ID: Date: Mon, 27 Sep 2021 19:05:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20210822231959.184061-4-kevinb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Sep 2021 18:05:31 -0000 On 2021-08-23 12:20 a.m., Kevin Buettner wrote: > There is one case that I examined which I could not figure out how to > handle. It is the scoped_switch_fork_info destructor in > gdb/linux-fork.c. It looks looks like this: > > ~scoped_switch_fork_info () > { > if (m_oldfp != nullptr) > { > /* Switch back to inferior_ptid. */ > try > { > remove_breakpoints (); > fork_load_infrun_state (m_oldfp); > insert_breakpoints (); > } > catch (const gdb_exception &ex) > { > warning (_("Couldn't restore checkpoint state in %s: %s"), > target_pid_to_str (m_oldfp->ptid).c_str (), > ex.what ()); > } > } > } > > The static analysis tool determined that there is a call path > to maybe_quit(): > > "_ZN23scoped_switch_fork_infoD2Ev" > -> "_Z18remove_breakpointsv" > -> "_ZL17remove_breakpointP11bp_location" > -> "_ZL19remove_breakpoint_1P11bp_location16remove_bp_reason" > -> "_Z26memory_validate_breakpointP7gdbarchP14bp_target_info" > -> "_Z18target_read_memorymPhl" > -> "_Z11target_readP10target_ops13target_objectPKcPhml" > -> "_Z10maybe_quitv"; > > The catch block simply prints a warning, thus (potentially) swallowing > a QUIT exception. It's not clear to me whether rethrowing the > exception is safe here. No, it's not safe -- you'd crash gdb. Destructors by default are noexcept, so if you let an exception escape a destructor the C++ runtime calls std::terminate(). I'm thinking we could instead do: ~foo::foo () { try { ... destruction code here ... } catch (const gdb_exception_quit &ex) { /* Can't let this escape the dtor, but we also don't want to lose it. Set the global quit flag so that we handle it later once we get to a QUIT. */ set_quit_flag (); } catch (const gdb_exception &ex) { warning (....); } } we could even wrap this in a function that take gdb::function_view to avoid duplicating the try/catch all over the place. Like: void wrap_dtor (gdb::function_view body) { try { body (); } catch (const gdb_exception_quit &ex) { /* Can't handle now. Set the global flag so that we handle it later once we get to a QUIT. */ set_quit_flag (); } catch (const gdb_exception &ex) { warning (....); } } ~foo::foo () { wrap_dtor ([] () { ... destruction code here ... }); } > --- > gdb/ada-lang.c | 4 ++++ > gdb/breakpoint.c | 15 +++++++++++++++ > gdb/i386-linux-tdep.c | 4 ++++ > gdb/infrun.c | 4 ++++ > gdb/jit.c | 4 ++++ > gdb/objc-lang.c | 4 ++++ > gdb/parse.c | 4 ++++ > gdb/printcmd.c | 4 ++++ > gdb/record-btrace.c | 4 ++++ > gdb/solib.c | 4 ++++ > gdb/sparc64-linux-tdep.c | 4 ++++ > 11 files changed, 55 insertions(+) > > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > index b098991612d..f2f6acf0bfe 100644 > --- a/gdb/ada-lang.c > +++ b/gdb/ada-lang.c > @@ -11679,6 +11679,10 @@ should_stop_exception (const struct bp_location *bl) > stop = value_true (evaluate_expression (ada_loc->excep_cond_expr.get ())); > value_free_to_mark (mark); > } > + catch (const gdb_exception_quit &ex) > + { > + throw; > + } > catch (const gdb_exception &ex) Why not just replace the existing: catch (const gdb_exception &ex) with: catch (const gdb_exception_error &ex) ? > { > exception_fprintf (gdb_stderr, ex, > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 5cc37430e36..6271545073b 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -2699,6 +2699,9 @@ insert_bp_location (struct bp_location *bl, > { > /* Can't set the breakpoint. */ > > + if (bp_excpt.reason == RETURN_QUIT) > + throw bp_excpt; > + > /* If the target has closed then it will have deleted any > breakpoints inserted within the target inferior, as a result > any further attempts to interact with the breakpoint objects > @@ -5082,6 +5085,10 @@ bpstat_check_watchpoint (bpstat bs) > { > e = watchpoint_check (bs); > } > + catch (const gdb_exception_quit &ex) > + { > + throw; > + } > catch (const gdb_exception &ex) Same question here. > { > exception_fprintf (gdb_stderr, ex, > @@ -5317,6 +5324,10 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread) > { > condition_result = breakpoint_cond_eval (cond); > } > + catch (const gdb_exception_quit &ex) > + { > + throw; > + } > catch (const gdb_exception &ex) Ditto. > { > exception_fprintf (gdb_stderr, ex, > @@ -14385,6 +14396,10 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition, > bpt->enable_state = bp_enabled; > update_watchpoint (w, 1 /* reparse */); > } > + catch (const gdb_exception_quit &ex) > + { > + throw; > + } > catch (const gdb_exception &e) Ditto. Same for the remainder of the patch.