Re: MI problem

Re: MI problem

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> The interesting question isn't where it gets switched, but where we're
> expecting the wrong contents.  If we're accessing uiout.data->buffer
> then we must have reached mi_out_put.  So where from, and how did we
> get there without switching the global uiout back to MI's? 
> mi_cmd_interpreter_exec ought to be doing that on the way out.

This is the backtrace from the core dump file:

#0  0x0805a220 in ui_file_put (file=0x19, write=0x80863f8 <do_write>, 
dest=0xa030f18)
     at ../../src/gdb/ui-file.c:192
#1  0x0808642a in mi_out_put (uiout=0xa028b90, stream=0xa030f18)
     at ../../src/gdb/mi/mi-out.c:376
#2  0x0808adc9 in mi_load_progress (section_name=0xa11f79f ".init", 
sent_so_far=54,
     total_section=54, total_sent=54, grand_total=523177) at 
../../src/gdb/mi/mi-main.c:1402
#3  0x080c4857 in load_section_callback (abfd=0xa0a5278, asec=0x36, 
data=0xbfffb930)
     at ../../src/gdb/symfile.c:1597
#4  0x0818c69d in bfd_map_over_sections (abfd=0xa0a5278,
     operation=0x80c4670 <load_section_callback>, user_storage=0xbfffb930)
     at ../../src/bfd/section.c:1226
#5  0x080c4a17 in generic_load (args=0xbfffb930 "", from_tty=1) at 
../../src/gdb/symfile.c:1670
#6  0x080edf1f in target_load (arg=0xa0322b0 
"/home/afra/users/stubbsa/os21test-microdev",
     from_tty=1) at ../../src/gdb/target.c:268

mi_load_progress is actually called through the function pointer 
deprecated_show_load_progress.

The problem appears to be that the program is finding it's way into the 
MI through deprecated_show_load_progress when uiout is not set as expected.

The problem only occurs in -i=mi (no number) and -i=mi1 because 
mi_load_progress does not do anything in other versions. Strangely -i=mi 
is supposed to be the same as -i=mi2, but it isn't in this respect.

I attach a patch which fixes both the crash and the inconsistency. I 
have tested it and got no unexpected failures. Of course I doubt the 
testsuite covers this feature ...

OK? I would like to see this issued fixed in 6.4 also.

Andrew Stubbs

