From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by sourceware.org (Postfix) with ESMTPS id DF5FC3858D1E for ; Mon, 30 Jan 2023 18:57:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DF5FC3858D1E 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-wm1-f45.google.com with SMTP id bg13-20020a05600c3c8d00b003d9712b29d2so10767712wmb.2 for ; Mon, 30 Jan 2023 10:57:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=knOfUEi/mlKe2VLZ6PAXvHpfkBwJBEDnTg/7/vYmYz8=; b=DWkDU5N5gmDxIoJ0xgcOTMBWaF5dM2Au2+3zCILbiouv4cfVXlBr0Ohz+K7wgPrdl6 NuP8t53J6L6m2ZTTUMxvHgI6t7v8kVkK4AlYg+dANMC0xP3T734nCSuh5fE9ULse2VFN Nk461PJLRqK340nC16RFhim41BsBMoFvw+/wKoaKzBvypImB/KJzxrRYYk4dyVCuKRpL 2pbqMqM7VDtd26RLexbv31osdKKk1907d7BDQfRynUFPrJsRYYgU94z6Bf/Oh0cvW5Xf Jc58yaZYtsH1yuxwi1m4YxM8yICfl/NxJeIfvfd+wZpxHRM5bQrRY+ptuxXARCSPG7EF fnNQ== X-Gm-Message-State: AO0yUKXrG3zlujXW9EfBn0o69Bq3ud4mJ8JitpFOrQOEltPYDCC/FZAj iBEP0GGsTc32TMIZyfeDfyQ= X-Google-Smtp-Source: AK7set94QHj0O/lhjU5BWqRF6c/7KYnwEEWorlqRup9P3bNisqvcoUU6vDNMageD3LmqPi+kTBZF/A== X-Received: by 2002:a05:600c:3b07:b0:3dc:5a0b:d256 with SMTP id m7-20020a05600c3b0700b003dc5a0bd256mr4171098wms.22.1675105063926; Mon, 30 Jan 2023 10:57:43 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id g7-20020a05600c308700b003dcc82ce53fsm2058557wmn.38.2023.01.30.10.57.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 30 Jan 2023 10:57:43 -0800 (PST) Subject: Re: [PATCH v4 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit To: Kevin Buettner , gdb-patches@sourceware.org Cc: simark@simark.ca, tdevries@suse.de References: <20230112015630.32999-1-kevinb@redhat.com> <20230112015630.32999-3-kevinb@redhat.com> From: Pedro Alves Message-ID: <90abf028-a86c-9813-fe8f-a199454415d6@palves.net> Date: Mon, 30 Jan 2023 18:57:42 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20230112015630.32999-3-kevinb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 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.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023-01-12 1:56 a.m., Kevin Buettner wrote: > When a GDB process receives the SIGTERM signal, handle_sigterm() in > event-top.c is called. The global variable 'sync_quit_force_run' is > set by this signal handler. It does some other things too, but the > setting of this global is the important bit for the SIGTERM part of > this discussion. > > GDB will periodically check to see whether a Ctrl-C or SIGTERM has > been received. This is performed via use of the QUIT macro in > GDB's code. QUIT is defined to invoke maybe_quit(), which will be > periodically called during any lengthy operation. This is supposed to > ensure that the user won't have to wait too long for a Ctrl-C or > SIGTERM to be acted upon. > > When a Ctrl-C / SIGINT is received, quit_handler() will decide whether > to pass the SIGINT onto the inferior or to call quit() which causes > gdb_exception_quit to be thrown. This exception (usually) propagates > to the top level. Control is then returned to the top level event > loop. > > At the moment, SIGTERM is handled very differently. Instead of > throwing an exception, quit_force() is called. This does eventually > cause GDB to exit(), but prior to that happening, the inferiors > are killed or detached and other target related cleanup occurs. > As shown in this discussion between Pedro Alves and myself... > > https://sourceware.org/pipermail/gdb-patches/2021-July/180802.html > https://sourceware.org/pipermail/gdb-patches/2021-July/180902.html > https://sourceware.org/pipermail/gdb-patches/2021-July/180903.html > > ...we found that it is possible for inferior_ptid and current_thread_ > to get out of sync. When that happens, the "current_thread_ != nullptr" > assertion in inferior_thread() can fail resulting in a GDB internal > error. > > Pedro recommended that we "let the normal quit exception propagate all > the way to the top level, and then have the top level call quit_force > if sync_quit_force_run is set." However, after the v2 series for this > patch set, we tweaked that idea by introducing a new exception for > handling SIGTERM. > > This commit implements the obvious part of Pedro's suggestion: > Instead of calling quit_force from quit(), throw_forced_quit() is now > called instead. This causes the new exception 'gdb_exception_forced_quit' > to be thrown. > > At the top level, I changed catch_command_errors() and captured_main() > to catch gdb_exception_forced_quit and then call quit_force() from the > catch block. I also changed start_event_loop() to also catch > gdb_exception_forced_quit; while we could also call quit_force() from > that catch block, it's sufficient to simply rethrow the exception > since it'll be caught by the newly added code in captured_main(). > > Making these changes fixed the failure / regression that I was seeing > for gdb.base/gdb-sigterm.exp when run on a machine with glibc-2.34. > However, there are many other paths back to the top level which this > test case does not test. I did an audit of all of the try / catch > code in GDB in which calls in the try-block might (eventually) call > QUIT. I found many cases where gdb_exception_quit and the new > gdb_exception_forced_quit will be swallowed. (When using GDB, have > you ever hit Ctrl-C and not have it do anything; if so, it could be > due to a swallowed gdb_exception_quit in one of the cases I've > identified.) The rest of the patches in this series deal with this > concern. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761 > --- > gdb/main.c | 12 ++++++++++++ > gdb/utils.c | 2 +- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/gdb/main.c b/gdb/main.c > index c04d37a45f9..323d99321c7 100644 > --- a/gdb/main.c > +++ b/gdb/main.c > @@ -410,6 +410,10 @@ start_event_loop () > { > result = gdb_do_one_event (); > } > + catch (const gdb_exception_forced_quit &ex) > + { > + throw; > + } > catch (const gdb_exception &ex) > { > exception_print (gdb_stderr, ex); > @@ -518,6 +522,10 @@ catch_command_errors (catch_command_errors_const_ftype command, > if (do_bp_actions) > bpstat_do_actions (); > } > + catch (const gdb_exception_forced_quit &e) > + { > + quit_force (NULL, 0); > + } > catch (const gdb_exception &e) > { > return handle_command_errors (e); > @@ -1309,6 +1317,10 @@ captured_main (void *data) > { > captured_command_loop (); > } > + catch (const gdb_exception_forced_quit &ex) > + { > + quit_force (NULL, 0); > + } Tabs vs spaces above. Otherwise this LGTM. Pedro Alves > catch (const gdb_exception &ex) > { > exception_print (gdb_stderr, ex); > diff --git a/gdb/utils.c b/gdb/utils.c > index 734c5bf7f70..0e126cf103b 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -673,7 +673,7 @@ quit (void) > if (sync_quit_force_run) > { > sync_quit_force_run = 0; > - quit_force (NULL, 0); > + throw_forced_quit ("SIGTERM"); > } > > #ifdef __MSDOS__ >