[Mesa-dev] [PATCH] i965/fs: Don't use brw->fragment_program in calculate_urb_setup().

[Kenneth Graunke]

On 08/31/2012 12:24 PM, Paul Berry wrote:
> On 31 August 2012 01:00, Kenneth Graunke <kenneth at whitecape.org
> <mailto:kenneth at whitecape.org>> wrote:
> 
>     Reading brw->fragment_program is nonsensical in compiler code: it
>     contains the currently active program (if any), not the one currently
>     being compiled.  Attempting to access it may either lead to crashes
>     (null pointer dereference if no program is active) or wrong results.
> 
>     Fixes piglit regressions since 9ef710575b914ddfc8e9a162d98ad554c1c217f7
>     on pre-Sandybridge hardware.  The actual bug was created in commit
>     7b1fbc688999fd568e65211d79d7678562061594.
> 
>     NOTE: This is a candidate for the 9.0 and 8.0 branches.
> 
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54183
>     Cc: Rico Tzschichholz <ricotz at t-online.de <mailto:ricotz at t-online.de>>
>     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at whitecape.org>>
> 
> 
> Good catch!
> 
> It looks like a similar mistake exists in brw_wm_pass2.c's
> init_registers() function ("if (brw->fragment_program->Base.InputsRead &
> BITFIELD64_BIT(j))").

Nice catch.  I just sent out a patch for that case, and in the meantime,
pushed the original one with your Reviewed-by since this was breaking
Ironlake something awful and I didn't want to wait.

> Incidentally, this gives me an idea for a structural change to the
> compiler back-end that would prevent bugs like this from happening in
> the future.  Rather than passing brw around inside the compiler
> functions, let's make a new data structure that contains just those
> parts of brw that make sense for the compiler to access (things like
> brw->intel.gen, brw->needs_unlit_centroid_workaround, brw->has_pln,
> etc.).  And pass around that data structure inside the compiler
> functions instead of brw.  In addition to preventing bugs like this one,
> it would prevent bugs where we forget to include important state in the
> program key, since the compiler functions wouldn't be able to just
> consult the state directly anymore.  I think I might experiment with
> that as a back-burner project in my copious free time :)

Yeah, that definitely seems like a good idea.  It's really easy to
access the wrong state, and making it inaccessible without a ton of
plumbing is a sure-fire way to make sure people don't accidentally do
it.  I don't know how feasible it would be to split it, but it's worth
looking into someday.