[-- Attachment #2: mi-load-crash.patch --]
[-- Type: text/plain, Size: 1276 bytes --]

2005-11-17  Andrew Stubbs  <andrew.stubbs@st.com>

	* mi/mi-main.c (mi_load_progress): Don't do anything for -i=mi.
	Ensure the use of mi1 uiout for the duration of the function.

Index: src/gdb/mi/mi-main.c
===================================================================
--- src.orig/gdb/mi/mi-main.c	2005-06-13 23:21:57.000000000 +0100
+++ src/gdb/mi/mi-main.c	2005-11-17 18:58:45.000000000 +0000
@@ -1365,11 +1365,17 @@ mi_load_progress (const char *section_na
   static struct timeval last_update;
   static char *previous_sect_name = NULL;
   int new_section;
+  struct ui_out *saved_uiout;
 
-  if (!current_interp_named_p (INTERP_MI)
-      && !current_interp_named_p (INTERP_MI1))
+  if (!current_interp_named_p (INTERP_MI1))
     return;
 
+  /* This function is called through deprecated_show_load_progress
+     which means uiout may not be correct.  Fix it for the duration
+     of this function.  */
+  saved_uiout = uiout;
+  uiout = mi_out_new (1);
+
   update_threshold.tv_sec = 0;
   update_threshold.tv_usec = 500000;
   gettimeofday (&time_now, NULL);
@@ -1424,6 +1430,9 @@ mi_load_progress (const char *section_na
       fputs_unfiltered ("\n", raw_stdout);
       gdb_flush (raw_stdout);
     }
+
+  xfree (uiout);
+  uiout = saved_uiout;
 }
 
 void

Re: MI problem

[Daniel Jacobowitz]

Any MI folks listening?

On Thu, Nov 17, 2005 at 07:17:01PM +0000, Andrew STUBBS wrote:
> mi_load_progress is actually called through the function pointer 
> deprecated_show_load_progress.
> 
> The problem appears to be that the program is finding it's way into the 
> MI through deprecated_show_load_progress when uiout is not set as expected.

Yes, this is the problem.

> The problem only occurs in -i=mi (no number) and -i=mi1 because 
> mi_load_progress does not do anything in other versions. Strangely -i=mi 
> is supposed to be the same as -i=mi2, but it isn't in this respect.

It looks like this is a mistake.  I believe that when that check was
added, there were only MI and MI1.  When the version was bumped, it was
left behind.

So should we fix it, or leave it disabled?

On a related note I would have expected current_interp_named_p to
return false while executing a program in the CLI interpreter.
interpreter_exec_cmd always sets the current interpreter.  But
that is only the CLI implementation of interpreter-exec.  The MI
implementation doesn't do this.

I'm always confused in the twisty maze of interpreters.  I think we
can't ditch this broken behavior until we're done with "the CLI hack"
(CLI commands printing out MI format output because there is no
equivalent MI command).  That'd be a good project for someone.

> I attach a patch which fixes both the crash and the inconsistency. I 
> have tested it and got no unexpected failures. Of course I doubt the 
> testsuite covers this feature ...
> 
> OK? I would like to see this issued fixed in 6.4 also.
> 
> Andrew Stubbs

> 2005-11-17  Andrew Stubbs  <andrew.stubbs@st.com>
> 
> 	* mi/mi-main.c (mi_load_progress): Don't do anything for -i=mi.
> 	Ensure the use of mi1 uiout for the duration of the function.

How about we commit this without the current_interp_named_p change for
now?  Normally we'd need to use a cleanup to save and restore uiout,
but nothing here can throw exceptions AFAICT, so we're OK with the way
you've done it.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: MI problem

[Andrew STUBBS]

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

> How about we commit this without the current_interp_named_p change for
> now?  Normally we'd need to use a cleanup to save and restore uiout,
> but nothing here can throw exceptions AFAICT, so we're OK with the way
> you've done it.

OK, I'm happy to do that.

Am I OK to commit the attached to both branches?

Andrew

[-- Attachment #2: mi-load-crash.patch --]
[-- Type: text/plain, Size: 1270 bytes --]

2005-11-18  Andrew Stubbs  <andrew.stubbs@st.com>

	* mi/mi-main.c (mi_load_progress): Ensure the use of the correct mi
	uiout for the duration of the function.

Index: src/gdb/mi/mi-main.c
===================================================================
--- src.orig/gdb/mi/mi-main.c	2005-11-17 19:47:42.000000000 +0000
+++ src/gdb/mi/mi-main.c	2005-11-18 16:16:38.000000000 +0000
@@ -1365,9 +1365,18 @@ mi_load_progress (const char *section_na
   static struct timeval last_update;
   static char *previous_sect_name = NULL;
   int new_section;
+  struct ui_out *saved_uiout;
 
-  if (!current_interp_named_p (INTERP_MI)
-      && !current_interp_named_p (INTERP_MI1))
+  /* This function is called through deprecated_show_load_progress
+     which means uiout may not be correct.  Fix it for the duration
+     of this function.  */
+  saved_uiout = uiout;
+
+  if (current_interp_named_p (INTERP_MI))
+    uiout = mi_out_new (2);
+  else if (current_interp_named_p (INTERP_MI1))
+    uiout = mi_out_new (1);
+  else
     return;
 
   update_threshold.tv_sec = 0;
@@ -1424,6 +1433,9 @@ mi_load_progress (const char *section_na
       fputs_unfiltered ("\n", raw_stdout);
       gdb_flush (raw_stdout);
     }
+
+  xfree (uiout);
+  uiout = saved_uiout;
 }
 
 void

Re: MI problem

[Daniel Jacobowitz]

On Fri, Nov 18, 2005 at 04:17:26PM +0000, Andrew STUBBS wrote:
> >How about we commit this without the current_interp_named_p change for
> >now?  Normally we'd need to use a cleanup to save and restore uiout,
> >but nothing here can throw exceptions AFAICT, so we're OK with the way
> >you've done it.
> 
> OK, I'm happy to do that.
> 
> Am I OK to commit the attached to both branches?

Yes please.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: MI problem

[Andrew STUBBS]

Daniel Jacobowitz wrote:
>>Am I OK to commit the attached to both branches?
> 
> 
> Yes please.

Thanks, commited twice.

Andrew Stubbs