s390 test fails since patch 8.2.2506

[Bram Moolenaar]

Somehow the Travis tests run fail for s390:
https://travis-ci.com/github/vim/vim/builds/217016917

I do not see this failure on other systems and valgrind and ASAN also do
not report a problem.

Does anyone have an idea of how to pinpoint this problem?


--
"Hegel was right when he said that we learn from history that man can
never learn anything from history." (George Bernard Shaw)

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[James McCoy]

On Sat, Feb 13, 2021 at 07:13:35PM +0100, Bram Moolenaar wrote:
>
> Somehow the Travis tests run fail for s390:
> https://travis-ci.com/github/vim/vim/builds/217016917
>
> I do not see this failure on other systems and valgrind and ASAN also do
> not report a problem.
>
> Does anyone have an idea of how to pinpoint this problem?

I'll see if I can replace on Debian's s390x system.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB

[James McCoy]

On Sat, Feb 13, 2021 at 04:23:02PM -0500, James McCoy wrote:
> On Sat, Feb 13, 2021 at 07:13:35PM +0100, Bram Moolenaar wrote:
> >
> > Somehow the Travis tests run fail for s390:
> > https://travis-ci.com/github/vim/vim/builds/217016917
> >
> > I do not see this failure on other systems and valgrind and ASAN also do
> > not report a problem.
> >
> > Does anyone have an idea of how to pinpoint this problem?
>
> I'll see if I can replace on Debian's s390x system.

It looks like the problematic test is Test_try_catch_throw. I was able
to whittle that down to

def Test_try_catch_throw()
var l = []
try # comment
finally # comment
add(l, '3')
endtry # comment

if 1
else
try | finally | endtry
endif

enddef

and "make test_vim9_script.res TEST_FILTER=Test_try_catch_throw"
consistently fails. I've attached the resulting valgrind log.

[Bram Moolenaar]

James McCoy wrote:

> On Sat, Feb 13, 2021 at 04:23:02PM -0500, James McCoy wrote:
> > On Sat, Feb 13, 2021 at 07:13:35PM +0100, Bram Moolenaar wrote:
> > >
> > > Somehow the Travis tests run fail for s390:
> > > https://travis-ci.com/github/vim/vim/builds/217016917
> > >
> > > I do not see this failure on other systems and valgrind and ASAN also do
> > > not report a problem.
> > >
> > > Does anyone have an idea of how to pinpoint this problem?
> >
> > I'll see if I can replace on Debian's s390x system.
>
> It looks like the problematic test is Test_try_catch_throw. I was able
> to whittle that down to
>
> def Test_try_catch_throw()
> var l = []
> try # comment
> finally # comment
> add(l, '3')
> endtry # comment
>
> if 1
> else
> try | finally | endtry
> endif
>
> enddef
>
> and "make test_vim9_script.res TEST_FILTER=Test_try_catch_throw"
> consistently fails. I've attached the resulting valgrind log.

This looks like the stack has been messed up. The first error happens
when creating a new non-empty list, which doesn't happen in
Test_try_catch_throw(). Perhaps it's after it returns with a messed up
stack.

Unfortunately I don't get any valgrind errors when I try on my system.
Perhaps you can try change the code a bit to see what matters. E.g.
change that "if 1" to "if 0". I guess the comments don't really matter.

Can you add:
disass Test_try_catch_throw
And show the output?

Otherwise it would require stepping through the function to see where it
goes wrong.

--
f y cn rd ths thn y cn hv grt jb n cmptr prgrmmng

[James McCoy]

I reduced it a little more.

def Test_try_catch_throw()
var l = []
try

finally
add(l, 0)
endtry

if 1
else
try
endtry
endif
enddef

Changing "if 1" to "if 0" or removing the finally in the first try make
the errors go away.

> I guess the comments don't really matter.
>
> Can you add:
> disass Test_try_catch_throw
> And show the output?

The assembly for the above function is:

Test_try_catch_throw
var l = []
0 NEWLIST size 8
1 STORE $0

try
2 TRY catch -> 7, finally -> 3

finally
add(l, 0)
3 LOAD $0
4 PUSHNR 0
5 LISTAPPEND
6 DROP

endtry
7 ENDTRY

if 1
else
try
endtry
endif
8 RETURN 0

Changing "add(l, 0)" to "echo l" results in slightly different behavior.

Test_try_catch_throw
var l = []
0 NEWLIST size 6
1 STORE $0

try
2 TRY catch -> 5, finally -> 3

finally
echo l
3 LOAD $0
4 ECHO 1

endtry
5 ENDTRY

if 1
else
try
endtry
endif
6 RETURN 0

I've attached both valgrind outputs.

[Bram Moolenaar]

This is where it runs into the problem: the size should be zero.
Since it's OK when the later statements are changed, I suspect that the
"isn_arg.number" value is overwritten when generating one of the later
statements. Since this is the first instruction I guess some index is
zero when filling in a jump position later. Perhaps it only fails on
s390 because it lays out the union of the struct differently.

There is one such place added in 8.2.2506, I think it should check for
the "skip" value. Let me give that a try.
If that fixes it I do wonder why it doesn't fail on other systems.
Perhaps because of how the union is laid out (overlapping different
bytes).


--
-rwxr-xr-x 1 root 24 Oct 29 1929 /bin/ed
-rwxr-xr-t 4 root 131720 Jan 1 1970 /usr/ucb/vi
-rwxr-xr-x 1 root 5.89824e37 Oct 22 1990 /usr/bin/emacs

[James McCoy]

On Sun, Feb 14, 2021 at 04:34:20PM +0100, Bram Moolenaar wrote:
> This is where it runs into the problem: the size should be zero.
> Since it's OK when the later statements are changed, I suspect that the
> "isn_arg.number" value is overwritten when generating one of the later
> statements. Since this is the first instruction I guess some index is
> zero when filling in a jump position later. Perhaps it only fails on
> s390 because it lays out the union of the struct differently.

s390 is big-endian. On little-endian systems, there was still a
problem. For the endtry that was supposed to be skipped, we're
modifying the previous instruction (newlist). It just happens to cause
a different, undetected failure.

Here's the disassembled code for little-endian:

Test_try_catch_throw
var l = []
0 NEWLIST size 34359738368

1 STORE $0

try
2 TRY catch -> 7, finally -> 3

finally
add(l, 0)
3 LOAD $0
4 PUSHNR 0
5 LISTAPPEND
6 DROP

endtry
7 ENDTRY

if 1
else
try
endtry
endif

8 RETURN 0

> There is one such place added in 8.2.2506, I think it should check for
> the "skip" value. Let me give that a try.
> If that fixes it I do wonder why it doesn't fail on other systems.
> Perhaps because of how the union is laid out (overlapping different
> bytes).

I was about to send a similar patch. That does fix the problem.

Cheers,