[RFC] gdb could leave inferior running as a background process

[RFC] gdb could leave inferior running as a background process

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

Greetings,

Problem:
- gdb calls target_terminal_inferior(),
- performs other stuff, which could issue warnings. warning()
  switches gdb to the foreground.
- calls target_wait without switching the terminal back to inferior,
  which then prevents inferior from working.

Demonstration:

$ cat pp.c
int main()
{
 char buf[1];
 while (0 != read(0, buf, 1))
  write(1, buf, 1);
 return 0;
}

$ gcc -static pp.c   # -static is needed to trigger warning with
                     # my version of gcc and glibc


$ gdb ./a.out
GNU gdb 6.8
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-linux"...
(gdb) r
warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)

warning: (Internal error: pc 0x8048100 in read in psymtab, but not in symtab.)

warning: (Internal error: pc 0x8048100 in read in psymtab, but not in symtab.)


Program received signal SIGTTIN, Stopped (tty input).
0xffffe410 in __kernel_vsyscall ()
(gdb) c

Program received signal SIGTTIN, Stopped (tty input).
0xffffe410 in __kernel_vsyscall ()


Note: the 'in psymtab, but not in symtab' warning appears to be triggered
by a separate bug in gold, but that's irrelevant: there could be any
other warning, and the effect would be the same.

Proposed patch below.

Thanks,
-- 
Paul Pluzhnikov


--------------------------------------------------------------------------------
2008-04-21  Paul Pluzhnikov  <ppluzhnikov@google.com>

      * gdb/infrun.c (wait_for_inferior): Call
      target_terminal_inferior before blocking.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 777 bytes --]

2008-04-21  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * gdb/infrun.c (wait_for_inferior): Call
       target_terminal_inferior before blocking.


