On 08/31/2012 12:24 PM, Paul Berry wrote: > On 31 August 2012 01:00, Kenneth Graunke <kenn...@whitecape.org > <mailto:kenn...@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 <ric...@t-online.de <mailto:ric...@t-online.de>> > Signed-off-by: Kenneth Graunke <kenn...@whitecape.org > <mailto:kenn...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
