[pushed] Fix gdbserver/MI testing regression (was: Re: [PATCH v3 31/34] Add testing infrastruture bits for running with MI on a separate UI)

  On 06/29/2016 11:50 AM, Pedro Alves wrote:
> On 06/28/2016 09:19 PM, Simon Marchi wrote:
>> I added a few traces to understand what's happening, and it seems that inferior_spawn_id
>> is being unset at two places:

Indeed.

>> The easy way would be to add a "info exists" check before unsetting it, but I don't know if
>> that would only hide a real problem.

I went ahead and pushed the patch below, with takes an even easier route.

I think we'll need to do something better in at least a couple scenarios:

 - If/when gdbserver learns about "set inferior-tty", the separate
   inferior tty spawn id should be used instead of gdbserver's.  gdbserver
   doesn't support that, so in tests that use that (or MI's equivalent),
   inferior output indeed is sent to gdbserver's tty.

 - Tests that disconnect from gdbserver/restart gdb/reconnect to gdbserver
   should end up inferior_spawn_id set to gdbserver's spawn id,
   otherwise tests that rely on inferior I/O, after the reconnect won't work
   properly.  I think there's no such test currently, though, so I'm ignoring
   this for now.

From 038d48680941f014349256aeb7bab14b3f01d58e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 30 Jun 2016 11:55:22 +0100
Subject: [PATCH] Fix gdbserver/MI testing regression

