Bug 1572540

Summary: wrong results from VMX code in pixman
Product: [Fedora] Fedora Reporter: Dan Horák <dan>
Component: pixmanAssignee: Adam Jackson <ajax>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: ajax, alexl, aoliva, caillon+fedoraproject, davejohansen, dmalcolm, fweimer, glaubitz, jakub, john.j5live, jwakely, law, mpolacek, msebor, nickc, oded.gabbay, rhughes, rstrode, wschmidt
Target Milestone: ---   
Target Release: ---   
Hardware: ppc64le   
OS: Unspecified   
Whiteboard:
Fixed In Version: pixman-0.34.0-8.fc28 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-05-11 01:24:24 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1071880    

Description Dan Horák 2018-04-27 09:40:43 UTC
Description of problem:
The VMX code in the pixman library produces wrong results when compiled with gcc8 and is causing issues described in bug #1546693. It's also visible in failing tests in the pixman build. Dunno if it makes sense, but -O0 + VMX enabled make the tests pass. The code in question is https://cgit.freedesktop.org/pixman/tree/pixman/pixman-vmx.c which hasn't change since autumn 2015.

There is no such problem in gcc7 (gcc-7.3.1-5.fc27.ppc64le).


Version-Release number of selected component (if applicable):
gcc-8.0.1-0.20.fc28.ppc64le
gcc-8.0.1-0.21.fc29.ppc64le


How reproducible:
100%


Steps to Reproduce:
1. fedpkg co pixman
2. fedpkg build (remove --disable-vmx from spec)
3.

Actual results:
9 tests failed

Expected results:
0 tests failed

Additional info:

Comment 1 Dan Horák 2018-04-27 09:45:24 UTC
here is the failing tests list

...
Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.lhFV49
+ umask 022
+ cd /home/sharkcz/pixman
+ cd pixman-0.34.0
+ make check -j5 V=1
Making check in pixman
make[1]: Entering directory '/home/sharkcz/pixman/pixman-0.34.0/pixman'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/home/sharkcz/pixman/pixman-0.34.0/pixman'
Making check in demos
make[1]: Entering directory '/home/sharkcz/pixman/pixman-0.34.0/demos'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/home/sharkcz/pixman/pixman-0.34.0/demos'
Making check in test
make[1]: Entering directory '/home/sharkcz/pixman/pixman-0.34.0/test'
make  check-TESTS
make[2]: Entering directory '/home/sharkcz/pixman/pixman-0.34.0/test'
make[3]: Entering directory '/home/sharkcz/pixman/pixman-0.34.0/test'
PASS: trap-crasher
PASS: infinite-loop
PASS: region-translate-test
PASS: oob-test
PASS: fence-image-self-test
PASS: fetch-test
PASS: a1-trap-test
PASS: prng-test
PASS: radial-invalid
PASS: pdf-op-test
PASS: combiner-test
PASS: region-test
PASS: scaling-crash-test
PASS: alpha-loop
FAIL: alphamap
PASS: scaling-helpers-test
FAIL: thread-test
PASS: rotate-test
PASS: pixel-test
PASS: matrix-test
FAIL: composite-traps-test
PASS: gradient-crash-test
SKIP: cover-test
PASS: region-contains-test
FAIL: glyph-test
FAIL: solid-test
FAIL: blitters-test
FAIL: affine-test
FAIL: scaling-test
PASS: stress-test
PASS: composite
FAIL: tolerance-test
============================================================================
Testsuite summary for pixman 0.34.0
============================================================================
# TOTAL: 32
# PASS:  22
# SKIP:  1
# XFAIL: 0
# FAIL:  9
# XPASS: 0
# ERROR: 0
============================================================================
See test/test-suite.log
Please report to pixman.org
============================================================================

Comment 2 Jakub Jelinek 2018-04-27 09:47:40 UTC
Have you tried -fsanitize=undefined ?

Comment 3 Dan Horák 2018-04-27 10:01:34 UTC
with -fsanitize=undefined alphamap.log contains now and other logs contain even more stuff like "left shift of negative value -1" or "signed integer overflow: 65536 * -47518 cannot be represented in type 'int'"

---
pixman-vmx.c:288:5: runtime error: load of misaligned address 0x7fffd9c66804 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 00 00 ff  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of address 0x7fffd9c66804 with insufficient space for an object of type 'uint32_t'
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 00 00 ff  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of misaligned address 0x7fffd9c66804 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 00 f8 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of address 0x7fffd9c66804 with insufficient space for an object of type 'uint32_t'
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 00 f8 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of misaligned address 0x7fffd9c66804 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 fc 00 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of address 0x7fffd9c66804 with insufficient space for an object of type 'uint32_t'
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 fc 00 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of misaligned address 0x7fffd9c66804 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 f8 00 00 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of address 0x7fffd9c66804 with insufficient space for an object of type 'uint32_t'
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 f8 00 00 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of misaligned address 0x7fffd9c66804 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 e0 00 e0 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of address 0x7fffd9c66804 with insufficient space for an object of type 'uint32_t'
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 e0 00 e0 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of misaligned address 0x7fffd9c66804 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 c0 00 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:288:5: runtime error: load of address 0x7fffd9c66804 with insufficient space for an object of type 'uint32_t'
0x7fffd9c66804: note: pointer points here
  30 68 c6 d9 00 c0 00 00  00 d6 07 d6 18 ff d5 1c  28 6b c6 d9 ff 7f 00 00  98 68 c6 d9 ff 7f 00 00
              ^ 
