[PATCH] gdb : Signal to pstack/gdb kills the attached process.

[PATCH] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

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 <<pid>>
or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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.
---
   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
+ 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..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) &&
+                             !(current_inferior()->sync_flag)) {
+                     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;
   }

   /* See top.h.  */
--
2.39.3

Re: [PATCH] gdb : Signal to pstack/gdb kills the attached process.

[Guinevere Larsen]

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 <<pid>>
> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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
>

Re: [External] : Re: [PATCH] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

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 <<pid>>
>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
(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<call_thread_fsm> 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

Re: [External] : Re: [PATCH v2] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

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 <<pid>>
>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
> (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<call_thread_fsm> 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


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 <<pid>>
or gdb /proc/<<pid>>/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;
+
               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;
  }

  /* See top.h.  */
--
1.8.3.1

Re: [External] : Re: [PATCH v2] gdb : Signal to pstack/gdb kills the attached process.

[Guinevere Larsen]

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 <<pid>>
>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>> (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<call_thread_fsm> 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://sourceware.org/git/binutils-gdb.git)

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 <<pid>>
> or gdb /proc/<<pid>>/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.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

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 <<pid>>
>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>> (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<call_thread_fsm> 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 <<pid>>
>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
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 <<pid>>
or gdb /proc/<<pid>>/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/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

RE: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

Hi Guinevere,

Gentle reminder for the review on this issue.

Thanks
Partha

-----Original Message-----
From: Partha Satapathy <partha.satapathy@oracle.com> 
Sent: Friday, November 17, 2023 8:18 PM
To: Guinevere Larsen <blarsen@redhat.com>; gdb-patches@sourceware.org; Bert Barbe <bert.barbe@oracle.com>; Rajesh Sivaramasubramaniom <rajesh.sivaramasubramaniom@oracle.com>
Subject: Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

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 <<pid>>
>>>>> or gdb /proc/<<pid>>/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 
>>>>> ctrl+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/<PID>/exe <PID> 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 <<pid>>
>>> (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<call_thread_fsm> 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.g
> it__;!!ACWV5N9M2RV99hQ!N_X8-tLG80n66yoOg95U0435CrvbbnDiHbebshHmNxivPGK
> LL5ZTy2le27VURzCGpKU6zzBqP4Jtu3d5km1jRA$ )
> 
> 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 <<pid>>
>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
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 <<pid>>
or gdb /proc/<<pid>>/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/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

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Guinevere Larsen]

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 <<pid>>
>>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>>> (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<call_thread_fsm> 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 <<pid>>
>>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
> 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 <<pid>>
> or gdb /proc/<<pid>>/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 <blarsen@redhat.com>

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
>
>

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 12/5/2023 6:43 PM, Guinevere Larsen wrote:
> 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 <<pid>>
>>>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>>>> (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<call_thread_fsm> 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 <<pid>>
>>>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
>> 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 <<pid>>
>> or gdb /proc/<<pid>>/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 <blarsen@redhat.com>
> 
> I hope some maintainer for this area look at this soon!
> 

Hi Team,

Great if we can get a update on further proceedings on this.

Thanks
Partha

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 1/10/2024 9:29 PM, Partha Satapathy wrote:
> On 12/5/2023 6:43 PM, Guinevere Larsen wrote:
>> 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 <<pid>>
>>>>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>>>>> (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<call_thread_fsm> 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 <<pid>>
>>>>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
>>> 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 <<pid>>
>>> or gdb /proc/<<pid>>/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 <blarsen@redhat.com>
>>
>> I hope some maintainer for this area look at this soon!
>>
> 
> Hi Team,
> 
> Great if we can get a update on further proceedings on this.
> 
> Thanks
> Partha

Hi Guinevere and Team,

Great if we can have further update on this.
One more thing notice the online thread for this issue:
https://sourceware.org/pipermail/gdb-patches/2023-November/204251.html
is missing last couple of communications.

Thanks
Partha

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 1/24/2024 8:49 PM, Partha Satapathy wrote:
> On 1/10/2024 9:29 PM, Partha Satapathy wrote:
>> On 12/5/2023 6:43 PM, Guinevere Larsen wrote:
>>> 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 <<pid>>
>>>>>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>>>>>> (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<call_thread_fsm> 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 <<pid>>
>>>>>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
>>>> 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 <<pid>>
>>>> or gdb /proc/<<pid>>/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 <blarsen@redhat.com>
>>>
>>> I hope some maintainer for this area look at this soon!
>>>
>>
>> Hi Team,
>>
>> Great if we can get a update on further proceedings on this.
>>
>> Thanks
>> Partha
> 
> Hi Guinevere and Team,
> 
> Great if we can have further update on this.
> One more thing notice the online thread for this issue:
> https://sourceware.org/pipermail/gdb-patches/2023-November/204251.html
> is missing last couple of communications.
> 
> Thanks
> Partha

Hi Team,

Can you please help with further proceedings on this.

Thanks
Partha

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Guinevere Larsen]

On 19/02/2024 06:10, Partha Satapathy wrote:
> On 1/24/2024 8:49 PM, Partha Satapathy wrote:
>> On 1/10/2024 9:29 PM, Partha Satapathy wrote:
>>> On 12/5/2023 6:43 PM, Guinevere Larsen wrote:
>>>> 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 <<pid>>
>>>>>>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>>>>>>> (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<call_thread_fsm> 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 <<pid>>
>>>>>>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
>>>>> 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 <<pid>>
>>>>> or gdb /proc/<<pid>>/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 <blarsen@redhat.com>
>>>>
>>>> I hope some maintainer for this area look at this soon!
>>>>
>>>
>>> Hi Team,
>>>
>>> Great if we can get a update on further proceedings on this.
>>>
>>> Thanks
>>> Partha
>>
>> Hi Guinevere and Team,
>>
>> Great if we can have further update on this.
>> One more thing notice the online thread for this issue:
>> https://sourceware.org/pipermail/gdb-patches/2023-November/204251.html
>> is missing last couple of communications.
>>
>> Thanks
>> Partha
>
> Hi Team,
>
> Can you please help with further proceedings on this.
>
> Thanks
> Partha
>
Hi Partha! I'm sorry this is taking so long. I would suggest that you 
rebase your patch on the master branch and send a version 4 which is 
just the rebase to ping this. You also don't need to keep our previous 
comments on the v4 email, since folks will be able to see this history, 
and that will make it easier for maintainers to review your code

If you do send a v4, remember to add my review tag to the end of the 
commit message :)

I hope this gets an approval soon.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 3/5/2024 2:17 PM, Guinevere Larsen wrote:
> On 19/02/2024 06:10, Partha Satapathy wrote:
>> On 1/24/2024 8:49 PM, Partha Satapathy wrote:
>>> On 1/10/2024 9:29 PM, Partha Satapathy wrote:
>>>> On 12/5/2023 6:43 PM, Guinevere Larsen wrote:
>>>>> 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 <<pid>>
>>>>>>>>>>> or gdb /proc/<<pid>>/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/<PID>/exe <PID> 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 <<pid>>
>>>>>>>>> (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<call_thread_fsm> 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 <<pid>>
>>>>>>>> or gdb /proc/<<pid>>/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 <partha.satapathy@oracle.com>
>>>>>> 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 <<pid>>
>>>>>> or gdb /proc/<<pid>>/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 <blarsen@redhat.com>
>>>>>
>>>>> I hope some maintainer for this area look at this soon!
>>>>>
>>>>
>>>> Hi Team,
>>>>
>>>> Great if we can get a update on further proceedings on this.
>>>>
>>>> Thanks
>>>> Partha
>>>
>>> Hi Guinevere and Team,
>>>
>>> Great if we can have further update on this.
>>> One more thing notice the online thread for this issue:
>>> https://urldefense.com/v3/__https://sourceware.org/pipermail/gdb-patches/2023-November/204251.html__;!!ACWV5N9M2RV99hQ!IQdn9yPks2xKcRBSz9b4tX70VzdhNoeJeDSZcP_19T3lqehiY8L3kI4fu0TrxTl30v7JqxkkW_XHAA77K1CGhg$ is missing last couple of communications.
>>>
>>> Thanks
>>> Partha
>>
>> Hi Team,
>>
>> Can you please help with further proceedings on this.
>>
>> Thanks
>> Partha
>>
> Hi Partha! I'm sorry this is taking so long. I would suggest that you 
> rebase your patch on the master branch and send a version 4 which is 
> just the rebase to ping this. You also don't need to keep our previous 
> comments on the v4 email, since folks will be able to see this history, 
> and that will make it easier for maintainers to review your code
> 
> If you do send a v4, remember to add my review tag to the end of the 
> commit message :)
> 
> I hope this gets an approval soon.
> 

Author: Partha Sarathi Satapathy <partha.satapathy@oracle.com>
Date:   Fri Nov 17 09:18:56 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 <<pid>>
or gdb /proc/<<pid>>/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.

Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
---
  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 c1fdbb300c67..8ab2d50477f4 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2528,6 +2528,8 @@ enum async_reply_reason
    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 7be28423aeb1..a6065da19a87 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 bbc1badc9e19..7c7df9c2ed87 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3794,6 +3794,9 @@ 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;
               current_inferior ()->top_target ()->pass_ctrlc ();
               return;
             }
--
1.8.3.1


Hi Team,

Here is the V4 for review and its a rebase to latest.

Thanks
Partha

Re: [External] : Re: [PATCH v4] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

Author: Partha Sarathi Satapathy <partha.satapathy@oracle.com>
Date:   Fri Nov 17 09:18:56 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 <<pid>>
or gdb /proc/<<pid>>/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.

Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
---
  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 c1fdbb300c67..8ab2d50477f4 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2528,6 +2528,8 @@ enum async_reply_reason
    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 7be28423aeb1..a6065da19a87 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 bbc1badc9e19..7c7df9c2ed87 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3794,6 +3794,9 @@ 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;
               current_inferior ()->top_target ()->pass_ctrlc ();
               return;
             }
-- 
1.8.3.1


Hi Team,

Here is the V4 for review and its a rebase to latest.

Thanks
Partha

Re: [External] : Re: [PATCH v4] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 3/7/2024 3:28 PM, Partha Satapathy wrote:
> Author: Partha Sarathi Satapathy <partha.satapathy@oracle.com>
> Date:   Fri Nov 17 09:18:56 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 <<pid>>
> or gdb /proc/<<pid>>/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.
> 
> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
> ---
>   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 c1fdbb300c67..8ab2d50477f4 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2528,6 +2528,8 @@ enum async_reply_reason
>     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 7be28423aeb1..a6065da19a87 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 bbc1badc9e19..7c7df9c2ed87 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3794,6 +3794,9 @@ 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;
>                current_inferior ()->top_target ()->pass_ctrlc ();
>                return;
>              }


Hi Team,

Can we have further proceedings on this.

Thanks
Partha

Re: [External] : Re: [PATCH v4] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 3/7/2024 3:28 PM, Partha Satapathy wrote:
> Author: Partha Sarathi Satapathy <partha.satapathy@oracle.com>
> Date:   Fri Nov 17 09:18:56 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 <<pid>>
> or gdb /proc/<<pid>>/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.
> 
> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
> ---
>   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 c1fdbb300c67..8ab2d50477f4 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2528,6 +2528,8 @@ enum async_reply_reason
>     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 7be28423aeb1..a6065da19a87 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 bbc1badc9e19..7c7df9c2ed87 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3794,6 +3794,9 @@ 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;
>                current_inferior ()->top_target ()->pass_ctrlc ();
>                return;
>              }

Re: [External] : Re: [PATCH v4] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy]

On 3/26/2024 3:29 PM, Partha Satapathy wrote:
> On 3/7/2024 3:28 PM, Partha Satapathy wrote:
>> Author: Partha Sarathi Satapathy <partha.satapathy@oracle.com>
>> Date:   Fri Nov 17 09:18:56 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 <<pid>>
>> or gdb /proc/<<pid>>/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.
>>
>> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
>> ---
>>   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 c1fdbb300c67..8ab2d50477f4 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -2528,6 +2528,8 @@ enum async_reply_reason
>>     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 7be28423aeb1..a6065da19a87 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 bbc1badc9e19..7c7df9c2ed87 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -3794,6 +3794,9 @@ 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;
>>                current_inferior ()->top_target ()->pass_ctrlc ();
>>                return;
>>              }
> 

Hi GDB Team,

Can you update on this.

Thanks
Partha