Commit 51f77c3704a6 ("Add testing infrastruture bits for running with
MI on a separate UI") broke MI testing with native-gdbserver:

 $ make check RUNTESTFLAGS="--target_board=native-gdbserver mi-var-child.exp"
	 ...
 Running .../src/binutils-gdb/gdb/testsuite/gdb.mi/mi-var-child.exp ...
 can't unset "inferior_spawn_id": no such variable
     while executing
 "unset inferior_spawn_id"
     (procedure "close_gdbserver" line 20)
     invoked from within
 "close_gdbserver"
 ...

When testing with gdbserver, gdb_exit is overridden with a special
version that calls close_gdbserver, which clears inferior_spawn_id.
The problem is that the commit mentioned above made
gdb_exit/mi_gdb_exit clear inferior_spawn_id too, and clearing a
non-existing variable is a tcl error.

Since gdb_exit/mi_gdb_exit always clears inferior_spawn_id now, the
fix is simply to stop clearing it in close_gdbserver.

gdb/testsuite/
2016-06-30  Pedro Alves  <palves@redhat.com>

	* lib/gdbserver-support.exp (close_gdbserver, gdb_exit): Don't
	unset inferior_spawn_id.
---
 gdb/testsuite/ChangeLog                 | 5 +++++
 gdb/testsuite/lib/gdbserver-support.exp | 6 ++----
 2 files changed, 7 insertions(+), 4 deletions(-)
  

BTW, while testing this, I noticed that all gdb.ada/ tests that build
a binary with the same name as their test directory are broken.
I suspect this is a regression caused by one of the gdb_remote_download,
etc. patches.

For example:

 Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/formatted_ref.exp ...
 PASS: gdb.ada/formatted_ref.exp: compilation formatted_ref.adb
 FAIL: gdb.ada/formatted_ref.exp: print/x s
 FAIL: gdb.ada/formatted_ref.exp: print/x s'access
 FAIL: gdb.ada/formatted_ref.exp: print s.x = 13

gdb.log shows:

 The program is not being run.
 (gdb) file /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref/formatted_ref
 Reading symbols from /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref/formatted_ref...done.
...
 (gdb) spawn ../gdbserver/gdbserver --once :2346 /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref
 Process /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref created; pid = 7553
 Cannot exec /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref: Permission denied.

 Child exited with status 127
 No program to debug

Note how gdb loads:

  .../gdb.ada/formatted_ref/formatted_ref/formatted_ref

while gdbserver tries to load:

  .../gdb.ada/formatted_ref/formatted_ref

... which fails because it's the directory.

This is probably because gdb_remote_download only minds "tail" when
deciding the destination filename:

proc gdb_remote_download {dest fromfile {tofile {}}} {
    # If TOFILE is not given, default to the same filename as FROMFILE.
    if {[string length $tofile] == 0} {
	set tofile [file tail $fromfile]
    }

    if {[is_remote $dest]} {
	# When the DEST is remote, we simply send the file to DEST.
	global cleanfiles

	set destname [remote_download $dest $fromfile $tofile]
	lappend cleanfiles $destname

	return $destname


and then gdbserver-base.exp does:

proc ${board}_download { board host dest } {
    # We pass DEST in standard_output_file, regardless of whether it is absolute
    # or relative, because we don't want the tests to be able to write outside
    # their standard output directory.
    set dest [standard_output_file $dest]

    file copy -force $host $dest

    return $dest
}

Here $dest already exists when we get here -- it's the directory.

I haven't confirmed, just inspected the code.

Not sure exactly what the best fix is.  Maybe simply tweak
the tests to never get into this situation, along with
making standard_ada_testfile issue perror when it happens?

Most tests pass an explicit base_file name to standard_ada_testfile
that avoids the conflict, like:

  gdb.ada/str_ref_cmp.exp:18:standard_ada_testfile foo

Maybe standard_ada_testfile's $base_file parameter should be
defaulted, like:

 -proc standard_ada_testfile {base_file {dir ""}} {
 +proc standard_ada_testfile {{base_file "test"} {dir ""}} {

and then most tests adjusted to not pass an explicit
base_file at all.

Thanks,
Pedro Alves
  

On 16-06-30 07:12 AM, Pedro Alves wrote:
> On 06/29/2016 11:50 AM, Pedro Alves wrote:
>> On 06/28/2016 09:19 PM, Simon Marchi wrote:
>>> I added a few traces to understand what's happening, and it seems that inferior_spawn_id
>>> is being unset at two places:
> 
> Indeed.
> 
>>> The easy way would be to add a "info exists" check before unsetting it, but I don't know if
>>> that would only hide a real problem.
> 
> I went ahead and pushed the patch below, with takes an even easier route.
> 
> I think we'll need to do something better in at least a couple scenarios:
> 
>  - If/when gdbserver learns about "set inferior-tty", the separate
>    inferior tty spawn id should be used instead of gdbserver's.  gdbserver
>    doesn't support that, so in tests that use that (or MI's equivalent),
>    inferior output indeed is sent to gdbserver's tty.
> 
>  - Tests that disconnect from gdbserver/restart gdb/reconnect to gdbserver
>    should end up inferior_spawn_id set to gdbserver's spawn id,
>    otherwise tests that rely on inferior I/O, after the reconnect won't work
>    properly.  I think there's no such test currently, though, so I'm ignoring
>    this for now.

Thanks!  Your patch fixes the problem I initially encountered.

Simon
  

On 16-06-30 08:10 AM, Pedro Alves wrote:
> BTW, while testing this, I noticed that all gdb.ada/ tests that build
> a binary with the same name as their test directory are broken.
> I suspect this is a regression caused by one of the gdb_remote_download,
> etc. patches.
> 
> For example:
> 
>  Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/formatted_ref.exp ...
>  PASS: gdb.ada/formatted_ref.exp: compilation formatted_ref.adb
>  FAIL: gdb.ada/formatted_ref.exp: print/x s
>  FAIL: gdb.ada/formatted_ref.exp: print/x s'access
>  FAIL: gdb.ada/formatted_ref.exp: print s.x = 13
> 
> gdb.log shows:
> 
>  The program is not being run.
>  (gdb) file /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref/formatted_ref
>  Reading symbols from /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref/formatted_ref...done.
> ...
>  (gdb) spawn ../gdbserver/gdbserver --once :2346 /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref
>  Process /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref created; pid = 7553
>  Cannot exec /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.ada/formatted_ref/formatted_ref: Permission denied.
> 
>  Child exited with status 127
>  No program to debug
> 
> Note how gdb loads:
> 
>   .../gdb.ada/formatted_ref/formatted_ref/formatted_ref
> 
> while gdbserver tries to load:
> 
>   .../gdb.ada/formatted_ref/formatted_ref
> 
> ... which fails because it's the directory.
> 
> This is probably because gdb_remote_download only minds "tail" when
> deciding the destination filename:
> 
> proc gdb_remote_download {dest fromfile {tofile {}}} {
>     # If TOFILE is not given, default to the same filename as FROMFILE.
>     if {[string length $tofile] == 0} {
> 	set tofile [file tail $fromfile]
>     }
> 
>     if {[is_remote $dest]} {
> 	# When the DEST is remote, we simply send the file to DEST.
> 	global cleanfiles
> 
> 	set destname [remote_download $dest $fromfile $tofile]
> 	lappend cleanfiles $destname
> 
> 	return $destname
> 
> 
> and then gdbserver-base.exp does:
> 
> proc ${board}_download { board host dest } {
>     # We pass DEST in standard_output_file, regardless of whether it is absolute
>     # or relative, because we don't want the tests to be able to write outside
>     # their standard output directory.
>     set dest [standard_output_file $dest]
> 
>     file copy -force $host $dest
> 
>     return $dest
> }
> 
> Here $dest already exists when we get here -- it's the directory.
> 
> I haven't confirmed, just inspected the code.

I think you are right.

> Not sure exactly what the best fix is.  Maybe simply tweak
> the tests to never get into this situation, along with
> making standard_ada_testfile issue perror when it happens?
> 
> Most tests pass an explicit base_file name to standard_ada_testfile
> that avoids the conflict, like:
> 
>   gdb.ada/str_ref_cmp.exp:18:standard_ada_testfile foo
> 
> Maybe standard_ada_testfile's $base_file parameter should be
> defaulted, like:
> 
>  -proc standard_ada_testfile {base_file {dir ""}} {
>  +proc standard_ada_testfile {{base_file "test"} {dir ""}} {
> 
> and then most tests adjusted to not pass an explicit
> base_file at all.
> 
> Thanks,
> Pedro Alves
> 

Is there a reason have a double level of directories named after the test?

outputs
└── gdb.ada
    └── formatted_ref
        └── formatted_ref
            ├── b~formatted_ref.adb
            ├── b~formatted_ref.ads
            ├── b~formatted_ref.ali
            ├── b~formatted_ref.o
            ├── defs.ali
            ├── defs.o
            ├── formatted_ref
            ├── formatted_ref.ali
            └── formatted_ref.o

If we removed the extra "formatted_ref" level, we wouldn't have the problem at all.
The source and destination would become identical when running locally, just like
when testing C/C++.  Or maybe it's needed for some reason in Ada?

I made this patch quickly:
https://github.com/simark/binutils-gdb/commit/d451ff22f1980d4a89c5d6d126bc839734fa393f

A few tests that assume the current directory layout need to be adjusted, but other
than that it looks fine.  I'll clean it up, give it more testing and post it tomorrow,
unless you say it's a wrong approach in the mean time.
  

> Is there a reason have a double level of directories named after the test?
> 
> outputs
> └── gdb.ada
>     └── formatted_ref
>         └── formatted_ref

Initially, I thought this was an obvious mistake. But after further
thought, I think it now expected and done for all testcases, including
the testscases where the main has a name that's different from
the name of the testcase.

I'm looking at Pedro's email, now, as I think the issue is indeed
confined to when testing remotely.
  

> > Is there a reason have a double level of directories named after the test?
> > 
> > outputs
> > └── gdb.ada
> >     └── formatted_ref
> >         └── formatted_ref
> 
> Initially, I thought this was an obvious mistake. But after further
> thought, I think it now expected and done for all testcases, including
> the testscases where the main has a name that's different from
> the name of the testcase.

When I started writing the email, I meant to give a little more
info than that.

The root "formatted_ref" is because the the .exp name. The second
is because gdb.ada organizes its test programs inside one subdirectory
per testcase, where the subdirectory name is the name of the testcase.
Hence the same second subdirectory, whose name is the same as the
first one.
  

> This is probably because gdb_remote_download only minds "tail" when
> deciding the destination filename:
> 
> proc gdb_remote_download {dest fromfile {tofile {}}} {
>     # If TOFILE is not given, default to the same filename as FROMFILE.
>     if {[string length $tofile] == 0} {
> 	set tofile [file tail $fromfile]
>     }

Looks like this code is indirectly assuming that the code is in the same
directory as the .exp. That's why it can extract the target directory
via a simple "tail".  I tried looking at whether we could make sure
"tofile" was explicitly passed at least for Ada, but it doesn't look
really like an option, as I think this comes from gdb_file_cmd where,
logically, all we care about is which executable to load into GDB.

I am running out of time to investigate this for now, but to me,
thinking this further, I think the most promising avenue is probably
to look at eliminating the subdirectory, since our testing procedure
now creates one per test already. But I'm not sure how this is going
to affect in-tree testing.
  

On 16-07-05 12:36 PM, Joel Brobecker wrote:
>> This is probably because gdb_remote_download only minds "tail" when
>> deciding the destination filename:
>>
>> proc gdb_remote_download {dest fromfile {tofile {}}} {
>>     # If TOFILE is not given, default to the same filename as FROMFILE.
>>     if {[string length $tofile] == 0} {
>> 	set tofile [file tail $fromfile]
>>     }
> 
> Looks like this code is indirectly assuming that the code is in the same
> directory as the .exp. That's why it can extract the target directory
> via a simple "tail".  I tried looking at whether we could make sure
> "tofile" was explicitly passed at least for Ada, but it doesn't look
> really like an option, as I think this comes from gdb_file_cmd where,
> logically, all we care about is which executable to load into GDB.

Hi Joel,

I posted this patch this morning, I should have CC'ed you:
https://sourceware.org/ml/gdb-patches/2016-07/msg00068.html

I think it solves it.

> I am running out of time to investigate this for now, but to me,
> thinking this further, I think the most promising avenue is probably
> to look at eliminating the subdirectory, since our testing procedure
> now creates one per test already. But I'm not sure how this is going
> to affect in-tree testing.

I think it will work regardless of in-tree/out-of-tree, since we always use the
same layout, under the "outputs" directory.  I tested it before sending my patch
and it seemed to work fine.

Simon
  

Hi Simon,

> I posted this patch this morning, I should have CC'ed you:
> https://sourceware.org/ml/gdb-patches/2016-07/msg00068.html
> 
> I think it solves it.

I think you are right, and in the way that I was leaning towards.

I got confused when I first saw the patch, as the meat of the patch
in lib/ada.exp was kind of obscured because it is at the end after
a number of changes I couldn't really understand (because I hadn't
seen the part in ada.exp).

I didn't think it would be so easy. That looks pretty good to me,
and indeed, now that everything is done in the "outputs" directory,
that should solve the issue of in-tree builds.

Go ahead, and push.
Thanks for the fix!
  

On 16-07-06 09:23 AM, Joel Brobecker wrote:
> Hi Simon,
> 
>> I posted this patch this morning, I should have CC'ed you:
>> https://sourceware.org/ml/gdb-patches/2016-07/msg00068.html
>>
>> I think it solves it.
> 
> I think you are right, and in the way that I was leaning towards.
> 
> I got confused when I first saw the patch, as the meat of the patch
> in lib/ada.exp was kind of obscured because it is at the end after
> a number of changes I couldn't really understand (because I hadn't
> seen the part in ada.exp).
> 
> I didn't think it would be so easy. That looks pretty good to me,
> and indeed, now that everything is done in the "outputs" directory,
> that should solve the issue of in-tree builds.
> 
> Go ahead, and push.
> Thanks for the fix!
> 

Ok, I pushed it.

I also pushed this one to fix an instance I forgot.
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=986cf455bfb25d8696232695fbcc93649c10a523
  

On 07/05/2016 06:19 PM, Simon Marchi wrote:

> I think it will work regardless of in-tree/out-of-tree, since we always use the
> same layout, under the "outputs" directory.  I tested it before sending my patch
> and it seemed to work fine.
> 

Thanks for fixing this.  Glad that we could normalize things a
bit more.

Thanks,
Pedro Alves