Bug 107012 - [PATCH] Radeon SI driver not architecture safe, crashes on ppc64[el]
Summary: [PATCH] Radeon SI driver not architecture safe, crashes on ppc64[el]
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/radeonsi (show other bugs)
Version: git
Hardware: PowerPC All
: medium major
Assignee: Default DRI bug account
QA Contact: Default DRI bug account
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-23 21:34 UTC by Timothy Pearson
Modified: 2018-11-06 10:49 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix crashes (1.17 KB, patch)
2018-06-23 21:34 UTC, Timothy Pearson
Details | Splinter Review
Fix crashes (1.17 KB, patch)
2018-06-24 00:41 UTC, Timothy Pearson
Details | Splinter Review

Description Timothy Pearson 2018-06-23 21:34:48 UTC
Created attachment 140294 [details] [review]
Fix crashes

color_interp_vgpr_index was declared as a generic char value.  Because signed values are used in this variable, the result was not safe across architectures and crashed on ppc64[el] and arm systems.

The atttached patch declares color_interp_vgpr_index as a signed type, and has been verified to work with alien-arena, glxgears, and Flightgear, all of which were crashing before the patch.
Comment 1 Simon McVittie 2018-06-23 23:02:25 UTC
Comment on attachment 140294 [details] [review]
Fix crashes

Review of attachment 140294 [details] [review]:
-----------------------------------------------------------------

::: src/gallium/drivers/radeonsi/si_shader.h
@@ +507,4 @@
>  		unsigned	ancillary_vgpr_index:5;
>  		unsigned	wqm:1;
>  		char		color_attr_index[2];
> +		signed		color_interp_vgpr_index[2]; /* -1 == constant */

Shouldn't this be a signed char, to make the size of the struct the same on all architectures?
Comment 2 Simon McVittie 2018-06-23 23:05:22 UTC
(In reply to Simon McVittie from comment #1)
> Shouldn't this be a signed char, to make the size of the struct the same on
> all architectures?

I should have said: to make the size of this field the same as it used to be, and make the effective type on all architectures the same as it always was on x86. (char with no signedness qualifier is the same as signed char on most architectures, but is the same as unsigned char on at least the powerpc family.)
Comment 3 Timothy Pearson 2018-06-23 23:41:24 UTC
Comment on attachment 140294 [details] [review]
Fix crashes

Review of attachment 140294 [details] [review]:
-----------------------------------------------------------------

I wasn't sure how you guys wanted it done; I can resubmit as signed char if desired no problem. :-)
Comment 4 Timothy Pearson 2018-06-24 00:41:57 UTC
Created attachment 140295 [details] [review]
Fix crashes

Re-upload with signed char used.  Not yet tested.
Comment 5 Timothy Pearson 2018-06-24 02:26:07 UTC
Confirm that latest patch works on a Radeon RX 470.
Comment 6 Michel Dänzer 2018-06-25 09:33:44 UTC
Please send patches like this directly to the mesa-dev mailing list for review.
Comment 7 Timothy Pearson 2018-07-16 19:22:15 UTC
(In reply to Michel Dänzer from comment #6)
> Please send patches like this directly to the mesa-dev mailing list for
> review.

Patch has been sent.  I don't monitor mesa-dev, so please address any concerns directly to the Email address the patch came from.

Thanks!
Comment 8 Emil Velikov 2018-11-06 10:49:16 UTC
The fix was merged as e1621fda84d92f154b19c5bf9562ee6963414399


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.