[Mesa-dev] [PATCH 1/1] glsl/blob: handle copy of NULL ptr in blob_write_string

[gregory hainaut]

On Fri, 31 Mar 2017 19:16:10 +1100
Timothy Arceri <tarceri at itsqueeze.com> wrote:

> 
> 
> On 31/03/17 18:00, gregory hainaut wrote:
> > On Fri, 31 Mar 2017 08:24:36 +0200
> > Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> >
> > Hello Nicolai
> >
> >> On 30.03.2017 21:55, Gregory Hainaut wrote:
> >>> Typically happen when we want to copy an unnamed shader parameter
> >>> in the shader cache.
> >>
> >> So this happens only when blob_write_string is called from nouveau?
> >
> > Sorry, I poorly explain myself. I should have written reproduce &
> > tested on Nouveau. I don't know for others drivers, they should be
> > impacted.
> >
> > _mesa_add_parameter seems to allow to store a NULL pointer in p->Name.
> > Which is later written by blob_write_string. I guess it could
> > depends on the shader cache state.
> >
> >
> > I got the crash with this piglit test:
> > textureGather fs offsets r 0 float 2D repeat -auto -fb
> 
> Others have reported this crashing on Nouveau. I haven't seen the 
> problem on radeonsi or i965.
> 
> >
> >
> >> By the way, please setup send-mail so that it threads your mails.
> >> That should be the default, so I'm not sure what happened here...
> > Oh. I edited my email in the mailer queue which moved the email from my
> > pop3 to my imap account. I guess it broke the threading link. I will
> > be more careful next time.
> >
> > Thanks
> >
> >
> >> Thanks,
> >> Nicolai
> >
> >>>
> >>> Note: it is safer to copy an empty string so we can read it back
> >>> safely.
> >>>
> >>> Fix piglit crashes of the 'texturegatheroffsets' tests
> >>>
> >>> Signed-off-by: Gregory Hainaut <gregory.hainaut at gmail.com>
> >>> ---
> >>>  src/compiler/glsl/blob.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c
> >>> index 769ebf1..f84d7f3 100644
> >>> --- a/src/compiler/glsl/blob.c
> >>> +++ b/src/compiler/glsl/blob.c
> >>> @@ -176,7 +176,10 @@ blob_write_intptr(struct blob *blob, intptr_t
> >>> value) bool
> >>>  blob_write_string(struct blob *blob, const char *str)
> >>>  {
> >>> -   return blob_write_bytes(blob, str, strlen(str) + 1);
> >>> +   if (str == NULL)
> >>> +      return blob_write_bytes(blob, "", 1);
> >>> +   else
> >>> +      return blob_write_bytes(blob, str, strlen(str) + 1);
> >>>  }
> >>>
> >>>  void
> >>>
> >>
> >>

Fwiw, I backtraced the origin of the NULL. As you can see _mesa_add_typed_unnamed_constant will set the name string to NULL instead of "".
So it seems Intel/AMD don't use unnamed constant when the shader is linked.

#0  _mesa_add_parameter (paramList=0x8126628, type=PROGRAM_CONSTANT, name=0x0, size=2, datatype=5124, values=0xffffc4d0, state=0x0) at mesa/program/prog_parameter.c:256
#1  0xf6d6224d in _mesa_add_typed_unnamed_constant (paramList=0x8126628, values=0xffffc4d0, size=2, datatype=5124, swizzleOut=0xffffc468) at mesa/program/prog_parameter.c:345
#2  0xf6d1d9ee in glsl_to_tgsi_visitor::add_constant (this=0x839b838, file=PROGRAM_CONSTANT, values=0xffffc4d0, size=2, datatype=5124, swizzle_out=0x83a0a98)
    at state_tracker/st_glsl_to_tgsi.cpp:1126
#3  0xf6d296af in glsl_to_tgsi_visitor::visit (this=0x839b838, ir=0x8395b60) at state_tracker/st_glsl_to_tgsi.cpp:3410
#4  0xf6e184c7 in ir_constant::accept (this=0x8395b60, v=0x839b838) at glsl/ir.h:2133
#5  0xf6d28b5d in glsl_to_tgsi_visitor::visit (this=0x839b838, ir=0x8395a98) at state_tracker/st_glsl_to_tgsi.cpp:3278


So we can do 3 different fixes (and potentially the 3).
1/ update _mesa_add_typed_unnamed_constant to use an empty string
2/ update _mesa_add_parameter "p->Name = name ? strdup(name) : NULL;" to use an empty string when name is null
3/ or my previous patch :)

Cheers,
Gregory