pixman-vmx.c:1593:2: runtime error: load of misaligned address 0x7fffab1c07a8 for type '__vector unsigned int', which requires 16 byte alignment
0x7fffab1c07a8: note: pointer points here
 2f 5a b8 a7  23 0f 83 e8 6b c9 99 99  85 f3 3f a0 76 8e 4d 1c  9c 5a ce df 4f c7 0c 86  44 44 48 e2
              ^ 

Wrong alpha value at (10, 10). Should be 0xff; got 0xc9. Source was 0xe8, original dest was 0xa1
src: a8r8g8b8, alpha: null, origin 0 0
dst: a8r8g8b8, alpha: a8, origin: 10 10

FAIL alphamap (exit status: 1)
---

Comment 4 Jakub Jelinek 2018-04-27 10:05:11 UTC
So package bug then.
# define LOAD_VECTOR(source)				\
    v ## source = *((typeof(v ## source)*)source);
is invalid.
See #1556989 for similar bug in gromacs.

Comment 5 Dan Horák 2018-04-27 10:07:08 UTC
Although same messages are present also in the logs for passed tests ...

Comment 6 Dan Horák 2018-04-27 10:07:40 UTC
(In reply to Jakub Jelinek from comment #4)
> So package bug then.
> # define LOAD_VECTOR(source)				\
>     v ## source = *((typeof(v ## source)*)source);
> is invalid.
> See #1556989 for similar bug in gromacs.

OK, thanks for looking.

Comment 7 Dan Horák 2018-04-27 10:33:37 UTC
Thanks for hint, after applying

--- pixman-vmx.c.orig	2018-04-27 12:10:14.411696502 +0200
+++ pixman-vmx.c	2018-04-27 12:27:41.174290224 +0200
@@ -227,7 +227,7 @@
 #define COMPUTE_SHIFT_MASKC(dest, source, mask)
 
 # define LOAD_VECTOR(source)				\
-    v ## source = *((typeof(v ## source)*)source);
+    v ## source = (typeof(v ## source))vec_xl(0, source);
 
 # define LOAD_VECTORS(dest, source)			\
     LOAD_VECTOR(source);				\

(maybe s/vec_ld/vec_xl/ is needed for the big endian variant)

the misaligned load errors went away and 4 more tests are passing, remaining failures are

FAIL: glyph-test
FAIL: solid-test
FAIL: blitters-test
FAIL: affine-test
FAIL: scaling-test

Comment 8 Jakub Jelinek 2018-04-27 10:39:38 UTC
I think it would be desirable to fix all the ubsan errors rather than just a subset of them before looking further, signed integer overflow, or shifts by negative count etc. can also practically result in very weird behavior when optimizing.

Comment 9 Dan Horák 2018-04-27 10:54:22 UTC
Definitely, this was just a poor-man's attempt to fix something :-)

Comment 10 Dan Horák 2018-04-27 15:11:33 UTC
For the record some of the "undefined behaviours" are common to other arches as well (I've checked on x86_64).

Comment 11 Dan Horák 2018-04-27 16:28:45 UTC
After a chat with Jakub it shows that the 5 failures go away if I remove the -fsanitize option from CFLAGS, so we have a solution with VMX enabled, although not perfect.

Bill, is it safe to use vec_ld() in the big endian variant of the LOAD_VECTOR() macro at https://cgit.freedesktop.org/pixman/tree/pixman/pixman-vmx.c#n189 or should it also be switched vec_xl()?

Comment 12 Bill Schmidt 2018-04-27 20:28:19 UTC
Yes, it looks fine to me as is.

Comment 13 Fedora Update System 2018-05-07 07:44:38 UTC
pixman-0.34.0-8.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-a72105b2c7

Comment 14 Fedora Update System 2018-05-07 16:02:41 UTC
pixman-0.34.0-8.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-a72105b2c7

Comment 15 Fedora Update System 2018-05-11 01:24:24 UTC
pixman-0.34.0-8.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 John Paul Adrian Glaubitz 2021-03-31 14:44:42 UTC
This change broke 64-bit PowerPC without VMX:

```/<<PKGBUILDDIR>>/gfx/cairo/libpixman/src/pixman-vmx.c: In function ‘load_128_unaligned’:
/<<PKGBUILDDIR>>/gfx/cairo/libpixman/src/pixman-vmx.c:230:40: error: implicit declaration of function ‘vec_xl’; did you mean ‘vec_rl’? [-Werror=implicit-function-declaration]
  230 |     v ## source = (typeof(v ## source))vec_xl(0, source);
      |                                        ^~~~~~```

Comment 17 John Paul Adrian Glaubitz 2021-03-31 15:05:35 UTC
(In reply to John Paul Adrian Glaubitz from comment #16)
> This change broke 64-bit PowerPC without VMX:
> 
> ```/<<PKGBUILDDIR>>/gfx/cairo/libpixman/src/pixman-vmx.c: In function
> ‘load_128_unaligned’:
> /<<PKGBUILDDIR>>/gfx/cairo/libpixman/src/pixman-vmx.c:230:40: error:
> implicit declaration of function ‘vec_xl’; did you mean ‘vec_rl’?
> [-Werror=implicit-function-declaration]
>   230 |     v ## source = (typeof(v ## source))vec_xl(0, source);
>       |                                        ^~~~~~```

Hmm, it seems this affects the pixman copy only shipped in Firefox. Never mind.

Comment 18 Dan Horák 2021-03-31 15:07:55 UTC
Also, if VMX is not available, then pixmap-vmx.c shouldn't be compiled at all, thus the detection in configure would be wrong. But FF has it's own build system, so the previous won't apply.