[Mesa-dev] [PATCH] i965: Do null pointer check before dereferencing vue_prog_data

[Nanley Chery]

On Wed, Feb 07, 2018 at 11:22:01AM -0800, Kenneth Graunke wrote:
> On Tuesday, February 6, 2018 4:38:00 PM PST Nanley Chery wrote:
> > On Mon, Feb 05, 2018 at 04:08:40PM -0800, Anuj Phogat wrote:
> > > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/genX_state_upload.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > > index aa4d64d08e..67fb328dbc 100644
> > > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> > > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > > @@ -3966,7 +3966,8 @@ genX(upload_ds_state)(struct brw_context *brw)
> > >             tes_prog_data->domain == BRW_TESS_DOMAIN_TRI;
> > >  
> > >  #if GEN_GEN >= 8
> > > -        if (vue_prog_data->dispatch_mode == DISPATCH_MODE_SIMD8)
> > > +        if (vue_prog_data &&
> > > +            vue_prog_data->dispatch_mode == DISPATCH_MODE_SIMD8)
> > 
> > In this else case (where tes_prog_data != NULL) this variable is also
> > guaranteed to be non-NULL. They point to the same location in memory as
> > far as I can tell.
> 
> NAK on this patch.  The if (!tes_prog_data) {...} else { surrounding
> this code guarantees that tes_prog_data != NULL, which also means that
> vue_prog_data != NULL.
> 
> > Though, what I find confusing is that we can simultaneously access
> > fields of struct brw_tes_prog_data as well as struct brw_vue_prog_data
> > even though neither is a subclass of the other.
> 
> The class hierarchy is:
> 
>   brw_stage_prog_data -> brw_vue_prog_data -> brw_vs_prog_data
>                                            -> brw_tcs_prog_data
>                                            -> brw_tes_prog_data
>                                            -> brw_gs_prog_data
>                       -> brw_wm_prog_data
>                       -> brw_cs_prog_data
> 
> This is done in the traditional C style of embedding the base object
> as the first member in the struct.  So, you can safely cast between
> brw_stage_prog_data, brw_vue_prog_data, and brw_tes_prog_data for a
> tessellation evaluation shader.
> 

Thanks for chiming in on this, Ken! I must've misread the struct
definition the first time around.

> > >             ds.DispatchMode = DISPATCH_MODE_SIMD8_SINGLE_PATCH;
> > >          ds.UserClipDistanceCullTestEnableBitmask =
> > >              vue_prog_data->cull_distance_mask;
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
>