[testsuite patch] Skip py-unwind.exp on x86_64 -m32

[testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Jan Kratochvil]

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

Hi,

(gdb) source /home/jkratoch/redhat/gdb-clean/gdb/testsuite/outputs/gdb.python/py-unwind/py-unwind.py^M
Python script imported^M
Python Exception <type 'exceptions.ValueError'> Bad register: ^M
(gdb) FAIL: gdb.python/py-unwind.exp: import python scripts

class TestUnwinder(Unwinder):
    AMD64_RBP = 6
    AMD64_RSP = 7
    AMD64_RIP = 16

This was already discussed here:
	Re: [testsuite patch] Fix gdb.btrace/tailcall-only.exp errors on x86_64-m32
	https://sourceware.org/ml/gdb-patches/2016-04/msg00222.html
	Message-ID: <20160411195537.GA22299@host1.jankratochvil.net>
but no GDB maintainer gave an answer how to run testsuite in cross-arch mode.


Jan

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

gdb/testsuite/ChangeLog
2016-07-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.python/py-unwind.exp: Test also ![is_lp64_target].

diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 5172a03..e31a472 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -28,7 +28,7 @@ if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
 if { [skip_python_tests] } { continue }
 
 # This test runs on a specific platform.
-if { ! [istarget x86_64-*]} { continue }
+if { ![istarget x86_64-*-* ] || ![is_lp64_target] } { continue }
 
 # The following tests require execution.
 

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Yao Qi]

On Sun, Jul 17, 2016 at 3:30 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi,
>
> (gdb) source /home/jkratoch/redhat/gdb-clean/gdb/testsuite/outputs/gdb.python/py-unwind/py-unwind.py^M
> Python script imported^M
> Python Exception <type 'exceptions.ValueError'> Bad register: ^M
> (gdb) FAIL: gdb.python/py-unwind.exp: import python scripts
>
> class TestUnwinder(Unwinder):
>     AMD64_RBP = 6
>     AMD64_RSP = 7
>     AMD64_RIP = 16
>
> This was already discussed here:
>         Re: [testsuite patch] Fix gdb.btrace/tailcall-only.exp errors on x86_64-m32
>         https://sourceware.org/ml/gdb-patches/2016-04/msg00222.html
>         Message-ID: <20160411195537.GA22299@host1.jankratochvil.net>
> but no GDB maintainer gave an answer how to run testsuite in cross-arch mode.
>
>

This problem is slightly different from "how to run testsuite in cross-arch
mode", IMO.
py-unwind.py defines a unwinder, in an arch-specific way.  It has nothing
wrong.
However, py-unwind.py should be more portable, which means, it should
define unwdiner for each arch it supports, and py-unwind.exp or py-unwind.py
chooses the right python unwinder according to the arch.  IOW, we need to
define a python unwinder for i386, and use it when arch is i386.

-- 
Yao (齐尧)

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Jan Kratochvil]

On Mon, 18 Jul 2016 12:04:27 +0200, Yao Qi wrote:
> This problem is slightly different from "how to run testsuite in cross-arch
> mode", IMO.
> py-unwind.py defines a unwinder, in an arch-specific way.  It has nothing
> wrong.
> However, py-unwind.py should be more portable, which means, it should
> define unwdiner for each arch it supports, and py-unwind.exp or py-unwind.py
> chooses the right python unwinder according to the arch.  IOW, we need to
> define a python unwinder for i386, and use it when arch is i386.

There will always exist at least one unsupported arch for an arch-specific
testcase.  I do not think it makes sense to say that very every testcase in
the testsuite must support very every arch supported by GDB.

So we can say that py-unwind.exp just does not support arch i386.
I do not find that wrong.

Wrong is that it should not FAIL on unsupported arch, it should skip the
testcase on unsupported arch.

The problem here is that py-unwind.exp thinks that it runs on arch x86_64 but
it runs on arch i386.

Even if py-unwind.exp did support i386 it would still FAIL because it would
run the testcase for %rbp/%rsp/%rip.

