[2/3] partially fix PR gdb/17130

Message ID 1405096676-22681-3-git-send-email-tromey@redhat.com
State Committed
Headers

Commit Message

Tom Tromey July 11, 2014, 4:37 p.m. UTC
  This is a partial fix for PR gdb/17130.

The bug is that some code in utils.c was not updated during the target
delegation change:

  if (job_control
      /* If there is no terminal switching for this target, then we can't
         possibly get screwed by the lack of job control.  */
      || current_target.to_terminal_ours == NULL)
    fatal ("Quit");
  else
    fatal ("Quit (expect signal SIGINT when the program is resumed)");

After the delegation change, to_terminal_ours will never be NULL.

I think this bug can be seen before the target delegation change by
enabling target debugging -- this would also cause to_terminal_ours to
be non-NULL.

The fix is to introduce a new target_supports_terminal_ours function,
that properly checks the target stack.  This is not perhaps ideal, but
I think is a reasonable-enough approach, and in keeping with some
other existing code of the same form.

This patch also fixes a similar bug in target_supports_delete_record.

Unfortunately this patch doesn't actually fix the PR, because now
windows-nat.c is using the native target code, which installs
child_terminal_ours.  Pedro is looking into this.

2014-07-11  Tom Tromey  <tromey@redhat.com>

	PR gdb/17130:
	* utils.c (quit): Use target_supports_terminal_ours.
	* target.h (target_supports_terminal_ours): Declare.
	* target.c (target_supports_delete_record): Don't check
	to_delete_record against NULL.
	(target_supports_terminal_ours): New function.
---
 gdb/ChangeLog |  9 +++++++++
 gdb/target.c  | 20 +++++++++++++++++++-
 gdb/target.h  |  5 +++++
 gdb/utils.c   |  2 +-
 4 files changed, 34 insertions(+), 2 deletions(-)
  

Comments

Pedro Alves July 15, 2014, 8:12 a.m. UTC | #1
On 07/11/2014 05:37 PM, Tom Tromey wrote:

> This patch also fixes a similar bug in target_supports_delete_record.
> 
> Unfortunately this patch doesn't actually fix the PR, because now
> windows-nat.c is using the native target code, which installs
> child_terminal_ours.  Pedro is looking into this.

Your patch makes that message go away from a Quit printed _before_
starting the program, before the target is pushed, on hosts that
don't have job control (such as native Windows).  And that's good.
No SIGINT is ever coming our way in that case.

Well, at least if we don't consider multi-process/multi-target
(we may have the focus on an inferior without execution, but
we may also have other inferiors started).

I got confused when I still saw:

 (gdb) Quit (expect signal SIGINT when the program is resumed)
 (gdb)

on Windows, after starting the program.  That is, after the
Windows native target is pushed.

I've looked at it, and it's not actually a bug.

I wrongly assumed that before the "native" target series that made
windows-nat.c inherit inf-child.c, the Windows target had a NULL
to_terminal_ours, but that turns out to be false.  It's been non-NULL,
for a looooong time, possibly forever.  In 1999, for example, we
already see:

 $ grep to_terminal_ours win32-nat.c
   child_ops.to_terminal_ours_for_output =   terminal_ours_for_output;
   child_ops.to_terminal_ours  =   terminal_ours;

...

  if (job_control
      /* If there is no terminal switching for this target, then we can't
	 possibly get screwed by the lack of job control.  */
      || current_target.to_terminal_ours == NULL)
    fprintf_unfiltered (gdb_stderr, "Quit\n");
  else
    fprintf_unfiltered (gdb_stderr,
	     "Quit (expect signal SIGINT when the program is resumed)\n");


So no regression on Windows there.

I got confused because when using the "system"'s cygwin GDB one
doesn't see that "expect SIGINT" note -- cygwin GDB uses the same
windows-nat.c -- but then, I recalled that Cygwin does have
job control.

Now that I try it out, it's really working as intended,
a SIGINT does really come out once the program is resumed.
On native Windows gdb:

 (gdb) *ctrl-c*
 (gdb) Quit (expect signal SIGINT when the program is resumed)
 (gdb) c
 Continuing.
 [New Thread 4004.0xf44]

 Program received signal SIGINT, Interrupt.
 [Switching to Thread 4004.0xf44]
 0x755073e7 in KERNEL32!CtrlRoutine () from C:\Windows\syswow64\kernel32.dll
 (gdb)

So in sum, the PR fully fixes the issue.  Please update the git log.  :-)

Thanks!
  
Tom Tromey July 15, 2014, 3:21 p.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> So in sum, the PR fully fixes the issue.  Please update the git log.  :-)

Thanks for digging into this.
I've fixed up the commit message.

Tom
  

Patch

diff --git a/gdb/target.c b/gdb/target.c
index c9c5e4b..93fc003 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -499,6 +499,23 @@  target_terminal_inferior (void)
   (*current_target.to_terminal_inferior) (&current_target);
 }
 
+/* See target.h.  */
+
+int
+target_supports_terminal_ours (void)
+{
+  struct target_ops *t;
+
+  for (t = current_target.beneath; t != NULL; t = t->beneath)
+    {
+      if (t->to_terminal_ours != delegate_terminal_ours
+	  && t->to_terminal_ours != tdefault_terminal_ours)
+	return 1;
+    }
+
+  return 0;
+}
+
 static void
 tcomplain (void)
 {
@@ -3456,7 +3473,8 @@  target_supports_delete_record (void)
   struct target_ops *t;
 
   for (t = current_target.beneath; t != NULL; t = t->beneath)
-    if (t->to_delete_record != NULL)
+    if (t->to_delete_record != delegate_delete_record
+	&& t->to_delete_record != tdefault_delete_record)
       return 1;
 
   return 0;
diff --git a/gdb/target.h b/gdb/target.h
index 8bf160c..d6e1516 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1396,6 +1396,11 @@  extern void target_terminal_inferior (void);
 #define target_terminal_ours() \
      (*current_target.to_terminal_ours) (&current_target)
 
+/* Return true if the target stack has a non-default
+  "to_terminal_ours" method.  */
+
+extern int target_supports_terminal_ours (void);
+
 /* Save our terminal settings.
    This is called from TUI after entering or leaving the curses
    mode.  Since curses modifies our terminal this call is here
diff --git a/gdb/utils.c b/gdb/utils.c
index 6f47cb0..f99843c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1095,7 +1095,7 @@  quit (void)
   if (job_control
       /* If there is no terminal switching for this target, then we can't
          possibly get screwed by the lack of job control.  */
-      || current_target.to_terminal_ours == NULL)
+      || !target_supports_terminal_ours ())
     fatal ("Quit");
   else
     fatal ("Quit (expect signal SIGINT when the program is resumed)");