Hi,
Junio C Hamano wrote:
> Andreas Schwab <schwab@xxxxxxxxxxxxxx> writes:
>
>> On Nov 29 2019, Todd Zullinger wrote:
>>
>>> When the 'grep with invalid UTF-8 data' tests were added/adjusted in
>>> 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26)
>>> and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,
>>> 2019-07-26) they lacked a redirect which caused them to falsely succeed
>>> on most architectures. They failed on big-endian arches where the test
>>> never reached the portion which was missing the redirect.
>>
>> It's not about big vs little endian, it's only about JIT vs non-JIT.
>
> So, which one of JIT / non-JIT sides did the test fail unexpectedly?
On s390x, the initial:
test_might_fail git grep -hi "Æ" invalid-0x80 >actual
fails to produce any output in actual, but since we use
test_might_fail, the test happily continues to:
test_cmp expected actual
which fails.
The test output from and s390x build:
expecting success of 7812.11 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i':
test_might_fail git grep -hi "Æ" invalid-0x80 >actual &&
test_cmp expected actual &&
test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 &&
test_cmp expected actual
++ test_might_fail git grep -hi Æ invalid-0x80
++ test_must_fail ok=success git grep -hi Æ invalid-0x80
++ case "$1" in
++ _test_ok=success
++ shift
++ git grep -hi Æ invalid-0x80
fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set
++ exit_code=128
++ test 128 -eq 0
++ test_match_signal 13 128
++ test 128 = 141
++ test 128 = 269
++ return 1
++ test 128 -gt 129
++ test 128 -eq 127
++ test 128 -eq 126
++ return 0
++ test_cmp expected actual
++ diff -u expected actual
--- expected 2019-10-19 21:56:08.634252012 +0000
+++ actual 2019-10-19 21:56:08.714252012 +0000
@@ -1 +0,0 @@
-ævar
error: last command exited with $?=1
not ok 11 - PCRE v2: grep non-ASCII from invalid UTF-8 data with -i
#
# test_might_fail git grep -hi "Æ" invalid-0x80 >actual &&
# test_cmp expected actual &&
# test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 &&
# test_cmp expected actual
#
# failed 1 among 11 test(s)
After Andreas' missing redirect fix, that still fails on
s390x (not surprisingly). But now systems with JIT enabled
fail at:
test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 >actual &&
test_cmp expected actual
Though we say that the command must fail, so we shouldn't be
surprised that 'expect' and 'actual' don't match. It would
be more surprising if they did. :)
> Should I do s/on big-endian arches/with PCRE with JIT disabled/
> while queuing the patch?
Here's how I changed the commit message locally. I was
going to wait a day or so for any other feedback on the
actual test changes, being a holiday weekend in the US (and
more generally a weekend).
1: d9aeaf0c98 ! 1: d0c083db78 t7812: expect failure for grep -i with invalid UTF-8 data
@@ Commit message
8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26)
and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,
2019-07-26) they lacked a redirect which caused them to falsely succeed
- on most architectures. They failed on big-endian arches where the test
- never reached the portion which was missing the redirect.
+ on most systems. The 'grep -i' test failed on systems where JIT was
+ disabled as it never reached the portion which was missing the redirect.
- A recent patch add the missing redirect and exposed the fact that the
- 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails on
- all architectures.
+ A recent patch added the missing redirect and exposed the fact that the
+ 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails
+ regardless of whether JIT is enabled.
Based on the final paragraph in in 870eea8166:
Thanks for pointing out the proper reasoning to use in the
commit message Andreas. I hadn't looked at the Fedora pcre2
package to see that it explicitly disables JIT on s390x.
I'm not sure if s390x is supported upstream or not -- it
doesn't appear to have a specific entry in the sljit config
header¹, so it seems likely it's not well-tested at the
least. (Not that any of that is our concern here.)
¹ https://github.com/zherczeg/sljit/blob/master/sljit_src/sljitConfigInternal.h
Thanks for the follow-up Junio.
--
Todd
