diff mbox series

Handle EINTR in run_under_shell

Message ID 20241014144926.10842-1-tdevries@suse.de
State Committed
Headers
Series Handle EINTR in run_under_shell |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Oct. 14, 2024, 2:49 p.m. UTC 
  When building gdb with -O2 -fsanitize=thread and running test-case
gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
...
(gdb) c&^M
Continuing.^M
(gdb) Quit^M
(gdb) quit_count=1
^M
Breakpoint 2, foo () at bg-exec-sigint-bp-cond.c:23^M
23        return 0;^M
FAIL: $exp: no force memory write: \
  SIGINT does not interrupt background execution
...

What happens is that:
- the breakpoint hits
- while evaluating the condition of the breakpoint,
  $_shell("kill -INT <pid-of-gdb>") is called, handled by run_under_shell
- in run_under_shell, a vfork is issued
- in the vfork child, execl executes the kill command
- in the vfork parent, waitpid is called to wait for the result of the kill
  command
- waitpid returns -1 with errno set to EINTR
- run_under_shell doesn't check the result of waitpid, and returns the
  value of local variable status.  Since waitpid returned -1, status was
  not assigned a value, so it's uninitialized, and happens to be
  non-zero
- the breakpoint condition evaluates to true, because
  $_shell("kill -INT <pid-of-gdb>") != 0
- the breakpoint triggers a stop, which the test-case doesn't expect.

Fix this by using gdb::handle_eintr to call waitpid in run_under_shell.

Also handle the case that waitpid returns an error other than EINTR, using
perror_with_name.

Tested on x86_64-linux.

PR gdb/30695
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30695
---
 gdb/cli/cli-cmds.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


base-commit: 22c62092858e5623338a18a42491718d754977e8

Comments

Tom Tromey Oct. 21, 2024, 5:44 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by using gdb::handle_eintr to call waitpid in run_under_shell.

Tom> Also handle the case that waitpid returns an error other than EINTR, using
Tom> perror_with_name.

This seems fine but I wonder if the other calls to waitpid need this treatment.
I see linux defines my_waitpid as wrapper... like, should that be used everywhere?

Tom> PR gdb/30695
Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30695

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Tom de Vries Oct. 22, 2024, 7:13 a.m. UTC | #2 
On 10/21/24 19:44, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this by using gdb::handle_eintr to call waitpid in run_under_shell.
> 
> Tom> Also handle the case that waitpid returns an error other than EINTR, using
> Tom> perror_with_name.
> 

Hi Tom,

thanks for the review, pushed.

> This seems fine but I wonder if the other calls to waitpid need this treatment.
> I see linux defines my_waitpid as wrapper... like, should that be used everywhere?
> 

I started to look at this, spent some time trying to rewrite the 
template because I didn't like the look of it, and then started 
wondering whether the loop needs to call QUIT or not.

I suppose it shouldn't matter, because for the automatic restart case we 
don't check it either.

I'll try to come up with a patch series.

Thanks,
- Tom

> Tom> PR gdb/30695
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30695
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom
Tom de Vries Oct. 25, 2024, 4:55 p.m. UTC | #3 
On 10/22/24 09:13, Tom de Vries wrote:
> On 10/21/24 19:44, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> Fix this by using gdb::handle_eintr to call waitpid in 
>> run_under_shell.
>>
>> Tom> Also handle the case that waitpid returns an error other than 
>> EINTR, using
>> Tom> perror_with_name.
>>
> 
> Hi Tom,
> 
> thanks for the review, pushed.
> 
>> This seems fine but I wonder if the other calls to waitpid need this 
>> treatment.
>> I see linux defines my_waitpid as wrapper... like, should that be used 
>> everywhere?
>>
> 
> I started to look at this, spent some time trying to rewrite the 
> template because I didn't like the look of it, and then started 
> wondering whether the loop needs to call QUIT or not.
> 
> I suppose it shouldn't matter, because for the automatic restart case we 
> don't check it either.
> 
> I'll try to come up with a patch series.
> 

Submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2024-October/212623.html ).

Thanks,
- Tom

>> Tom> PR gdb/30695
>> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30695
>>
>> Approved-By: Tom Tromey <tom@tromey.com>
>>
>> Tom
>
diff mbox series

Patch

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index ea2e156a00c..65ac7d6e7fb 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -55,6 +55,7 @@ 
 #include "extension.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/gdb_tilde_expand.h"
+#include "gdbsupport/eintr.h"
 
 #ifdef TUI
 #include "tui/tui.h"
@@ -921,7 +922,11 @@  run_under_shell (const char *arg, int from_tty)
     }
 
   if (pid != -1)
-    waitpid (pid, &status, 0);
+    {
+      int ret = gdb::handle_eintr (-1, ::waitpid, pid, &status, 0);
+      if (ret == -1)
+	perror_with_name ("Cannot get status of shell command");
+    }
   else
     error (_("Fork failed"));
   return status;