That is I do not agree with your mail.


In fact I think the cross-arch running of the GDB testsuite should be
described at
	https://sourceware.org/gdb/wiki/TestingGDB
but currently it is not.  Currently there is only
	https://sourceware.org/gdb/wiki/TestingGDB#Changing_the_compiler_used_to_build_the_testcases
which uses the simple way I currently use and which breaks the testsuite
needing the ![is_lp64_target] additional test.  Maybe GDB maintainers could
say that the only valid way to run the testsuite cross-arch is properly
setting arget_triplet as Markus Metzger suggests.  (But then I would need to
fix many scripts of mine; also not sure if Sergio's Buildbot runs it that
way.)


Jan

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Yao Qi]

On Mon, Jul 18, 2016 at 12:33 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Mon, 18 Jul 2016 12:04:27 +0200, Yao Qi wrote:
>> This problem is slightly different from "how to run testsuite in cross-arch
>> mode", IMO.
>> py-unwind.py defines a unwinder, in an arch-specific way.  It has nothing
>> wrong.
>> However, py-unwind.py should be more portable, which means, it should
>> define unwdiner for each arch it supports, and py-unwind.exp or py-unwind.py
>> chooses the right python unwinder according to the arch.  IOW, we need to
>> define a python unwinder for i386, and use it when arch is i386.
>
> There will always exist at least one unsupported arch for an arch-specific
> testcase.  I do not think it makes sense to say that very every testcase in
> the testsuite must support very every arch supported by GDB.
>
> So we can say that py-unwind.exp just does not support arch i386.
> I do not find that wrong.

It is still better to get one test case cover one more arch.

>
> Wrong is that it should not FAIL on unsupported arch, it should skip the
> testcase on unsupported arch.
>

IMO, it is wrong that py-unwind.py creates an x86_64 specific unwinder.
py-unwind.py should create a unwinder instance according to the arch if the
arch is supported.  On i386, or other archs, like arm, mips, py-unwind.py
can error, and py-unwind.exp knows unwinder is not created successfully,
and mark the test unsupported.  If people want to extend py-unwind.py for
their archs, they can modify py-unwind.py to create an unwinder instance
for their own arch.

> The problem here is that py-unwind.exp thinks that it runs on arch x86_64 but
> it runs on arch i386.
>
> Even if py-unwind.exp did support i386 it would still FAIL because it would
> run the testcase for %rbp/%rsp/%rip.

py-unwind.exp does nothing on arch specific thing, so py-unwind.exp shouldn't
be aware of the arch difference, but py-unwind.py should.

-- 
Yao (齐尧)

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Pedro Alves]

On 07/19/2016 11:06 AM, Yao Qi wrote:

> IMO, it is wrong that py-unwind.py creates an x86_64 specific unwinder.

I find the "wrong" qualifier a bit too strong, since it is probably
not possible to make this sort of test completely arch-independent.

> py-unwind.py should create a unwinder instance according to the arch if the
> arch is supported.  On i386, or other archs, like arm, mips, py-unwind.py
> can error, and py-unwind.exp knows unwinder is not created successfully,
> and mark the test unsupported.  If people want to extend py-unwind.py for
> their archs, they can modify py-unwind.py to create an unwinder instance
> for their own arch.
> 
>> The problem here is that py-unwind.exp thinks that it runs on arch x86_64 but
>> it runs on arch i386.
>>
>> Even if py-unwind.exp did support i386 it would still FAIL because it would
>> run the testcase for %rbp/%rsp/%rip.
> 
> py-unwind.exp does nothing on arch specific thing, so py-unwind.exp shouldn't
> be aware of the arch difference, but py-unwind.py should.
> 

Looks like py-unwind.c is ABI-specific as well, and that there's not much code
that can be shared between architectures in py-unwind.py, though.  It may be we'd
end up with separate py-unwind-$arch.py|c files even.  

How about we handle this in the .exp file for now and leave something
more complicated for when the test is first ported to some other
arch.  WDYT?

