MI testsuite fix

MI testsuite fix

[Pedro Alves]

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

Running the testsuite in linux native async mode in
an x86_64-unknown-linux-gnu with -m32 to simulate an x86-pc-linux-gnu,
I get a bunch of MI testsuite errors related to this difference of
output in async vs sync modes:

async:

220^running
&"warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at 
ffffe0b4\n"
(gdb)

sync:

220^running
(gdb)
&"warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at 
ffffe0b4\n"


It results in errors that look like these:

220-exec-run
220^running
&"warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at 
ffffe0b4\n"
(gdb)
~"[Thread debugging using libthread_db enabled]\n"
220*stopped,thread-id="1",frame={addr="0x08048542",func="main",args=[],file="../../../src/gdb/testsuite/gdb.mi/pth
reads.c",fullname="/home/pedro/gdb/track_mi/src/gdb/testsuite/gdb.mi/pthreads.c",line="79"}
(gdb)
ERROR: Unable to start target

The difference comes from the fact that on sync mode, the MI prompt is output
immediatelly after ^running, while on async mode, it is output a bit later.

  if (!target_can_async_p ())
    {
      /* NOTE: For synchronous targets asynchronous behavour is faked by
         printing out the GDB prompt before we even try to execute the
         command.  */
      if (last_async_command)
	fputs_unfiltered (last_async_command, raw_stdout);
      fputs_unfiltered ("^running\n", raw_stdout);
      fputs_unfiltered ("(gdb) \n", raw_stdout);
      gdb_flush (raw_stdout);
    }
  else
    {
      /* FIXME: cagney/1999-11-29: Printing this message before
         calling execute_command is wrong.  It should only be printed
         once gdb has confirmed that it really has managed to send a
         run command to the target.  */
      if (last_async_command)
	fputs_unfiltered (last_async_command, raw_stdout);
      fputs_unfiltered ("^running\n", raw_stdout);
    }

The async case looks more correct than the sync one, so I propose fixing
the regex to match warnings before the MI prompt.

It is also arguable if that warning has any value, but in any case,
we should be filtering warnings.

Fixing this leaves me with one MI regression, mi-pending.exp, which is 
related to throwing an exception running the exec cleanups, which deletes
the MI token, when it shouldn't.  That is fixed by Vladimir's pending
"murder exec cleanup" patch, or when the token in *stopped is removed.
Both will go in very soon.

-- 
Pedro Alves

[-- Attachment #2: mi_fix.diff --]
[-- Type: text/x-diff, Size: 1174 bytes --]

2008-04-10  Pedro Alves  <pedro@codesourcery.com>

	* lib/mi-support.exp (mi_warnings): New.
	(mi_run_cmd): Filter warnings in the -exec-run case.
	
---
 gdb/testsuite/lib/mi-support.exp |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: src/gdb/testsuite/lib/mi-support.exp
===================================================================
--- src.orig/gdb/testsuite/lib/mi-support.exp	2008-04-10 20:45:40.000000000 +0100
+++ src/gdb/testsuite/lib/mi-support.exp	2008-04-10 20:51:46.000000000 +0100
@@ -33,6 +33,8 @@ global mi_inferior_tty_name
 
 set MIFLAGS "-i=mi"
 
+set mi_warnings "(&\".*\"\r\n)*"
+
 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary
 #
@@ -780,6 +782,7 @@ proc mi_run_cmd {args} {
 	return -1
     }
     global mi_gdb_prompt
+    global mi_warnings
 
     if [target_info exists gdb_init_command] {
 	send_gdb "[target_info gdb_init_command]\n";
@@ -821,7 +824,7 @@ proc mi_run_cmd {args} {
 
     send_gdb "220-exec-run $args\n"
     gdb_expect {
-	-re "220\\^running\r\n${mi_gdb_prompt}" {
+	-re "220\\^running\r\n${mi_warnings}${mi_gdb_prompt}" {
 	}
 	timeout {
 	    perror "Unable to start target"

Re: MI testsuite fix

[Luis Machado]

Thanks for the patch.

On Thu, 2008-04-10 at 21:02 +0100, Pedro Alves wrote:
> Fixing this leaves me with one MI regression, mi-pending.exp, which is 
> related to throwing an exception running the exec cleanups, which deletes
> the MI token, when it shouldn't.  That is fixed by Vladimir's pending
> "murder exec cleanup" patch, or when the token in *stopped is removed.
> Both will go in very soon.

This is still happening on PPC. So i assume this will be fixed with
Vladimir's patch as well.

There is an additional failure in the MI testsuite (PPC) when running in
asynch mode, mi-simplerun.exp and mi2-simplerun.exp, which seems to be
an output ordering issue as well.

"Correct" output:
220-exec-continue^M
220^running^M
(gdb) ^M
Hello, World!callme^M
callme^M
220*stopped,reason="exited-normally"^M
(gdb) ^M
PASS: gdb.mi/mi-simplerun.exp: continue to end

"Incorrect" output:
220-exec-continue^M
220^running^M
Hello, World!callme^M
callme^M
(gdb) ^M
FAIL: gdb.mi/mi-simplerun.exp: continue to end (failed to resume)

Correct and incorrect between "" because both ending results are valid.

This is not fixed by your latest patch, Pedro. What do you think?

Regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

Re: MI testsuite fix

[Pedro Alves]

A Thursday 10 April 2008 21:36:42, Luis Machado escreveu:

> There is an additional failure in the MI testsuite (PPC) when running in
> asynch mode, mi-simplerun.exp and mi2-simplerun.exp, which seems to be
> an output ordering issue as well.
>
> "Correct" output:
> 220-exec-continue^M
> 220^running^M
> (gdb) ^M
> Hello, World!callme^M
> callme^M
> 220*stopped,reason="exited-normally"^M
> (gdb) ^M
> PASS: gdb.mi/mi-simplerun.exp: continue to end
>
> "Incorrect" output:
> 220-exec-continue^M
> 220^running^M
> Hello, World!callme^M
> callme^M
> (gdb) ^M
> FAIL: gdb.mi/mi-simplerun.exp: continue to end (failed to resume)
>
> Correct and incorrect between "" because both ending results are valid.
>
> This is not fixed by your latest patch, Pedro. What do you think?
>

Ah, this is the one I had already analized here:
http://sourceware.org/ml/gdb-patches/2008-03/msg00293.html

The issue is that there is a race between gdb printing (gdb)
and the inferior starting to print too.  In sync mode, you
don't see it, because ^running and (gdb) and output before
starting the target:

      fputs_unfiltered ("^running\n", raw_stdout);
      fputs_unfiltered ("(gdb) \n", raw_stdout);

I don't know if the protocol specifies when is it that
a frontend should expect that the inferior starts outputing
to the console.  (gdb) should mean GDB is ready for input,
not that the inferior started, right?  Maybe GDB should
wait for a *running notification instead of (GDB), but
even so, I'm not sure every target will emit it in 
single-threaded programs.  So, to me this looks like
a testsuite deficiency.  Perhaps we should just put
a sleep (1) at the start of that test.

-- 
Pedro Alves

Re: MI testsuite fix

[Pedro Alves]

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

A Thursday 10 April 2008 21:02:07, Pedro Alves wrote:
> Running the testsuite in linux native async mode in
> an x86_64-unknown-linux-gnu with -m32 to simulate an x86-pc-linux-gnu,
> I get a bunch of MI testsuite errors related to this difference of
> output in async vs sync modes:
>
> async:
>
> 220^running
> &"warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at
> ffffe0b4\n"
> (gdb)
>
> sync:
>
> 220^running
> (gdb)
> &"warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at
> ffffe0b4\n"
>
>
> It results in errors that look like these:
>
> 220-exec-run
> 220^running
> &"warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at
> ffffe0b4\n"
> (gdb)
> ~"[Thread debugging using libthread_db enabled]\n"
> 220*stopped,thread-id="1",frame={addr="0x08048542",func="main",args=[],file
>="../../../src/gdb/testsuite/gdb.mi/pth
> reads.c",fullname="/home/pedro/gdb/track_mi/src/gdb/testsuite/gdb.mi/pthrea
>ds.c",line="79"} (gdb)
> ERROR: Unable to start target
>
> The difference comes from the fact that on sync mode, the MI prompt is
> output immediatelly after ^running, while on async mode, it is output a bit
> later.
>
>   if (!target_can_async_p ())
>     {
>       /* NOTE: For synchronous targets asynchronous behavour is faked by
>          printing out the GDB prompt before we even try to execute the
>          command.  */
>       if (last_async_command)
> 	fputs_unfiltered (last_async_command, raw_stdout);
>       fputs_unfiltered ("^running\n", raw_stdout);
>       fputs_unfiltered ("(gdb) \n", raw_stdout);
>       gdb_flush (raw_stdout);
>     }
>   else
>     {
>       /* FIXME: cagney/1999-11-29: Printing this message before
>          calling execute_command is wrong.  It should only be printed
>          once gdb has confirmed that it really has managed to send a
>          run command to the target.  */
>       if (last_async_command)
> 	fputs_unfiltered (last_async_command, raw_stdout);
>       fputs_unfiltered ("^running\n", raw_stdout);
>     }
>
> The async case looks more correct than the sync one, so I propose fixing
> the regex to match warnings before the MI prompt.
>
> It is also arguable if that warning has any value, but in any case,
> we should be filtering warnings.
>
> Fixing this leaves me with one MI regression, mi-pending.exp, which is
> related to throwing an exception running the exec cleanups, which deletes
> the MI token, when it shouldn't.  That is fixed by Vladimir's pending
> "murder exec cleanup" patch, or when the token in *stopped is removed.
> Both will go in very soon.


Ahem, that regex will probably eat to much, cli errors as well.  It
isn't because it's called mi_warnings that it will only
filter warnings (blush).

Updated patch attached.

-- 
Pedro Alves

[-- Attachment #2: mi_fix.diff --]
[-- Type: text/x-diff, Size: 1183 bytes --]

2008-04-10  Pedro Alves  <pedro@codesourcery.com>

	* lib/mi-support.exp (mi_warnings): New.
	(mi_run_cmd): Filter warnings in the -exec-run case.
	
---
 gdb/testsuite/lib/mi-support.exp |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: src/gdb/testsuite/lib/mi-support.exp
===================================================================
--- src.orig/gdb/testsuite/lib/mi-support.exp	2008-04-10 20:45:40.000000000 +0100
+++ src/gdb/testsuite/lib/mi-support.exp	2008-04-10 22:39:29.000000000 +0100
@@ -33,6 +33,8 @@ global mi_inferior_tty_name
 
 set MIFLAGS "-i=mi"
 
+set mi_warnings "(&\"warning: .*\"\r\n)*"
+
 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary
 #
@@ -780,6 +782,7 @@ proc mi_run_cmd {args} {
 	return -1
     }
     global mi_gdb_prompt
+    global mi_warnings
 
     if [target_info exists gdb_init_command] {
 	send_gdb "[target_info gdb_init_command]\n";
@@ -821,7 +824,7 @@ proc mi_run_cmd {args} {
 
     send_gdb "220-exec-run $args\n"
     gdb_expect {
-	-re "220\\^running\r\n${mi_gdb_prompt}" {
+	-re "220\\^running\r\n${mi_warnings}${mi_gdb_prompt}" {
 	}
 	timeout {
 	    perror "Unable to start target"

Re: MI testsuite fix

[Daniel Jacobowitz]

On Thu, Apr 10, 2008 at 09:02:07PM +0100, Pedro Alves wrote:
> It is also arguable if that warning has any value, but in any case,
> we should be filtering warnings.

I am not sure this is true.  Warning the user about this sort of
thing should imply something really wrong; otherwise, bothering
the user (who probably can not do anything about it except report it
to us) is not very helpful.  And during the testsuite there shouldn't
be anything wrong that the user (i.e. the testsuite harness) can't
fix.

As for the warning itself, I'd approve a patch to remove it.  I'd also
approve a patch to reuse some of the qOffsets machinery for this, but
that would take more testing... if you want to look at that,
keep in mind that SEC_LOAD doesn't mean the VMA is useful; you
also need to check !SEC_THREAD_LOCAL.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI testsuite fix

[Pedro Alves]

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

A Monday 14 April 2008 19:21:31, Daniel Jacobowitz wrote:
> On Thu, Apr 10, 2008 at 09:02:07PM +0100, Pedro Alves wrote:
> > It is also arguable if that warning has any value, but in any case,
> > we should be filtering warnings.
>
> I am not sure this is true.  Warning the user about this sort of
> thing should imply something really wrong; otherwise, bothering
> the user (who probably can not do anything about it except report it
> to us) is not very helpful.  And during the testsuite there shouldn't
> be anything wrong that the user (i.e. the testsuite harness) can't
> fix.
>

Fair enough.

> As for the warning itself, I'd approve a patch to remove it.  

Ok, that's what the attached does.  Regtested on
x86_64-unknown-linux-gnu, and confirmed that is also fixes the
async MI testsuite on x86 due to the gate page warning.

BTW, What is it that won't work with the look for .text first?

      /* Find lowest loadable section to be used as starting point for
         continguous sections. FIXME!! won't work without call to find
	 .text first, but this assumes text is lowest section. */
      lower_sect = bfd_get_section_by_name (objfile->obfd, ".text");
      if (lower_sect == NULL)
	bfd_map_over_sections (objfile->obfd, find_lowest_section,


> I'd also 
> approve a patch to reuse some of the qOffsets machinery for this, but
> that would take more testing... if you want to look at that,
> keep in mind that SEC_LOAD doesn't mean the VMA is useful; you
> also need to check !SEC_THREAD_LOCAL.

Now you've lost me.  This is mapping absolute section addresses
to section offsets.  It doesn't seem symfile_map_offsets_to_segments
or objfile_relocate would give any help here?

I could see passing OFFSETS instead ADDRS in more cases where we
want to pass addresses/offsets for all sections so we wouldn't have
to rely on sections names for addr->offset mapping.   There's the
unfortunaty that xcoff_symfile_offsets ignores ADDRS, but I'm sure
something could be done about it.

(While I was looking at it, I've reorganized the code to use
objfile_relocate after reading the symbols, instead of setting
the section offsets before reading them, but it didn't
give any simplification, so I dropped it.)

What machinery do you mean exactly?   

-- 
Pedro Alves

[-- Attachment #2: remove_warning.diff --]
[-- Type: text/x-diff, Size: 1433 bytes --]

2008-04-21  Pedro Alves  <pedro@codesourcery.com>

	* symfile.c (syms_from_objfile): Don't warn if lowest loadable
	section is not a code section.

---
 gdb/symfile.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Index: src/gdb/symfile.c
===================================================================
--- src.orig/gdb/symfile.c	2008-04-18 22:12:58.000000000 +0100
+++ src/gdb/symfile.c	2008-04-18 22:14:02.000000000 +0100
@@ -821,18 +821,13 @@ syms_from_objfile (struct objfile *objfi
 	bfd_map_over_sections (objfile->obfd, find_lowest_section,
 			       &lower_sect);
       if (lower_sect == NULL)
-	warning (_("no loadable sections found in added symbol-file %s"),
-		 objfile->name);
-      else
-	if ((bfd_get_section_flags (objfile->obfd, lower_sect) & SEC_CODE) == 0)
-	  warning (_("Lowest section in %s is %s at %s"),
-		   objfile->name,
-		   bfd_section_name (objfile->obfd, lower_sect),
-		   paddr (bfd_section_vma (objfile->obfd, lower_sect)));
-      if (lower_sect != NULL)
- 	lower_offset = bfd_section_vma (objfile->obfd, lower_sect);
+	{
+	  warning (_("no loadable sections found in added symbol-file %s"),
+		   objfile->name);
+	  lower_offset = 0;
+	}
       else
- 	lower_offset = 0;
+	lower_offset = bfd_section_vma (objfile->obfd, lower_sect);
 
       /* Calculate offsets for the loadable sections.
  	 FIXME! Sections must be in order of increasing loadable section

Re: MI testsuite fix

[Daniel Jacobowitz]

On Mon, Apr 21, 2008 at 12:35:58PM +0100, Pedro Alves wrote:
> Ok, that's what the attached does.  Regtested on
> x86_64-unknown-linux-gnu, and confirmed that is also fixes the
> async MI testsuite on x86 due to the gate page warning.
> 
> BTW, What is it that won't work with the look for .text first?

Beats me.  But it probably means find_lowest_section doesn't work
on some platforms.  e.g. the SEC_THREAD_LOCAL thing but that problem
is much newer than this code.

> > I'd also 
> > approve a patch to reuse some of the qOffsets machinery for this, but
> > that would take more testing... if you want to look at that,
> > keep in mind that SEC_LOAD doesn't mean the VMA is useful; you
> > also need to check !SEC_THREAD_LOCAL.
> 
> Now you've lost me.  This is mapping absolute section addresses
> to section offsets.  It doesn't seem symfile_map_offsets_to_segments
> or objfile_relocate would give any help here?

Well, anything that just carries offsets forwards from the previous
section is suspect.  If the user gives us the addresses of .text and
.data, it'd be nice to move the containing segments appropriately.

> 2008-04-21  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* symfile.c (syms_from_objfile): Don't warn if lowest loadable
> 	section is not a code section.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI testsuite fix

[Pedro Alves]

A Monday 21 April 2008 13:32:58, Daniel Jacobowitz wrote:
> On Mon, Apr 21, 2008 at 12:35:58PM +0100, Pedro Alves wrote:

> > Now you've lost me.  This is mapping absolute section addresses
> > to section offsets.  It doesn't seem symfile_map_offsets_to_segments
> > or objfile_relocate would give any help here?
>
> Well, anything that just carries offsets forwards from the previous
> section is suspect.  If the user gives us the addresses of .text and
> .data, it'd be nice to move the containing segments appropriately.
>

I see.  We could also give the user an option to pass segment
addresses instead of section addresses, and then it indeed starts
looking like qOffsets.

> > 2008-04-21  Pedro Alves  <pedro@codesourcery.com>
> >
> > 	* symfile.c (syms_from_objfile): Don't warn if lowest loadable
> > 	section is not a code section.
>
> OK.

Checked in.  Thanks.

-- 
Pedro Alves