[1/2] GDB test suite: Add helper for locating core files

  On Sat, Oct 07 2017, Kevin Buettner wrote:

[...]

> E.g. when I test your patch on my x86-64 linux box using the
> following command...
>
>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
>
> ...I see 32 fewer passes than before and also one more known failure.
>
> Here are the passes that no longer occur when using your patch:

[...]

> Instead, several warnings are now printed instead:
>
>     WARNING: Can not generate core dump on remote target.

These warnings are newly introduced by the patch.  They are meant to
improve diagnostics when someone attempts to run the tests on a "real"
remote target.  I wanted to clearly document the fact that this is
unsupported (and always was).  Also, by documenting this restriction,
maybe someone feels encouraged to lift it ;-)

But it seems I went overboard, now also bailing out in case of
native-gdbserver, which is unnecessary, since no extra handling is
required for that.

How to fix this, though?  Rather than bailing out on "is_remote target",
maybe we should check for "isnative" instead?  See the delta-patch
below.  This should fix the problem with native-gdbserver and is
probably not worse than before in other scenarios, so maybe it's good
enough.  WDYT?

> If you can restore support for handling of remote core files, I'd very
> much like to review this patch again.

Thanks, I'd appreciate that.

--
Andreas

-- >8 --
Subject: [PATCH] Squash into "GDB test suite: Add helper for locating core files"
  

On Mon, 09 Oct 2017 20:46:21 +0200
Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:

> > Instead, several warnings are now printed instead:
> >
> >     WARNING: Can not generate core dump on remote target.  
> 
> These warnings are newly introduced by the patch.

Yes, I saw that.

> They are meant to
> improve diagnostics when someone attempts to run the tests on a "real"
> remote target.  I wanted to clearly document the fact that this is
> unsupported (and always was).  Also, by documenting this restriction,
> maybe someone feels encouraged to lift it ;-)

In the distant past, I used to run the testsuite against resource
constrained linux machines often of a different architecture from the
host I was running the tests from.  These machines would run gdbserver
built for that architecture.

Now, I don't recall whether corefile support in the testsuite actually
worked for those targets, but it at least seems possible due to the
various invocations of remote_exec which are present (prior to your
patch).

Do you think you could restore those calls to remote_exec in your
patch?  Or do you know for a fact that they do not work?

Kevin
  

On Wed, Oct 11 2017, Kevin Buettner wrote:

> On Mon, 09 Oct 2017 20:46:21 +0200
> Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
>
>> > Instead, several warnings are now printed instead:
>> >
>> >     WARNING: Can not generate core dump on remote target.  
>> 
>> These warnings are newly introduced by the patch.
>
> Yes, I saw that.
>
>> They are meant to
>> improve diagnostics when someone attempts to run the tests on a "real"
>> remote target.  I wanted to clearly document the fact that this is
>> unsupported (and always was).  Also, by documenting this restriction,
>> maybe someone feels encouraged to lift it ;-)
>
> In the distant past, I used to run the testsuite against resource
> constrained linux machines often of a different architecture from the
> host I was running the tests from.  These machines would run gdbserver
> built for that architecture.

OK, in that scenario, core file test cases would need to be roughly
executed in four steps:

(1) Build executable.
(2) Run executable on the remote machine.
(3) Transfer core dump to the local host.
(4) Start GDB on the local host.

AFAIK, steps (2) and (3) have not been implemented for the GDB test
suite so far.

> Now, I don't recall whether corefile support in the testsuite actually
> worked for those targets, but it at least seems possible due to the
> various invocations of remote_exec which are present (prior to your
> patch).

Fairly unlikely.  See, for instance, the beginning of corefile.exp:

  # are we on a target board
  if ![isnative] then {
      return
  }

A check like this appears in all core dump test cases.

Also note that the core_find command in gdb.exp uses expect's "system"
command for invoking the executable:

  catch "system \"(cd ${coredir}; ulimit -c unlimited; ${binfile} ${arg}; true) >/dev/null 2>&1\""