Thanks,
Pedro Alves

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Yao Qi]

On Tue, Jul 19, 2016 at 7:04 PM, Pedro Alves <palves@redhat.com> wrote:
> On 07/19/2016 11:06 AM, Yao Qi wrote:
>
>> IMO, it is wrong that py-unwind.py creates an x86_64 specific unwinder.
>
> I find the "wrong" qualifier a bit too strong, since it is probably
> not possible to make this sort of test completely arch-independent.
>

At least py-unwinder.py should create unwinder for each arch, or we should
rename it to py-unwinder-$arch.py, as you said.

>> py-unwind.py should create a unwinder instance according to the arch if the
>> arch is supported.  On i386, or other archs, like arm, mips, py-unwind.py
>> can error, and py-unwind.exp knows unwinder is not created successfully,
>> and mark the test unsupported.  If people want to extend py-unwind.py for
>> their archs, they can modify py-unwind.py to create an unwinder instance
>> for their own arch.
>>
>>> The problem here is that py-unwind.exp thinks that it runs on arch x86_64 but
>>> it runs on arch i386.
>>>
>>> Even if py-unwind.exp did support i386 it would still FAIL because it would
>>> run the testcase for %rbp/%rsp/%rip.
>>
>> py-unwind.exp does nothing on arch specific thing, so py-unwind.exp shouldn't
>> be aware of the arch difference, but py-unwind.py should.
>>
>
> Looks like py-unwind.c is ABI-specific as well, and that there's not much code
> that can be shared between architectures in py-unwind.py, though.  It may be we'd
> end up with separate py-unwind-$arch.py|c files even.

Agreed.

>
> How about we handle this in the .exp file for now and leave something
> more complicated for when the test is first ported to some other
> arch.  WDYT?
>

Well, that is fine by me, because I don't plan to port this test to
arm or aarch64
recently.

-- 
Yao (齐尧)

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Pedro Alves]

On 07/20/2016 12:20 PM, Yao Qi wrote:
>> >
> Well, that is fine by me, because I don't plan to port this test to
> arm or aarch64
> recently.

OK, thanks.

Jan, please go ahead with your patch then.

-- 
Pedro Alves

[commit] [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Jan Kratochvil]

On Wed, 20 Jul 2016 15:49:51 +0200, Pedro Alves wrote:
> Jan, please go ahead with your patch then.

Checked in:
	72b5d09937fa2dac8ca7c801b9ddefe1b0176b6f


Thanks,
Jan

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Sergio Durigan Junior]

On Monday, July 18 2016, Jan Kratochvil wrote:

> In fact I think the cross-arch running of the GDB testsuite should be
> described at
> 	https://sourceware.org/gdb/wiki/TestingGDB
> but currently it is not.  Currently there is only
> 	https://sourceware.org/gdb/wiki/TestingGDB#Changing_the_compiler_used_to_build_the_testcases
> which uses the simple way I currently use and which breaks the testsuite
> needing the ![is_lp64_target] additional test.  Maybe GDB maintainers could
> say that the only valid way to run the testsuite cross-arch is properly
> setting arget_triplet as Markus Metzger suggests.  (But then I would need to
> fix many scripts of mine; also not sure if Sergio's Buildbot runs it that
> way.)

I run the cross-arch tests like you, i.e., by specifycing
RUNTESTFLAGS='--target_board=unix/-m32'.  It should be possible to hack
our BuildBot to run the tests using arch_triplet as Markus suggested.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [testsuite patch] Skip py-unwind.exp on x86_64 -m32

[Pedro Alves]

On 07/19/2016 08:30 PM, Sergio Durigan Junior wrote:

> I run the cross-arch tests like you, i.e., by specifycing
> RUNTESTFLAGS='--target_board=unix/-m32'.  It should be possible to hack
> our BuildBot to run the tests using arch_triplet as Markus suggested.
> 

Me too.  I think '--target_board=unix/-m32'-style testing is fine
and that there's no need to change the bots.

Thanks,
Pedro Alves