Last Comment Bug 519338 - Purify UMR in QCMS
: Purify UMR in QCMS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: GFX: Color Management (show other bugs)
: 1.9.2 Branch
: x86 Windows Server 2003
: -- normal (vote)
: ---
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-28 16:32 PDT by Robert Sayre
Modified: 2015-10-16 11:51 PDT (History)
5 users (show)
mbeltzner: blocking1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta3-fixed
.6-fixed


Attachments
purify stack (1.76 KB, text/plain)
2009-09-28 16:33 PDT, Robert Sayre
no flags Details
Initialize the unused element of the input array (499 bytes, patch)
2009-09-28 20:37 PDT, Jeff Muizelaar [:jrmuizel]
joe: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Description Robert Sayre 2009-09-28 16:32:41 PDT
will attach stack
Comment 1 Robert Sayre 2009-09-28 16:33:09 PDT
Created attachment 403368 [details]
purify stack
Comment 2 Jeff Muizelaar [:jrmuizel] 2009-09-28 20:37:29 PDT
Created attachment 403412 [details] [diff] [review]
Initialize the unused element of the input array

This UMR is mostly harmless because we never use the result. We just load the uninitialized value, take it along for the computation and discard it. This code has also been rewritten in bug 512865. Still, it doesn't hurt to initialize it so here's a patch.
Comment 3 Joe Drew (not getting mail) 2009-09-29 08:30:18 PDT
Comment on attachment 403412 [details] [diff] [review]
Initialize the unused element of the input array

>diff --git a/gfx/qcms/transform.c b/gfx/qcms/transform.c
>--- a/gfx/qcms/transform.c
>+++ b/gfx/qcms/transform.c
>@@ -812,6 +812,8 @@ static void qcms_transform_data_rgb_out_
>         /* share input and output locations to save having to keep the
>          * locations in separate registers */
>         uint32_t* output = (uint32_t*)input;
>+
>+        input[3] = 0; /* initialize the unused 4th element of the input array */

Not the greatest place to put this, but C doesn't give us a lot of options.
Comment 4 Jeff Muizelaar [:jrmuizel] 2009-09-29 08:44:02 PDT
(In reply to comment #3)
> (From update of attachment 403412 [details] [diff] [review])
> >diff --git a/gfx/qcms/transform.c b/gfx/qcms/transform.c
> >         uint32_t* output = (uint32_t*)input;
> >+
> >+        input[3] = 0; /* initialize the unused 4th element of the input array */
> 
> Not the greatest place to put this, but C doesn't give us a lot of options.

Why not?
Comment 5 Joe Drew (not getting mail) 2009-09-29 08:51:36 PDT
It should either go with the input stuff above output, or in the body of the loop (one is impossible, the other sub-optimal performance-wise).
Comment 6 Daniel Veditz [:dveditz] 2009-10-21 16:27:45 PDT
Comment on attachment 403412 [details] [diff] [review]
Initialize the unused element of the input array

Approved for 1.9.1.5, a=dveditz for release-drivers

Is there a testcase that exercises this code? Or does it happen with any graphics?

In addition to Purify, a windows debug build with the -RTCu or -RTC1 switch should also catch UMRs like this -- if a testcase causes the code to be executed.
Comment 7 Joe Drew (not getting mail) 2009-11-09 13:32:58 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8415bc81a9d3
Comment 8 Samuel Sidler (old account; do not CC) 2009-11-11 23:34:47 PST
This should block 1.9.2 since we took the patch on 1.9.1 and it'd otherwise be a regression when 1.9.2 ships.
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-13 06:50:02 PST
Agreed; you can clear the approval request and land.
Comment 10 Jeff Muizelaar [:jrmuizel] 2009-11-13 12:53:07 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0417352b24d6
Comment 11 Samuel Sidler (old account; do not CC) 2009-11-13 15:52:55 PST
Does this need to land on mozilla-central?
Comment 12 Jeff Muizelaar [:jrmuizel] 2009-11-14 05:27:45 PST
No. Mozilla-central doesn't have this bug because the relevant code has been rewritten.

Note You need to log in before you can comment on or make changes to this bug.