From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by sourceware.org (Postfix) with ESMTPS id A86DD3858413 for ; Mon, 27 Sep 2021 17:39:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A86DD3858413 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-f50.google.com with SMTP id s24so1173990wmh.4 for ; Mon, 27 Sep 2021 10:39:15 -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=4RIQ0cICrnmhQRoiIzV68MkNvPSxKHkERwYd3/yrqso=; b=1y1rSJMfgUQniL4d+jiKcjKiTE7fuhG9CRgg+3wZQhnlOlMA3BKq1Q+bMKMGwglpeM 7kgLBrNJ+ppI71PV6tnoXVmp34E1EzMshvpLtiFEpYAk8/h5mxiIDKlctD2/0IUsa2zD lO3uCaIOiuE+Ub9Grr/gfl6uSlzPlcBYiJtMjSAu/eEjJ0y3NmLCgAD0x6yi6RWa6WiR tCwZmBQOUGrUzbmhKL4YeEI26rgaSPCrYgrr8wCvJkJGBJkseQdeviCRvrNS9BIUXxxF yWLx56X9ipKHwRxZ7mCWEwcB2qVgqxeNcx1uAjl0NP6CsPJPOcYFOBwZiQwTW/ZXFcu1 VQew== X-Gm-Message-State: AOAM531tCTPQFhaImt6tq3qGdgLQ5+CUK1jhm3n72NU5uEcbAoT/L0Ds EW3adZlXE0Gr7v7jXyUasaS7rqeCxqk= X-Google-Smtp-Source: ABdhPJwXVu2t4drzb5cAZJ49KIzOxcgY+QWQ9AVhlvIWNs/yCv4lmxgjahukmdXtTJ7KcRBxeHkV9w== X-Received: by 2002:a7b:ca45:: with SMTP id m5mr330066wml.104.1632764354194; Mon, 27 Sep 2021 10:39:14 -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 o17sm17125345wrj.96.2021.09.27.10.39.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 27 Sep 2021 10:39:13 -0700 (PDT) Subject: Re: [PATCH v2 2/6] Handle gdb SIGTERM via normal QUIT processing From: Pedro Alves To: Kevin Buettner , gdb-patches@sourceware.org References: <20210822231959.184061-1-kevinb@redhat.com> <20210822231959.184061-3-kevinb@redhat.com> Message-ID: Date: Mon, 27 Sep 2021 18:39:11 +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-3-kevinb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, 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 17:39:17 -0000 Hi Kevin, On 2021-08-23 12:19 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 indicated 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 ensures 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 (eventually) causes gdb_exception_quit to be thrown. This > exception (usually) propagates to the top level. At that point, > exception_print() is called to print the exception. 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." > > This commit implements the obvious part of Pedro's suggestion: > Instead of calling quit_force from quit(), throw_quit() is now called > instead. We leave sync_quit_force set for interrogation at a higher > level. With you up to here. > > At the top level, I found that the first thing done in each catch > block is to call exception_print(). Thus I placed a check to > see whether sync_quit_force is set within exception_print(). When > it's set, quit_force() will be called. But here, not so much. I don't think it's a good idea to do this within expection_print. For example, nothing prevents some low level code say within linux-nat.c to try/catch and print the exception, at a point again where we're very deep in the target stack and shouldn't assume much of global state. > > 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. It seemed likely to me that some catch > blocks might swallow a QUIT, not allowing it to get to the top level. > The rest of the patches in this series deal with this concern.