Which means that this command line will be executed on the local machine
and will fail in a remote setup.

But you're right that there were remote_exec invocations; mostly for
file handling such as:

  remote_exec build "mv $i $destcore"

  remote_exec build "mv $corefile $destcore"

  remote_exec build "rmdir $coredir"

TBH, I don't quite understand their intention.  In a native remote
setup, "build" is the remote machine.  Moving the core file around there
doesn't make it appear on the local machine.  And in a cross remote
setup, "build" is the local machine, whereas the core dump should be
generated on the remote machine.  So I think these commands only work if
host == build == target.  Since I found this too misleading, I replaced
them by local commands.

> Do you think you could restore those calls to remote_exec in your
> patch?  Or do you know for a fact that they do not work?

I'm pretty sure they don't.  Thus I wanted to document this restriction
more clearly.

--
Andreas
  

On 10/09/2017 07:46 PM, Andreas Arnez wrote:

> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>  # specified.  Return that path name, or "" if no core file was found.
>  
>  proc find_core {binfile coredir {destcore ""}} {
> -    if {[is_remote target]} {
> +    if {![isnative]} {
>  	warning "Can not access remote core file."
>  	return ""
>      }

This seems incorrect to me.  "isnative" only checks
if the build and target _triplets_ are the same.  So
foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
machine still returns isnative==true.

I think the real problem is that the native-gdbserver board
returns true to is_remote, when I think it shouldn't.

Doing that alone results in fallout in the testsuite, of
course.  I'm seeing if fixing it is doable.

Thanks,
Pedro Alves
  

On Thu, Oct 12 2017, Pedro Alves wrote:

> On 10/09/2017 07:46 PM, Andreas Arnez wrote:
>
>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>>  # specified.  Return that path name, or "" if no core file was found.
>>  
>>  proc find_core {binfile coredir {destcore ""}} {
>> -    if {[is_remote target]} {
>> +    if {![isnative]} {
>>  	warning "Can not access remote core file."
>>  	return ""
>>      }
>
> This seems incorrect to me.  "isnative" only checks
> if the build and target _triplets_ are the same.  So
> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
> machine still returns isnative==true.

Exactly, that's why I originally wrote is_remote instead.  And I also
wondered why the core dump tests check isnative.  Does anyone run the
testsuite on a native remote setup?

> I think the real problem is that the native-gdbserver board
> returns true to is_remote, when I think it shouldn't.
>
> Doing that alone results in fallout in the testsuite, of
> course.  I'm seeing if fixing it is doable.

Right, this seems all a bit mixed up to me, similar to the confusing
remote_file and remote_exec operations in core_find.

--
Andreas
  

On Thu, 12 Oct 2017, Andreas Arnez wrote:

> > This seems incorrect to me.  "isnative" only checks
> > if the build and target _triplets_ are the same.  So
> > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
> > machine still returns isnative==true.
> 
> Exactly, that's why I originally wrote is_remote instead.  And I also
> wondered why the core dump tests check isnative.  Does anyone run the
> testsuite on a native remote setup?

 Well, I had cases where I did that, and offhand I can find two reasons:

1. You want to verify `gdbserver' itself rather than GDB, in which case 
   you may even run it locally (i.e. where the remote target is really 
   `localhost').

2. You want to test a feature (e.g. an extra register set) only your 
   target system has and it is too slow or unequipped to run DejaGNU 
   itself.

  Maciej
  

On Fri, Oct 13 2017, Maciej W. Rozycki wrote:

> On Thu, 12 Oct 2017, Andreas Arnez wrote:
>
>> > This seems incorrect to me.  "isnative" only checks
>> > if the build and target _triplets_ are the same.  So
>> > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
>> > machine still returns isnative==true.
>> 
>> Exactly, that's why I originally wrote is_remote instead.  And I also
>> wondered why the core dump tests check isnative.  Does anyone run the
>> testsuite on a native remote setup?
>
>  Well, I had cases where I did that, and offhand I can find two reasons:
>
> 1. You want to verify `gdbserver' itself rather than GDB, in which case 
>    you may even run it locally (i.e. where the remote target is really 
>    `localhost').

This is the native-gdbserver setup, right?  In that case the core dump
tests should work...

> 2. You want to test a feature (e.g. an extra register set) only your 
>    target system has and it is too slow or unequipped to run DejaGNU 
>    itself.

...and in this case they shouldn't.  Do you remember whether you saw
FAILs from corefile.exp and friends?

--
Andreas
  

On 10/09/2017 07:46 PM, Andreas Arnez wrote:
> On Sat, Oct 07 2017, Kevin Buettner wrote:
> 
> [...]
> 
>> E.g. when I test your patch on my x86-64 linux box using the
>> following command...
>>
>>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
>>
>> ...I see 32 fewer passes than before and also one more known failure.
>>
>> Here are the passes that no longer occur when using your patch:
> 
> [...]
> 
>> Instead, several warnings are now printed instead:
>>
>>     WARNING: Can not generate core dump on remote target.
> 
> These warnings are newly introduced by the patch.  They are meant to
> improve diagnostics when someone attempts to run the tests on a "real"
> remote target.  I wanted to clearly document the fact that this is
> unsupported (and always was).  Also, by documenting this restriction,
> maybe someone feels encouraged to lift it ;-)

Wouldn't an UNTESTED or UNSUPPORTED be better?  It's what
we tend to do with other cases of unsupported/untested tests.
Those convey what happened to the testscase, and are
accounted for and an untested/unsupported count is shown at
the end of a test run, while a count of warnings isn't.

Thanks,
Pedro Alves
  

On 10/12/2017 06:00 PM, Andreas Arnez wrote:
> On Thu, Oct 12 2017, Pedro Alves wrote:
> 
>> On 10/09/2017 07:46 PM, Andreas Arnez wrote:
>>
>>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>>>  # specified.  Return that path name, or "" if no core file was found.
>>>  
>>>  proc find_core {binfile coredir {destcore ""}} {
>>> -    if {[is_remote target]} {
>>> +    if {![isnative]} {
>>>  	warning "Can not access remote core file."
>>>  	return ""
>>>      }
>>
>> This seems incorrect to me.  "isnative" only checks
>> if the build and target _triplets_ are the same.  So
>> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
>> machine still returns isnative==true.
> 
> Exactly, that's why I originally wrote is_remote instead.  And I also
> wondered why the core dump tests check isnative.  Does anyone run the
> testsuite on a native remote setup?

I think most if not all isnative checks are stale/bogus,
and likely were introduced at a time when cross debugging,
including cross-core debugging wasn't that well supported.
I'd be delighted to see them cleaned up and replaced with
a more appropriate check.

> Right, this seems all a bit mixed up to me, similar to the confusing
> remote_file and remote_exec operations in core_find.

I wonder whether it'd be equally simple to move in the opposite
direction of making these operations use the right FOO
"remote_exec FOO", making it easier to actually support remote
core dump testing.  (Maybe not by default, since uploading the
cores may be slow, but with a board option/setting.)

Thanks,
Pedro Alves
  

On Fri, 13 Oct 2017, Andreas Arnez wrote:

> >> Exactly, that's why I originally wrote is_remote instead.  And I also
> >> wondered why the core dump tests check isnative.  Does anyone run the
> >> testsuite on a native remote setup?
> >
> >  Well, I had cases where I did that, and offhand I can find two reasons:
> >
> > 1. You want to verify `gdbserver' itself rather than GDB, in which case 
> >    you may even run it locally (i.e. where the remote target is really 
> >    `localhost').
> 
> This is the native-gdbserver setup, right?  In that case the core dump
> tests should work...

 Yes and indeed.

> > 2. You want to test a feature (e.g. an extra register set) only your 
> >    target system has and it is too slow or unequipped to run DejaGNU 
> >    itself.
> 
> ...and in this case they shouldn't.  Do you remember whether you saw
> FAILs from corefile.exp and friends?

 Nope, sorry.  It was a while ago and I only looked for regressions with 
whatever I meant to verify and not preexisting failures anyway.

  Maciej
  

On Tue, 17 Oct 2017, Pedro Alves wrote:

> >> E.g. when I test your patch on my x86-64 linux box using the
> >> following command...
> >>
> >>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
> >>
> >> ...I see 32 fewer passes than before and also one more known failure.
> >>
> >> Here are the passes that no longer occur when using your patch:
> > 
> > [...]
> > 
> >> Instead, several warnings are now printed instead:
> >>
> >>     WARNING: Can not generate core dump on remote target.
> > 
> > These warnings are newly introduced by the patch.  They are meant to
> > improve diagnostics when someone attempts to run the tests on a "real"
> > remote target.  I wanted to clearly document the fact that this is
> > unsupported (and always was).  Also, by documenting this restriction,
> > maybe someone feels encouraged to lift it ;-)
> 
> Wouldn't an UNTESTED or UNSUPPORTED be better?  It's what
> we tend to do with other cases of unsupported/untested tests.

 I think UNSUPPORTED is the right status; UNTESTED is meant for missing 
tests really; see:

UNTESTED
    A test was not run.  This is a placeholder, used when there is no real 
    test case yet.

vs:

UNSUPPORTED
    There is no support for the tested case. This may mean that a 
    conditional feature of an operating system, or of a compiler, is not 
    implemented.  DejaGnu also uses this message when a testing 
    environment (often a "bare board" target) lacks basic support for 
    compiling or running the test case.  For example, a test for the 
    system subroutine gethostname would never work on a target board 
    running only a boot monitor.

(taken from <http://www.delorie.com/gnu/docs/dejagnu/dejagnu_6.html>).

 FWIW,

  Maciej
  

Hi Maciej,

On 10/17/2017 07:19 PM, Maciej W. Rozycki wrote:
> On Tue, 17 Oct 2017, Pedro Alves wrote:

>> Wouldn't an UNTESTED or UNSUPPORTED be better?  It's what
>> we tend to do with other cases of unsupported/untested tests.
> 
>  I think UNSUPPORTED is the right status; UNTESTED is meant for missing 
> tests really; see:
> 
> UNTESTED
>     A test was not run.  This is a placeholder, used when there is no real 
>     test case yet.
> 
> vs:
> 
> UNSUPPORTED
>     There is no support for the tested case. This may mean that a 
>     conditional feature of an operating system, or of a compiler, is not 
>     implemented.  DejaGnu also uses this message when a testing 
>     environment (often a "bare board" target) lacks basic support for 
>     compiling or running the test case.  For example, a test for the 
>     system subroutine gethostname would never work on a target board 
>     running only a boot monitor.
> 

IMHO, this "placeholder" status of UNTESTED is pretty useless, at
least for GDB.  I don't recall ever adding such a placeholder
testcase, and I don't think GDB uses it like that.

I think a more useful distinction would be:

- UNSUPPORTED to indicate that the feature isn't supported by GDB or
  the remote stub.  E.g., in this case we'd use it if the GDB port
  does not support debugging core dumps at all.

- UNTESTED to indicate that the feature is supported by GDB but
  the test wasn't run because it's not possible to run it,
  or we choose to not run it, in the current test environment, e.g.,
  because of a board limitation.  In this case, we skip core tests
  when the host or target boards are remote, even though
  the feature is supported by GDB (even with "target remote" and
  cross debugging, at least on some ports).

We use UNTESTED in many places already meaning something like
the above (instead of placeholder status), though I'm not aware
of rationale written down anywhere.  The above is just my
intuition, and most of these untested/unsupported calls predate
my involvement with GDB.

Thanks,
Pedro Alves