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

[Anuj Phogat]

On Wed, Feb 7, 2018 at 11:22 AM, 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 
> > > ---
> > >  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.
>
​Right. Dropping this patch.​



>
> > 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.
>
> > > ds.DispatchMode = DISPATCH_MODE_SIMD8_SINGLE_PATCH;
> > >  ds.UserClipDistanceCullTestEnableBitmask =
> > >  vue_prog_data->cull_distance_mask;
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
> > > ---
> > >  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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Kenneth Graunke]

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 
> > ---
> >  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.

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



signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Nanley Chery]

On Mon, Feb 05, 2018 at 04:08:40PM -0800, Anuj Phogat wrote:
> Signed-off-by: Anuj Phogat 
> ---
>  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.

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.

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

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

[Anuj Phogat]

Signed-off-by: Anuj Phogat 
---
 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)
ds.DispatchMode = DISPATCH_MODE_SIMD8_SINGLE_PATCH;
 ds.UserClipDistanceCullTestEnableBitmask =
 vue_prog_data->cull_distance_mask;
-- 
2.13.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev