[PATCH] PR mi/2077 "set edit off" breaks MI

[PATCH] PR mi/2077 "set edit off" breaks MI

[Nick Roberts]

This patch is taken out of the async patch.  It's probably a good idea to
commit the different parts separately anyway.  The bug arises because GDB gets
confused about the current interpreter when doing "set edit off" (via
-interpreter-exec.  To see this more clearly, instead of Vladimir's example
try:


  ~"This GDB was configured as \"i686-pc-linux-gnu\"...\n"
  ~"Using host libthread_db library \"/lib/libthread_db.so.1\".\n"
  (gdb)
  start
  &"start\n"
  ~"Breakpoint 1 at 0x8048513: file myprog.c, line 67.\n"
  ~"Starting program: /home/nickrob/myprog \n"
  ~"main () at myprog.c:67\n"
  ~"67\t  int i, n,  m[10] = {0, 1, 2, 3, 4 , 5, 6, 7, 8, 9};\n"
  ^done
  (gdb)
  -exec-next
  ^running
  (gdb)
  *stopped,reason="end-stepping-range",thread-id="0",frame={addr="0x08048574",func="main",args=[],file="myprog.c",fullname="/home/nickrob/myprog.c",line="68"}
  (gdb)
  set edit off
  &"set edit off\n"
  ^done
  (gdb)
  -exec-next
  &"Undefined command: \"-exec-next\".  Try \"help\".\n"

and note that the response to the second -exec-next is a CLI one (no ^error).

Apple switch interpreters which fixes this bug.  Jim Ingham can probably
explain more eloquently why it works.

I don't know how to give attribution to these changes in the ChangeLog as I
can't match them to entries in Apple's.  Perhaps Jim can provide the details.
Otherwise can I just write?:

2006-10-19  Apple Computer, Inc  <www.apple.com>

Or do I need a person/e-mail address?

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** interps.h	18 Dec 2005 11:34:01 +1300	1.10
--- interps.h	19 Oct 2006 11:21:41 +1300	
*************** extern struct interp *interp_new (const 
*** 59,65 ****
  				  struct ui_out *uiout,
  				  const struct interp_procs *procs);
  extern void interp_add (struct interp *interp);
! extern int interp_set (struct interp *interp);
  extern struct interp *interp_lookup (const char *name);
  extern struct ui_out *interp_ui_out (struct interp *interp);
  
--- 59,66 ----
  				  struct ui_out *uiout,
  				  const struct interp_procs *procs);
  extern void interp_add (struct interp *interp);
! extern struct interp *interp_set (struct interp *interp);
! extern int interp_set_quiet (struct interp *interp, int quiet);
  extern struct interp *interp_lookup (const char *name);
  extern struct ui_out *interp_ui_out (struct interp *interp);
  
*** interps.c	18 Sep 2006 16:44:39 +1200	1.18
--- interps.c	19 Oct 2006 11:23:44 +1300	
*************** interp_add (struct interp *interp)
*** 127,133 ****
     interpreter back in place and return 0.  If we can't restore the
     old interpreter, then raise an internal error, since we are in
     pretty bad shape at this point. */
! int
  interp_set (struct interp *interp)
  {
    struct interp *old_interp = current_interpreter;
--- 127,133 ----
     interpreter back in place and return 0.  If we can't restore the
     old interpreter, then raise an internal error, since we are in
     pretty bad shape at this point. */
! struct interp *
  interp_set (struct interp *interp)
  {
    struct interp *old_interp = current_interpreter;
*************** interp_set (struct interp *interp)
*** 188,194 ****
  	internal_error (__FILE__, __LINE__,
  			_("Failed to initialize new interp \"%s\" %s"),
  			interp->name, "and could not restore old interp!\n");
!       return 0;
      }
  
    /* Finally, put up the new prompt to show that we are indeed here. 
--- 188,194 ----
  	internal_error (__FILE__, __LINE__,
  			_("Failed to initialize new interp \"%s\" %s"),
  			interp->name, "and could not restore old interp!\n");
!       return NULL;
      }
  
    /* Finally, put up the new prompt to show that we are indeed here. 
*************** interp_set (struct interp *interp)
*** 206,212 ****
        display_gdb_prompt (NULL);
      }
  
!   return 1;
  }
  
  /* interp_lookup - Looks up the interpreter for NAME.  If no such
--- 206,218 ----
        display_gdb_prompt (NULL);
      }
  
!   /* If there wasn't any interp before, return the current interp.  
!      That way if somebody is grabbing the return value and using
!      it, it will actually work first time through.  */
!   if (old_interp == NULL)
!     return current_interpreter;
!   else
!     return old_interp;
  }
  
  /* interp_lookup - Looks up the interpreter for NAME.  If no such
*************** interp_quiet_p (struct interp *interp)
*** 289,295 ****
      return current_interpreter->quiet_p;
  }
  
! static int
  interp_set_quiet (struct interp *interp, int quiet)
  {
    int old_val = interp->quiet_p;
--- 295,301 ----
      return current_interpreter->quiet_p;
  }
  
! int
  interp_set_quiet (struct interp *interp, int quiet)
  {
    int old_val = interp->quiet_p;
*** mi/mi-interp.c	23 Dec 2005 18:57:46 -0000	1.17
--- mi/mi-interp.c	19 Oct 2006 06:40:21 -0000
*************** struct mi_interp
*** 48,53 ****
--- 48,56 ----
    struct interp *mi_interp;
  };
  
+ /* Points to the current interpreter, used by the mi context callbacks.  */
+ static struct interp *mi_interp;
+ 
  /* These are the interpreter setup, etc. functions for the MI interpreter */
  static void mi_execute_command_wrapper (char *cmd);
  static void mi_command_loop (int mi_version);
*************** enum mi_cmd_result
*** 188,196 ****
  mi_cmd_interpreter_exec (char *command, char **argv, int argc)
  {
    struct interp *interp_to_use;
    enum mi_cmd_result result = MI_CMD_DONE;
    int i;
!   struct interp_procs *procs;
  
    if (argc < 2)
      {
--- 191,200 ----
  mi_cmd_interpreter_exec (char *command, char **argv, int argc)
  {
    struct interp *interp_to_use;
+   struct interp *old_interp;
    enum mi_cmd_result result = MI_CMD_DONE;
    int i;
!   int old_quiet;
  
    if (argc < 2)
      {
*************** mi_cmd_interpreter_exec (char *command, 
*** 211,216 ****
--- 215,235 ----
  				     argv[0]);
        return MI_CMD_ERROR;
      }
+   
+   old_quiet = interp_set_quiet (interp_to_use, 1);
+ 
+   old_interp = interp_set (interp_to_use); 
+   if (old_interp == NULL)
+     {
+       asprintf (&mi_error_message,
+                 "Could not switch to interpreter \"%s\".", argv[0]);
+       return MI_CMD_ERROR;
+     }  
+   
+   /* Set the global mi_interp.  We need this so that the hook functions
+      can leave their results in the mi interpreter, rather than dumping
+      them to the console.  */
+   mi_interp = old_interp;
  
    /* Insert the MI out hooks, making sure to also call the interpreter's hooks
       if it has any. */
*************** mi_cmd_interpreter_exec (char *command, 
*** 254,260 ****
--- 273,284 ----
        sync_execution = 0;
      }
  
+   /* Now do the switch */
+   interp_set (old_interp);
+   mi_interp = NULL;
+ 
    mi_remove_notify_hooks ();
+   interp_set_quiet (interp_to_use, old_quiet);
  
    /* Okay, now let's see if the command set the inferior going...
       Tricky point - have to do this AFTER resetting the interpreter, since

Re: [PATCH] PR mi/2077 "set edit off" breaks MI

[Daniel Jacobowitz]

On Thu, Oct 19, 2006 at 09:55:56PM +1300, Nick Roberts wrote:
> 
> This patch is taken out of the async patch.  It's probably a good idea to
> commit the different parts separately anyway.

Definitely.

> Apple switch interpreters which fixes this bug.  Jim Ingham can probably
> explain more eloquently why it works.

FYI, the reason I shelved this one to reply to later was that you
didn't explain how it works.  It's much harder to review a patch for
correctness in that case.  I have to go figure it out myself
whether it's fixing the symptom or the cause.

When I went to do that I decided that the patch only fixed the symptom,
unfortunately.  Try repeating your test case, but instead of saying
"set edit off", say "-gdb-set edit off".  That won't switch
interpreters because it isn't running a CLI command.  Then try
-exec-next and it will fail.

The problem occurs because it's futzing with the CLI input handlers
when they are not installed, particularly input_handler.  I don't
really understand how all the event loop bits work.  The right solution
to this may be to not change anything if the current interpreter isn't
the CLI, but I don't know what should happen if TUI is enabled.  Or
maybe the right solution is to not change input_handler in
change_line_handler, just the other two.  The comment says to do
that in case "set edit off" is in .gdbinit, but I don't see why
that matters; do you?

The patch itself is probably good, by the way, just not for this
bug.  I would definitely like to merge the async changes in pieces,
so if you can explain why this patch is necessary and write a changelog
for it, I'll take a second look at it on its own merits.

> I don't know how to give attribution to these changes in the ChangeLog as I
> can't match them to entries in Apple's.  Perhaps Jim can provide the details.
> Otherwise can I just write?:
> 
> 2006-10-19  Apple Computer, Inc  <www.apple.com>
> 
> Or do I need a person/e-mail address?

I think it would be best to put your own name on the changelog, and
mention something like "From Apple Computer, Inc." at the top of the
entry.  I don't really know though so if anyone else has an opinion,
please share.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PR mi/2077 "set edit off" breaks MI

[Nick Roberts]

 > > This patch is taken out of the async patch.  It's probably a good idea to
 > > commit the different parts separately anyway.
 > 
 > Definitely.

I can split it into one async patch (the largest part) and several small MI
patches.  The MI patches are probably quite safe changes and the async part
shouldn't interfere with normal use.  Unless Elena becomes active again, I
think it's likely that you will be the only person who would want to/be willing
to review the latter.  You have suggested that you would also like to use it
for other reasons.  When you have time to review it please tell me and I will
re-submit it (without the MI changes).

 > > Apple switch interpreters which fixes this bug.  Jim Ingham can probably
 > > explain more eloquently why it works.
 > 
 > FYI, the reason I shelved this one to reply to later was that you
 > didn't explain how it works.  It's much harder to review a patch for
 > correctness in that case.  I have to go figure it out myself
 > whether it's fixing the symptom or the cause.
 > 
 > When I went to do that I decided that the patch only fixed the symptom,
 > unfortunately.  Try repeating your test case, but instead of saying
 > "set edit off", say "-gdb-set edit off".  That won't switch
 > interpreters because it isn't running a CLI command.  Then try
 > -exec-next and it will fail.

Ah, I see.  I'll look into it.

 >...
 > > Otherwise can I just write?:
 > > 
 > > 2006-10-19  Apple Computer, Inc  <www.apple.com>
 > > 
 > > Or do I need a person/e-mail address?
 > 
 > I think it would be best to put your own name on the changelog, and
 > mention something like "From Apple Computer, Inc." at the top of the
 > entry.  I don't really know though so if anyone else has an opinion,
 > please share.

I'll ask RMS, I've always been told that the committer's name should not be
the one to go in the changelog.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] PR mi/2077 "set edit off" breaks MI

[Daniel Jacobowitz]

On Mon, Nov 20, 2006 at 03:32:42PM +1300, Nick Roberts wrote:
>  > > This patch is taken out of the async patch.  It's probably a good idea to
>  > > commit the different parts separately anyway.
>  > 
>  > Definitely.
> 
> I can split it into one async patch (the largest part) and several small MI
> patches.  The MI patches are probably quite safe changes and the async part
> shouldn't interfere with normal use.  Unless Elena becomes active again, I
> think it's likely that you will be the only person who would want to/be willing
> to review the latter.  You have suggested that you would also like to use it
> for other reasons.  When you have time to review it please tell me and I will
> re-submit it (without the MI changes).

We've branched; you have a patch ready; let's get it going!  I'd be
glad to see this merged.  I'm afraid I really haven't looked at what
you have so far; could you post a current patch that I can experiment
with?

I'll try to look at it during this week.

> I'll ask RMS, I've always been told that the committer's name should not be
> the one to go in the changelog.

True, but you've updated the patch and in general done more work on it;
the next best thing would probably be to either use Jim's name, or ask
him whose name to use.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PR mi/2077 "set edit off" breaks MI

[Nick Roberts]

[-- Attachment #1: message body and .signature --]
[-- Type: text/plain, Size: 10029 bytes --]

 > We've branched; you have a patch ready; let's get it going!  I'd be
 > glad to see this merged.  I'm afraid I really haven't looked at what
 > you have so far; could you post a current patch that I can experiment
 > with?

I'm attaching it below.  To run testsuite with asynchronous event loop put the
line:

  set GDBFLAGS "--async"

at the bottom of site.exp.

I've taken nearly all the MI changes out but running MI is still one good way
of checking asynchronous behaviour:

synchronous:

  n
  &"n\n"
  ~"70\t  int n1 = 7, n2 = 8, n3 = 9;\n"
  ^done
  (gdb)

asynchronous:

n
&"n\n"
^running
^done
(gdb)
*stopped,reason="end-stepping-range",thread-id="0",frame={addr="0x080485bb",func="main",args=[],file="myprog.c",fullname="/home/nickrob/myprog.c",line="70"}
(gdb)

I've created a new testfile (below) for this kind of thing.

 > I'll try to look at it during this week.

Thanks.  It's probably still a bit rough but if you can tell me what to do I'll
clean it up/make it more general.

 > > I'll ask RMS, I've always been told that the committer's name should not be
 > > the one to go in the changelog.
 > 
 > True, but you've updated the patch and in general done more work on it;

That's true.  I've tried to put my changes at the top of the changelog and
pure Apple at the bottom prefixed with "copied verbatim" i.e idiot fashion
where I probably don't really know what is going on.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2006-11-21  Nick Roberts  <nickrob@snap.net.nz>

	* defs.h (async_signal_hook): Declare.
	(event_loop_p): New extern.
	(deprecated_command_loop_hook): Make argument type void*.
	
	* main.c (captured_main, print_gdb_help): Add and describe --async
	option conditional on macro ASYNC.
	(event_loop_p): New global variable.
	
	* event-loop.c (async_signal_hook): New function pointer.
	(gdb_wait_for_event): Use it.

	* linux-nat.c: Include inf-loop.h.  Include poll.h conditionally.
	(gdb_post_startup): New extern.
	(gdb_status, gdb_file_event): New variables.
	(async_mask, old_mask, async_action, old_action): New variables.
	(linux_nat_attach, linux_nat_detach): Condition on
	target_can_async_p.
	(linux_nat_resume): Add async file handler if necessary.
	(linux_nat_fetch_event): New function.
	(linux_nat_wait): Use it when asynchronous.
	(async_sigchld_handler, linux_nat_signal_hook): New functions.
	(_initialize_linux_nat): Set up signal handling and pipe for
	asynchronous behaviour.

	* infrun.c: Include event-top.h.
	(sync_execution): Set sync_execution to 1 initially.
	(proceed): Set target_executing to 0 if synchronous.
	(handle_inferior_event): Print process switching for MI(?).
	Don't call context_switch (ecs) or flush_cached_frames ().
	Use local variable was_sync_execution.

	* inf-ptrace.c: Include inf-loop.h, event-loop.h and event-top.h.
	(standard_is_async_p, standard_can_async_p)
	(initialize_sigint_signal_handler): New externs.
	(async_client_context, async_terminal_ours_p): New variables.
	(async_file_handler, inf_ptrace_async): New functions.
	(inf_ptrace_him): Use gdb_post_startup as a flag.
	(inf_ptrace_mourn_inferior): Don't call waitpid if asynchronous.
	(inf_ptrace_attach): Set gdb_post_startup.  Call wait_for_inferior
	if asynchronous.
	(async_terminal_inferior): New function.
	(inf_ptrace_target): Add methods if asynchronous.

	* exec.c (standard_async, standard_is_async_p, standard_can_async_p):
	New functions.
	(init_exec_ops): Initialise above methods if GDB is invoked
	with --async.

	* infcmd.c (jump_command): Call async_disable_stdin if necessary
	before decode_line_spec_1.
	(attach_command): Make attach work asynchronously.

	 (copied verbatim)
	* event-top.h (cli_command_loop):  Make argument type void*.

	 (copied verbatim)
	* event-top.c (cli_command_loop): Make argument void*.
	(display_gdb_prompt, async_enable_stdin)
	(async_disable_stdin, handle_sigint, async_request_quit)
	(gdb_setup_readline, _initialize_event_loop): 
	Changes for asynchronous operation.

	 (copied verbatim)
	* interps.c (interp_set): Make type struct interp *.
	Don't do_all_continuations or call clear_interpreter_hooks. 
	(current_interp_command_loop): Call command_loop with NULL argument.
	(interp_set_quiet): Don't make static.

	 (copied verbatim)
	* interps.h (interp_set, interp_set_quiet): New externs.

	 (copied verbatim)
	* inf-loop.c (inferior_event_handler, complete_execution): 
	Changes for asynchronous operation.

	 (copied verbatim)
	* cli/cli-interp.c (cli_interpreter_resume): Set sync_execution.
	(safe_execute_command): Don't make it static.
	(_initialize_cli_interp): Add cli_command_loop to interp_procs
	structure.

	 (copied verbatim)
	* wrapper.h (safe_execute_command): Declare here.

	 (copied verbatim)
	* remote.c (cleanup_sigint_signal_handler)
	(initialize_sigint_signal_handler): Don't make static.

	 (copied verbatim)
	* top.c (deprecated_command_loop_hook): Make argument type void*.

	 (copied verbatim)
	* mi/mi-interp.c (mi1_command_loop, mi2_command_loop)
	(mi3_command_loop): Make argument type void*.

	* config/i386/nm-linux.h: Add ASYNC macro definition.

	* Makefile.in (inf-ptrace.o, infrun.o, linux-nat.o)
	(cli-interp.o): Add new header dependencies.

 
 ChangeLog              |  102 +++++++++++++++++++++++++++
 Makefile.in            |    9 !!
 cli/cli-interp.c       |   11 -!
 config/i386/nm-linux.h |    3
 defs.h                 |    6 +
 event-loop.c           |   15 ++++
 event-top.c            |   93 ++++++++++++++!!!!!!!!!!!
 event-top.h            |    2
 exec.c                 |   26 ++++++-
 inf-loop.c             |   46 ++++++!!!!!!
 inf-ptrace.c           |  101 ++++++++++++++++++++++++++!
 infcmd.c               |   25 ++--!!
 infrun.c               |   34 ++++---!
 interps.c              |   40 +++!!!!!!!
 interps.h              |    3
 linux-nat.c            |  181 +++++++++++++++++++++++++++++!!!!!!!!!!!!!!!!!!!
 main.c                 |   15 ++++
 mi/mi-interp.c         |   12 !!!
 remote.c               |    8 !!
 top.c                  |    2
 wrapper.h              |    2
 21 files changed, 490 insertions(+), 25 deletions(-), 221 modifications(!)


# Copyright 1999, 2000, 2001, 2002, 2004 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
# 
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
# 
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  

#
# Test asynchronous Machine Interface (MI) operations
#

load_lib mi-support.exp
set saved_gdbflags $GDBFLAGS
set GDBFLAGS "-nw --async"
set MIFLAGS "-i=mi"
#set verbose 3

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile "basics"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
     untested mi-async.exp
     return -1
}

mi_delete_breakpoints
mi_gdb_reinitialize_dir $srcdir/$subdir
mi_gdb_load ${binfile}

# mi_gdb_test cannot be used for asynchronous commands because there are
# two prompts involved and this can lead to a race condition.

proc linux_async_tests {} {
    global mi_gdb_prompt
    global hex

    set line_main_head     [gdb_get_line_number "main ("]
    set line_main_body     [expr $line_main_head + 2]
    set line_main_next     [expr $line_main_head + 3]

    send_gdb "start\n"
    gdb_expect {
	-re ".*\\^running\r\n\\^done\r\n$mi_gdb_prompt" {
	    gdb_expect {
		-re "\\*stopped,thread-id=\"0\",frame=\{addr=\"$hex\",func=\"main\",args=\\\[\\\],file=\".*basics.c\",line=\"$line_main_body\"\}\r\n$mi_gdb_prompt$" {
		    pass "Asynchronous response after start command"
		}
		-re ".*$mi_gdb_prompt$" {
		    fail "Asynchronous response after start command (2)"
		}
		timeout	{
		    fail "Asynchronous response after start command (timeout 2)"
		}
	    }
	}
	-re ".*$mi_gdb_prompt$" {
	    fail "Asynchronous response after start command (1)"
	}
	timeout {fail "Asynchronous response after start command (timeout 1)"}
    }

    send_gdb "next\n"
    gdb_expect {
	-re "\\^running\r\n\\^done\r\n$mi_gdb_prompt" {
	    gdb_expect {
		-re "\\*stopped,reason=\"end-stepping-range\",thread-id=\"0\",frame=\{addr=\"$hex\",func=\"main\",args=\\\[\\\],file=\".*basics.c\",line=\"$line_main_next\"\}\r\n$mi_gdb_prompt$" {
		    pass "Asynchronous response after next command"
		}
		-re ".*$mi_gdb_prompt$" {
		    fail "Asynchronous response after next command (2)"
		}
		timeout {
		    fail "Asynchronous response after next command (timeout 2)"
		}
	    }
	}
	-re ".*$mi_gdb_prompt$" {
	    fail "Asynchronous response after next command (1)"
	}
	timeout {fail "Asynchronous response after next command (timeout 1)"}
    }

    send_gdb "start\n"
    gdb_expect {
	-re ".*\\^running\r\n\\^done\r\n$mi_gdb_prompt" {
	    gdb_expect {
		-re "\\*stopped,thread-id=\"0\",frame=\{addr=\"$hex\",func=\"main\",args=\\\[\\\],file=\".*basics.c\",line=\"$line_main_body\"\}\r\n$mi_gdb_prompt$" {
		    pass "Asynchronous response after (re) start"
		}
		-re ".*$mi_gdb_prompt$" {
		    fail "Asynchronous response after (re) start (2)"
		}
		timeout	{
		    fail "Asynchronous response after (re) start (timeout 2)"
		}
	    }
	}
	-re ".*$mi_gdb_prompt$" {
	    fail "Asynchronous response after (re) start (1)"
	}
	timeout {fail "Asynchronous response after (re) start (timeout 1)"}
    }
}


#if [istarget "i386-*-linux-gnu"] then {
    linux_async_tests
#}

mi_gdb_exit

set GDBFLAGS $saved_gdbflags

return 0



[-- Attachment #2: async without threads, handle SIGCHLD in gdb_wait_for_event. --]
[-- Type: application/octet-stream, Size: 14324 bytes --]