From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id QOrNLyEib2W5IBgAWB0awg (envelope-from ) for ; Tue, 05 Dec 2023 08:14:09 -0500 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=BRkhke8t; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id BEFD41E0D2; Tue, 5 Dec 2023 08:14:09 -0500 (EST) 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 A8C0A1E00F for ; Tue, 5 Dec 2023 08:14:07 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5479C385C306 for ; Tue, 5 Dec 2023 13:14:07 +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 4EBEC3858C2A for ; Tue, 5 Dec 2023 13:13:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4EBEC3858C2A 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 4EBEC3858C2A 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=1701782035; cv=none; b=K7f2NLw3Dz/tvViPaqv/i8LuAXtodANCXUrt8eWntDMank0sHFW+9KsJ5P1FClRj0B2fq5DLGtygTRp2pvfODViWRAMOwI/lDriTRmi02rvFa7F0Hb+iTdyj9/fkYIUPuXA6lvQ5ocDd73zG/pVjasCB8SBynJRSKW1is4y6gKw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701782035; c=relaxed/simple; bh=nc4KuKTvP7Sw4Vh8Om0l9P4H7VGROOyc28kXWAbuM84=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=btNdmSUG5cd8L3g3JyVJHTNg2wmUpsimO4BqWG9+uaoQVoTjFq0Ie79Rrq5AtzAgYiVv+2IZ+5NtTCxfaCRDR30yYWfwps9D0TfwUnEeGwWDLgst7FWRFDxYM76wf4vLGhDUfPfvNCIMzEx/va+CyEtMwEom/iq5NZp4+BoaS3s= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1701782031; 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=AwQ7HLTQQvjyP4lrlxHT4ZnRubXV8vf4txUzsVNvP90=; b=BRkhke8tsFbfqbUlGCWXrJ79ThydvsphUUDMXxh45CJ/bJy7VfwliAojA2/xv9g656EHEW E5cpgUd88GpHt/m0gEMd8Ce5RRkxJp9EjZ0nuzFbItjjOY3OpydP59uI7z53ScsNoWRzWP 70flnMTa2Thrlc2on5ukJREuPxxHLbs= 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-107-KXpg6ic5P6u_or_YBFUl8g-1; Tue, 05 Dec 2023 08:13:50 -0500 X-MC-Unique: KXpg6ic5P6u_or_YBFUl8g-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-a1c989d460eso33402966b.1 for ; Tue, 05 Dec 2023 05:13:50 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701782029; x=1702386829; 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=AwQ7HLTQQvjyP4lrlxHT4ZnRubXV8vf4txUzsVNvP90=; b=C8sjLyhxryWGUObuCNmTMzEhuY40I4pbnLMz1NbpAafgtzbpO791r450ZC7MyFM4Qz XsBLjYEG1j7GO+BWA0G7YdERr0wfJPHuRB2Syes2IznESb7U0+tWTYZoBJGY28yDyXz7 LspDd4ENeU3iuu8PfnIZk6MToHnGjlDFWpr+uuqrv0NKGTGBmfn8bW4jQpqECLTUZCm0 4QWEVoOvtRzXFOBEXMg/RvF3VgU1MIeowZhpLnz8W+sA1RB1WiYl0jI1ZSy5OAQX8abe Euo26L6kESkFndO/G66rq5UAPjy82Tlb8olhbCcdtL9klXoBU5uGBlA8gd6fTja2Yl6b xRzQ== X-Gm-Message-State: AOJu0YxLMO0fd/OvSoAZ+l2m64NHd4pyyJib7Gq8UOrbrRH8oM+ObrOl KcQGOgDlws+s8sru8HyBf2kCuTteHk69W8N2QCQEmrMc/A97w5Vvp4y/6a6mLxsPzjs/xG0Lxys WB9/tkb4XjPNjlqJQ5LYUXemiprqj4A== X-Received: by 2002:a17:906:513:b0:a01:77ef:e887 with SMTP id j19-20020a170906051300b00a0177efe887mr4307743eja.64.1701782028851; Tue, 05 Dec 2023 05:13:48 -0800 (PST) X-Google-Smtp-Source: AGHT+IH04YQjInlfuqCVII4zADgIi2MalW3Key41gtbtxcbCHGiIWYOgI8F54aziDJhF/M7y8TpbZQ== X-Received: by 2002:a17:906:513:b0:a01:77ef:e887 with SMTP id j19-20020a170906051300b00a0177efe887mr4307731eja.64.1701782028267; Tue, 05 Dec 2023 05:13:48 -0800 (PST) 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 qh10-20020a170906ecaa00b00a1caa50feb3sm615050ejb.40.2023.12.05.05.13.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 05 Dec 2023 05:13:47 -0800 (PST) Message-ID: Date: Tue, 5 Dec 2023 14:13:46 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [External] : Re: [PATCH v3] 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: <123ee8d6-e6da-4227-ac7a-27d22041c20e@oracle.com> 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=-10.4 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, T_SCC_BODY_TEXT_LINE 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 On 17/11/2023 15:48, Partha Satapathy wrote: > On 11/6/2023 7:08 PM, Guinevere Larsen wrote: >> On 02/11/2023 19:27, Partha Satapathy wrote: >>> On 11/2/2023 11:54 PM, Partha Satapathy wrote: >>>> On 10/25/2023 9:24 PM, Guinevere Larsen wrote: >>>>> 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 some how missed this mail, hence a delay in reply. >>>> I sorry for this and unfortunately have made the mistake once again. >>>> Hope the extra threads can be deleted and I will keep my discussion >>>> bound to this thread. >>>> >>>>> >>>>> 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). >>>>> >>>> >>>> wait_sync_command_done() is not frequently called with command >>>> execution. >>>> strace -k -o log ./gdb -p <> >>>> (gdb) ls >>>> Undefined command: "ls".  Try "help". >>>> (gdb) !ls >>>> (gdb) disassemble main >>>> >>>> confirmed the function wait_sync_command_done() is not part of this >>>> trace. wait_sync_command_done() is called from run_inferior_call() >>>> and serve as inferior startup and wait for it to stop. >>>> >>>> /* Subroutine of call_function_by_hand to simplify it. >>>>     Start up the inferior and wait for it to stop. >>>>     Return the exception if there's an error, or an exception with >>>>     reason >= 0 if there's no error. >>>> >>>>     This is done inside a TRY_CATCH so the caller needn't worry about >>>>     thrown errors.  The caller should rethrow if there's an error.  */ >>>> >>>> static struct gdb_exception >>>> run_inferior_call (std::unique_ptr sm, >>>>                     struct thread_info *call_thread, CORE_ADDR >>>> real_pc) >>>> { >>>> >>>>        /* Inferior function calls are always synchronous, even if the >>>>           target supports asynchronous execution.  */ >>>>        wait_sync_command_done (); >>>> >>>> So wait_sync_command_done called once per inferior at startup. >>>> >>>> >>>> Hi Guinevere, >>>> >>>> Thanks for the review and sorry for the delay in reply. >>>> Please find comments inline. >>>> >>>> I will send the V2 incorporating rest of the comment. >>>> >>>> Thanks >>>> Partha >>> >> Hi! Thanks for the updated version. It looks much better! >> >> However, I still cant apply the patch. Are you sure you're developing >> on the master branch of our upstream repository? >> (https://urldefense.com/v3/__https://sourceware.org/git/binutils-gdb.git__;!!ACWV5N9M2RV99hQ!N_X8-tLG80n66yoOg95U0435CrvbbnDiHbebshHmNxivPGKLL5ZTy2le27VURzCGpKU6zzBqP4Jtu3d5km1jRA$ >> ) >> >> I have been manually changing the source code to test the patch, but >> it should cleanly apply for other maintainer to have an easier time >> reviewing things. >> >>> >>> Problem: While gdb is attaching an inferior, if ctrl-c is pressed in >>> the >>> middle of the process attach,  the sigint is passed to the debugged >>> process. This triggers the exit of the inferior. For example in pstack, >>> printing a stack can take significant time, and ctrl-c is pressed to >>> abort the 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 is to kill pstack/gdb, but not the inferior >>> application. >>> gdb -p <> >>> or gdb /proc/<>/exe pid >>> Attaching to process >>> << ctrl+c is pressed during attach >>> (gdb) q >>> <<<< inferior process exited >>>> >>> >>> A Ctrl-C/sigint received by gdb during the attachment of an inferior >>> passed to the debugged at some definite points during the window of >>> process attachment. The process of attaching an inferior is a multistep >>> process, and it takes time to get ready with the GDB prompt. As the >>> debugger and debugger are not fully attached during this period, the >>> sigint takes its default action to terminate the process. >>> >>> Solution: While GDB attaches processes, the inferior is not the current >>> session leader. Hence, until attach is complete and the GDB prompt is >>> available, the sigint should not be passed to the inferior. >>> The signal should be skipped if the process runs in the background. >>> With >>> this approach, we can skip passing the signature if the process is >>> attached to the GDB and the process attach is not complete. >>> --- >>>  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..d5d01bd0d09c 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 inferior has been fully synced and prompt is no longer >>> blocked.  */ >>> +  bool sync_flag = false; >>> + >>>    /* 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..4eff3130bad7 100644 >>> --- a/gdb/target.c >>> +++ b/gdb/target.c >>> @@ -3826,6 +3826,10 @@ struct target_ops * >>>                  through the target_stack.  */ >>>               scoped_restore_current_inferior restore_inferior; >>>               set_current_inferior (inf); >>> +             if ((current_inferior ()->attach_flag) >>> +                 && !(current_inferior ()->sync_flag)) >>> +                   return; >> also, there's still 8 spaces here. All 8 space-identations should be >> replaced with tabs. >>> + >>>               current_inferior ()->top_target ()->pass_ctrlc (); >>>               return; >>>             } >>> diff --git a/gdb/top.c b/gdb/top.c >>> index a685dbf5122e..f05fdd161a42 100644 >>> --- a/gdb/top.c >>> +++ b/gdb/top.c >>> @@ -542,6 +542,8 @@ struct ui_out ** >>>    while (gdb_do_one_event () >= 0) >>>      if (ui->prompt_state != PROMPT_BLOCKED) >>>        break; >>> + >>> +  current_inferior ()->sync_flag = true; >> >> I'm still not 100% convinced this is the best place to put this. >> Mainly because this function is also called by >> maybe_wait_sync_command_done; it didn't show up in your testing >> because when we run gdb from a terminal, the UI is synchronous (so it >> fails the first part of the IF condition), but this would be >> exercised in other situations. And maybe_wait_sync_command_done is >> called after every single command. >> >> I tried adding this to setup_inferior, which looked like the perfect >> place for it, but it unfortunately didn't work. Since done is better >> than perfect, I'm not going to block this patch on this, but I'd love >> to see a more logical place for this code. >> > > Hi Guinevere, > > Can't agree more on setup_inferior is the best place to reset this > variable. Updated V3 of review accordingly. Added a check_quit_flag > which will clear quit_flag, if set before setup_inferior. > > > Author: Partha Sarathi Satapathy > Date:   Fri Nov 17 11:42:11 2023 +0000 > > gdb : Signal to pstack/gdb kills the attached process. > > Problem: While gdb is attaching an inferior, if ctrl-c is pressed in the > middle of the process attach,  the sigint is passed to the debugged > process. This triggers the exit of the inferior. For example in pstack, > printing a stack can take significant time, and ctrl-c is pressed to > abort the 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 is to kill pstack/gdb, but not the inferior > application. > gdb -p <> > or gdb /proc/<>/exe pid > Attaching to process > << ctrl+c is pressed during attach > (gdb) q > <<<< inferior process exited >>>> > > A Ctrl-C/sigint received by gdb during the attachment of an inferior > passed to the debugged at some definite points during the window of > process attachment. The process of attaching an inferior is a multistep > process, and it takes time to get ready with the GDB prompt. As the > debugger and debugger are not fully attached during this period, the > sigint takes its default action to terminate the process. > > Solution: While GDB attaches processes, the inferior is not the current > session leader. Hence, until attach is complete and the GDB prompt is > available, the sigint should not be passed to the inferior. > The signal should be skipped if the process runs in the background. With > this approach, we can skip passing the signature if the process is > attached to the GDB and the process attach is not complete. Hi! Sorry about the delay on this. I think this is patch looks good to me, Reviewed-By: Guinevere Larsen I hope some maintainer for this area look at this soon! -- Cheers, Guinevere Larsen She/Her/Hers > --- >  gdb/infcmd.c   | 2 ++ >  gdb/inferior.h | 3 +++ >  gdb/target.c   | 3 +++ >  3 files changed, 8 insertions(+) > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index cf8cd527955..0aedbfc06b8 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -2556,6 +2556,8 @@ setup_inferior (int from_tty) >    target_post_attach (inferior_ptid.pid ()); > >    post_create_inferior (from_tty); > +  current_inferior ()->sync_flag = true; > +  check_quit_flag(); >  } > >  /* What to do after the first program stops after attaching.  */ > diff --git a/gdb/inferior.h b/gdb/inferior.h > index 33eff7a9141..4e517bf9bc4 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -600,6 +600,9 @@ class inferior : public refcounted_object, >    /* True if this child process was attached rather than forked. */ >    bool attach_flag = false; > > +  /* True if inferior has been fully synced and prompt is no longer > blocked */ > +  bool sync_flag = false; > + >    /* 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 a6ca7fc4f07..b5556eaeb5a 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -3811,6 +3811,9 @@ target_pass_ctrlc (void) >                  through the target_stack.  */ >               scoped_restore_current_inferior restore_inferior; >               set_current_inferior (inf); > +             if ((current_inferior ()->attach_flag) > +               && !(current_inferior ()->sync_flag)) > +                 return; >               current_inferior ()->top_target ()->pass_ctrlc (); >               return; >             } > -- > 2.39.3 > > > Thanks > Partha > >