From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id TLmBADI6OWWMojYAWB0awg (envelope-from ) for ; Wed, 25 Oct 2023 11:54:26 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=BzwH1un0; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id E65EE1E0C1; Wed, 25 Oct 2023 11:54:25 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id C3FEF1E00F for ; Wed, 25 Oct 2023 11:54:23 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2FD41385841B for ; Wed, 25 Oct 2023 15:54:23 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 87E803858D1E for ; Wed, 25 Oct 2023 15:54:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 87E803858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 87E803858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698249251; cv=none; b=hTbChmLoM58GxSDVhazWKulIlgQQl/JLmpZQjUf+qO+8cOuF4spC1FcpaRaPSlq41rJnfgg6pMOCzS5bzPHbGQyMPm2zZi4YOpZfzhkJ8sC4t44rgz/C9LL+JrWplJH95qQctjDHwMpTJgV3+e87uP7fh7Gev3pwN0BHDN191co= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698249251; c=relaxed/simple; bh=lWyLakcH2zgywSg2dqmZdLwtm5wupUrhBK2sijzRhOs=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Gkefm+lo1dOqkHYSBxh5zgLYsHybHQADQfcBwfr6j5b6UFYGUDr6Wd30rkb0rTqgxXKhp4YT5UQYc4A/9+7+kqZe2f8fp5MBd4Ur0yast1ym5YzAO/Jt6OJn/Yxd7fikERr/p8irwYBk0sUJtQ7bYGwy8Xug+GyK7Z8pLrdZoRg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698249249; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=bvd3+pbL3dbeyOzjjfQ4CZhZC3eVe+YntqmXFuDjGtQ=; b=BzwH1un0Kgo1XK2b64BiH1vwDSI9quuZ//8DE5THdu0fsGAbEAOP8fhSls7vYSGZ1juU8o 0BURa1kJs6v0xnT0H3eFlv1zMbP2ran7lmrb7MaAQhFPIaDXFafbMKuiJRTKZymgpF1lME 4DjLJS68hH0rthe1dbNaZcYASN8DDi0= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-211-xY-GsjnbM-aM0Ul8R6j06g-1; Wed, 25 Oct 2023 11:54:07 -0400 X-MC-Unique: xY-GsjnbM-aM0Ul8R6j06g-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-9bf1047cb28so357744366b.2 for ; Wed, 25 Oct 2023 08:54:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698249246; x=1698854046; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bvd3+pbL3dbeyOzjjfQ4CZhZC3eVe+YntqmXFuDjGtQ=; b=vcwaYQKqRbPxph69zvRe6ijJz15YP65Md5PIlsW1bkRBoCPLt5uMAhAU9zGFGD4Sez Lv6KqCDsNv5zNAf2JfFO65zaSKX9+KB/FADnj7c2smTSHZo3CKx5OMb9fE3cERYZGSlN m9DWTgSHTB7Vx21WTFECvjTE68bxKhX9Too23mukFiMXUYg+hjxLwPlU9XQ+zu/+P7d+ G/zoUZQwz8/5M/LjD1v7sghawVmjqCtXAjYRH8TRMbw4P7Ekxjbf3PINaI97cf128JkG LrcWRrAXATu05NMLJzVstE0zijKaGU+mx5xIdyzBoHdWf4FixtQTUdT8imnOt9dYtXy8 /h1w== X-Gm-Message-State: AOJu0YyBukcgOghRZ1mgrr3j7Pkwasgk7DnESOUn+4KMhApMgCkqVsd2 93eIN+04Ah0nSyzQ45JJs9tzO1AimzVrTWZqDSUeQN7RChCdO1xR0qN7JTT3Zxnu1NltE1gLaA6 nFqvJqPMCE49ttIPAHA1hCw== X-Received: by 2002:a17:907:608a:b0:9be:6ccb:6a8f with SMTP id ht10-20020a170907608a00b009be6ccb6a8fmr13087656ejc.48.1698249246462; Wed, 25 Oct 2023 08:54:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHug3XQnEKq981qVceBd+kl2oiOn+flyAuoOlU+NTMrWnW29B0XuHKeXhSU7t+LIYlsYrBtnA== X-Received: by 2002:a17:907:608a:b0:9be:6ccb:6a8f with SMTP id ht10-20020a170907608a00b009be6ccb6a8fmr13087640ejc.48.1698249246020; Wed, 25 Oct 2023 08:54:06 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-227-180.bb.vodafone.cz. [94.112.227.180]) by smtp.gmail.com with ESMTPSA id x22-20020a170906711600b0099b7276235esm10092185ejj.93.2023.10.25.08.54.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Oct 2023 08:54:05 -0700 (PDT) Message-ID: Date: Wed, 25 Oct 2023 17:54:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH] gdb : Signal to pstack/gdb kills the attached process. To: Partha Satapathy , gdb-patches@sourceware.org, bert.barbe@oracle.com, rajesh.sivaramasubramaniom@oracle.com References: From: Guinevere Larsen In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hi! Thanks for working on this issue, and sorry about the delay in getting this reviewed. For future reference, we (at least I) tend to try and go for patches with many pings, so it is better to ping existing patches than re-sending them :) I'm not very knowledgeable on how GDB does signal handling, so I'm going to review this patch at face value. I hope someone who does know how this part works gets a look at this soon! On 16/10/2023 11:28, Partha Satapathy wrote: > Problem : > While gdb attaching a target, If ctrl-c pressed in the midst of the > process attach,  the sigint is passed to the debugged process. This > triggers exit of the debugged. > > Let’s take the example of pstack,  which dumps the stack of all > threads in a process. In some cases printing of stack can take > significant time and ctrl-c is pressed to abort pstack/gdb > application. This in turn kills the debugged process, which can be  > critical for the system. In this case the intention of “ctrl+c” to > kill pstack/gdb, but not the target application. > > Reproduction: > > The debugged application generally attached to process by: > gdb -p <> > or gdb /proc/<>/exe pid > pstack uses the latter  method to attach the debugged to gdb. If the > application is large or process of reading symbols is slow, gives a > good window to press the ctrl+c during attach. Spawning "gdb" under > "strace -k" makes gdb a lot slower and gives a larger window to easily > press the > ctrl+c at the precise period i.e. during the attach of the debugged > process. The above strace hack will enhance rate of reproduction of > the issue. Testcase: > > With GDB 13.1 > ps aux | grep abrtd > root     2195168   /usr/sbin/abrtd -d -s > > #strace -k -o log gdb -p 2195168 > Attaching to process 2195168 > [New LWP 2195177] > [New LWP 2195179] > ^C[Thread debugging using libthread_db enabled] > <<<<   Note the ctrl+c is pressed after attach is initiated and it’s > still reading the symbols from library >>>> Using host libthread_db > library "/lib64/libthread_db.so.1". > 0x00007fe3ed6d70d1 in poll () from /lib64/libc.so.6 > (gdb) q > A debugging session is active. >           Inferior 1 [process 2195168] will be detached Quit anyway? > (y or n) y Detaching from program: /usr/sbin/abrtd, process 2195168 > > # ps aux | grep 2195168 > <<<< Process exited >>>> > > Description: > > We are installing a signal handler in gdb that marks the Ctrl-c/sigint > received by gdb. GDB passes this sigint to the debugged at some > definite points during the window of process attach. The process of > attaching debugged  involves steps like PTRACE_ATTACH , reading > symbols, getting the stop signal from the debugged and get ready with > GDB prompt. Note: > one of the example of this is sigint passing is: > "     - installs a SIGINT handler that forwards SIGINT to the inferior. >          Otherwise a Ctrl-C pressed just while waiting for the initial >          stop would end up as a spurious Quit. > " > > There are few other places where sigint is passed to the debugged > during attach of process to gdb. As the debugger and debugged are not > fully attached during this period, the sigint takes its default action > and terminates the process. > > Solution: > > While gdb attaches process, the target is not the current session > leader. Hence, until attach is complete and GDB prompt is availed, the > sigint should not be passed to the debugged. A similar approach is > taken for "gdb) run &". In target_terminal::inferior() >     /* A background resume (``run&'') should leave GDB in control of the >        terminal.  */ >     if (ui->prompt_state != PROMPT_BLOCKED) >       return; > > The passing of signal is skipped if the process ran in background.  > With this approach we can skip passing the sigint if the process is > attached to gdb and process attach is not complete. > Here is the proposed solution: > > > > Fix : > > While gdb attaching a target, If ctrl-c/sigint pressed in the midst of > the process attach, the sigint is passed to the debugged process. > This triggers exit of the debugged. > > This issue is evident while getting the process stack with ./gdb > --quiet -nx  -ex 'set width 0' -ex 'set height 0' > -ex 'set pagination no' -ex 'set confirm off' > -ex 'thread apply all bt' -ex quit /proc//exe and press the > ctrl+c while attach. > > The above method is also used in pstack application which is a wrapper > over gdb to print the process stack. A Ctrl+C intended to kill gdb or > pstack, but kills the debugged even if it is attached and not spawned > by gdb. This is a very good description of the error you've encountered, but given the repetition on this "fix:" part, I'm wondering, what is meant to be the commit message? Is it just these last few lines, or is it the whole thing? If it is just this last bit, I think it would benefit from some more explanation of the solution. If it is the whole message, I think you can reduce a bit the repetition. Also, at many points you say "debugged process" and "target". In GDB-land we call that the "inferior". Target has a very specific meaning in the context of GDB (roughly the CPU you're running, and some extra bits here and there). I also have a few comments on the specific changes, that are inlined. > --- >   gdb/inferior.h | 3 +++ >   gdb/target.c   | 4 ++++ >   gdb/top.c      | 2 ++ >   3 files changed, 9 insertions(+) > > diff --git a/gdb/inferior.h b/gdb/inferior.h index > 4d001b0ad50e..b7048d10bbe4 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -557,6 +557,9 @@ class inferior : public refcounted_object, >     /* True if this child process was attached rather than forked.  */ >     bool attach_flag = false; > > +  /* True if target process synced and gdb ui is out of block. */ bool This comment is oddly worded. Based on the change to gdb/top.c, I think you could reword it like this: /* True if inferior has been fully synced and the prompt is no longer blocked.  */ > + sync_flag = false; Typo here, the variable's type should be on this line. > + >     /* If this inferior is a vfork child, then this is the pointer to >        its vfork parent, if GDB is still attached to it.  */ >     inferior *vfork_parent = NULL; > diff --git a/gdb/target.c b/gdb/target.c index > d5bfd7d0849b..f7c115497451 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -3826,6 +3826,10 @@ target_pass_ctrlc (void) >                   through the target_stack.  */ >                scoped_restore_current_inferior restore_inferior; >                set_current_inferior (inf); > +             if ((current_inferior()->attach_flag) && A couple of style issues here: when the indentation would have 8 spaces, you should use a tab instead; There should be a space between the function name and the parameters; And when you need to cut a logical expression in half, the operator should be at the start of a new line. > + !(current_inferior()->sync_flag)) { In this case, since it is just one line, there is no need to have the curly braces. However, when they are needed, they should be on the following line, and 2 spaces further in indentation. > + return; > +             } >                current_inferior ()->top_target ()->pass_ctrlc (); >                return; >              } > diff --git a/gdb/top.c b/gdb/top.c > index 621aa6883233..26cc6caac0e5 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -542,6 +542,8 @@ wait_sync_command_done (void) >     while (gdb_do_one_event () >= 0) >       if (ui->prompt_state != PROMPT_BLOCKED) >         break; > + > +  current_inferior()->sync_flag = true; I'm not very knowledgeable on this part of GDB, so take this with a grain of salt, but I wonder if this is the best place to put this. Since you only set this flag as false when first creating the inferior structure, I don't see why it should be re-set every time we're waiting for a command to be done. You could set the sync flag to false every command, but that feels like overkill. I feel like there should be some a mechanism in GDB already that knows if we're the session leader or not, and thus handles things correctly, but I don't know what it is. Another possibility, based on the exact problem you had, is to put this at the end of either symbol expansions, or the reasons they are being expanded in the first place (which I suspect is something like trying to identify the language or  name of the main function). -- Cheers, Guinevere Larsen She/Her/Hers >   } > >   /* See top.h.  */ > -- > 2.39.3 >