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

[Paul Berry]

On 31 August 2012 01:00, Kenneth Graunke <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>
> Signed-off-by: Kenneth Graunke <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))").


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 :)


> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> As I suspected: the precompile just exposed a bug that'd been lurking for
> a while (February 2012 to be precise).
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index a8d55ff..167ea08 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -979,7 +979,7 @@ fs_visitor::calculate_urb_setup()
>         *
>         * See compile_sf_prog() for more info.
>         */
> -      if (brw->fragment_program->Base.InputsRead &
> BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
> +      if (fp->Base.InputsRead & BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
>           urb_setup[FRAG_ATTRIB_PNTC] = urb_next++;
>     }
>
> --
> 1.7.11.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120831/439cdc40/attachment-0001.html>