--- gdb/infrun.c.orig	2008-04-21 18:00:15.159567000 -0700
+++ gdb/infrun.c	2008-04-21 23:19:16.802206000 -0700
@@ -1023,12 +1023,14 @@ wait_for_inferior (int treat_exec_as_sig
      status mechanism. */
 
   registers_changed ();
 
   while (1)
     {
+      /* Make sure terminal is switched to inferior before waiting for it.  */
+      target_terminal_inferior ();
       if (deprecated_target_wait_hook)
 	ecs->ptid = deprecated_target_wait_hook (ecs->waiton_ptid, ecs->wp);
       else
 	ecs->ptid = target_wait (ecs->waiton_ptid, ecs->wp);
 
       if (treat_exec_as_sigtrap && ecs->ws.kind == TARGET_WAITKIND_EXECD)

Re: [RFC] gdb could leave inferior running as a background process

[Daniel Jacobowitz]

On Tue, Apr 22, 2008 at 07:41:20AM -0700, Paul Pluzhnikov wrote:
> 2008-04-21  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>       * gdb/infrun.c (wait_for_inferior): Call
>       target_terminal_inferior before blocking.

Sorry, don't think this will work reliably.  If the target tried to
read after it was resumed but before the call to target_wait, and GDB
had the terminal, then it will have a pending SIGTTIN already at this
point.

We can't call terminal_ours* after resuming.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] gdb could leave inferior running as a background process

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On Tue, Apr 22, 2008 at 8:55 AM, Daniel Jacobowitz <drow@false.org> wrote:

>  Sorry, don't think this will work reliably.  If the target tried to
>  read after it was resumed but before the call to target_wait, and GDB
>  had the terminal, then it will have a pending SIGTTIN already at this
>  point.

I see.

My preferred way of fixing this was to have warning() restore
terminal to inferior if it wasn't ours when warning was called,
but that's not how the rest of the code is structured.

Would attached patch work better then?

Thanks,
-- 
Paul Pluzhnikov


2008-04-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

     * gdb/target.h (target_resume): Call target_terminal_inferior before
     resuming inferior.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 584 bytes --]

2008-04-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

     * gdb/target.h (target_resume): Call target_terminal_inferior before
     resuming inferior.

--- ../../../vendor_src/gdb/gdb-6.8.x/gdb/target.h	2008-03-25 09:51:34.779643000 -0700
+++ gdb/target.h	2008-04-22 09:27:16.894610000 -0700
@@ -573,6 +573,7 @@ extern void target_disconnect (char *, i
 #define	target_resume(ptid, step, siggnal)				\
   do {									\
     dcache_invalidate(target_dcache);					\
+    target_terminal_inferior ();					\
     (*current_target.to_resume) (ptid, step, siggnal);			\
   } while (0)
 

Re: [RFC] gdb could leave inferior running as a background process

[Paul Pluzhnikov]

On Tue, Apr 22, 2008 at 9:46 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

>  Would attached patch work better then?

Actually, the second patch doesn't fix the problem by itself:
inferior is resumed, then warning is issued, then we block.

So both the first and second patches are needed.

Thanks,
-- 
Paul Pluzhnikov

Re: [RFC] gdb could leave inferior running as a background process

[Daniel Jacobowitz]

On Tue, Apr 22, 2008 at 10:51:29AM -0700, Paul Pluzhnikov wrote:
> On Tue, Apr 22, 2008 at 9:46 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> 
> >  Would attached patch work better then?
> 
> Actually, the second patch doesn't fix the problem by itself:
> inferior is resumed, then warning is issued, then we block.
> 
> So both the first and second patches are needed.

Any time the second patch is "needed", there's still a race condition
and the bug is present.  We're running in parallel with the inferior
at this point.

Where is the warning issued?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] gdb could leave inferior running as a background process

[Paul Pluzhnikov]

On Tue, Apr 22, 2008 at 11:04 AM, Daniel Jacobowitz <drow@false.org> wrote:

>  Any time the second patch is "needed", there's still a race condition
>  and the bug is present.  We're running in parallel with the inferior
>  at this point.

Indeed.

IOW, any time inferior is running, gdb should be prohibited
from calling terminal_ours (or warning). Save/restore terminal in
warning() itself isn't good enough ...

This protocol will probably be hard to enforce, and I wonder how
this will play with non-stop debugging ...

>  Where is the warning issued?

#0  warning (string=0x82e9520 "(Internal error: pc 0x%s in read in
psymtab, but not in symtab.)\n") at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/utils.c:615
#1  0x08112c5b in find_pc_sect_symtab (pc=134512896, section=0x0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/symtab.c:1984
#2  0x08112d79 in find_pc_sect_line (pc=134512896, section=0x0,
notcurrent=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/symtab.c:2140
#3  0x08112fcc in find_pc_line (pc=134512896, notcurrent=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/symtab.c:2262
#4  0x08128698 in init_execution_control_state (ecs=0xffffd05c) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infrun.c:1128
#5  0x08128344 in wait_for_inferior (treat_exec_as_sigtrap=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infrun.c:1012
#6  0x0812821a in proceed (addr=4294967295, siggnal=TARGET_SIGNAL_0,
step=0) at ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infrun.c:844
#7  0x08124a2f in run_command_1 (args=0x0, from_tty=0,
tbreak_at_main=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infcmd.c:563
#8  0x08124a56 in run_command (args=0x0, from_tty=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infcmd.c:570
#9  0x080c3303 in do_cfunc (c=0x83bfc00, args=0x0, from_tty=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/cli/cli-decode.c:60
#10 0x080c58ac in cmd_func (cmd=0x83bfc00, args=0x0, from_tty=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/cli/cli-decode.c:1663
#11 0x08085f81 in execute_command (p=0x83a3109 "", from_tty=1) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/top.c:449
#12 0x08139d7b in command_handler (command=0x83a3108 "") at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-top.c:518
#13 0x0813a588 in command_line_handler (rl=0x8405870 "�=\b") at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-top.c:804
#14 0x08209fac in rl_callback_read_char () at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/readline/callback.c:205
#15 0x0813945b in rl_callback_read_char_wrapper (client_data=0x0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-top.c:177
#16 0x08139c29 in stdin_event_handler (error=0, client_data=0x0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-top.c:431
#17 0x08138b11 in handle_file_event (event_file_desc=0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-loop.c:728
#18 0x081383cc in process_event () at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-loop.c:341
#19 0x08138415 in gdb_do_one_event (data=0x0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/event-loop.c:378
#20 0x0813564d in catch_errors (func=0x81383e1 <gdb_do_one_event>,
func_args=0x0, errstring=0x82d9ba9 "", mask=6) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/exceptions.c:513
#21 0x080d612b in tui_command_loop (data=0x0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/tui/tui-interp.c:153
#22 0x08135b5a in current_interp_command_loop () at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/interps.c:276
#23 0x0807ed7f in captured_command_loop (data=0x0) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/main.c:99
#24 0x0813564d in catch_errors (func=0x807ed74
<captured_command_loop>, func_args=0x0, errstring=0x82c2513 "",
mask=6) at ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/exceptions.c:513
#25 0x0807fd83 in captured_main (data=0xffffd670) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/main.c:882
#26 0x0813564d in catch_errors (func=0x807edb5 <captured_main>,
func_args=0xffffd670, errstring=0x82c2513 "", mask=6) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/exceptions.c:513
#27 0x0807fdb9 in gdb_main (args=0xffffd670) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/main.c:891
#28 0x0807ed67 in main (argc=2, argv=0xffffd724) at
../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/gdb.c:33



-- 
Paul Pluzhnikov

Re: [RFC] gdb could leave inferior running as a background process

[Doug Evans]

On Tue, Apr 22, 2008 at 12:55 PM, Daniel Jacobowitz <drow@false.org> wrote:
>  > >  Where is the warning issued?
>  >
>  > #0  warning (string=0x82e9520 "(Internal error: pc 0x%s in read in
>  > psymtab, but not in symtab.)\n") at
>  > ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/utils.c:615
>  > #1  0x08112c5b in find_pc_sect_symtab (pc=134512896, section=0x0) at
>  > ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/symtab.c:1984
>  > #2  0x08112d79 in find_pc_sect_line (pc=134512896, section=0x0,
>  > notcurrent=0) at
>  > ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/symtab.c:2140
>  > #3  0x08112fcc in find_pc_line (pc=134512896, notcurrent=0) at
>  > ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/symtab.c:2262
>  > #4  0x08128698 in init_execution_control_state (ecs=0xffffd05c) at
>  > ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infrun.c:1128
>  > #5  0x08128344 in wait_for_inferior (treat_exec_as_sigtrap=0) at
>  > ../../../google_vendor_src_branch/gdb/gdb-6.8.x/gdb/infrun.c:1012
>
>  Ugh.  Perhaps we can do this before resuming.

It seems like there are multiple places where this can happen.  E.g.
wait_for_inferior -> handle_inferior_event -> find_pc_partial_function
-> target_terminal_ours_for_output.

Re: [RFC] gdb could leave inferior running as a background process

[Daniel Jacobowitz]

On Tue, Apr 22, 2008 at 01:36:01PM -0700, Doug Evans wrote:
> It seems like there are multiple places where this can happen.  E.g.
> wait_for_inferior -> handle_inferior_event -> find_pc_partial_function
> -> target_terminal_ours_for_output.

But by then it's stopped, right?  As long as something makes sure we
give the terminal back after the warning it shouldn't be a problem.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] gdb could leave inferior running as a background process

[Doug Evans]

On Tue, Apr 22, 2008 at 1:39 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Tue, Apr 22, 2008 at 01:36:01PM -0700, Doug Evans wrote:
>  > It seems like there are multiple places where this can happen.  E.g.
>  > wait_for_inferior -> handle_inferior_event -> find_pc_partial_function
>  > -> target_terminal_ours_for_output.
>
>  But by then it's stopped, right?  As long as something makes sure we
>  give the terminal back after the warning it shouldn't be a problem.

Ah.  It's hard to reason about correctness in this part of gdb.  I can
see that keep_going calls target_terminal_inferior, but the code paths
are embedded in a big hairy state machine.  [I've seen what
wait_for_inferior used to be, things *have* improved though.]

Re: [RFC] gdb could leave inferior running as a background process

[Daniel Jacobowitz]

On Tue, Apr 22, 2008 at 03:08:49PM -0700, Doug Evans wrote:
> Ah.  It's hard to reason about correctness in this part of gdb.  I can
> see that keep_going calls target_terminal_inferior, but the code paths
> are embedded in a big hairy state machine.  [I've seen what
> wait_for_inferior used to be, things *have* improved though.]

Sadly, it's hard to get a grip on how much of it is necessary
somewhere.

-- 
Daniel Jacobowitz
CodeSourcery