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

[Nicolai Hähnle]

On 31.03.2017 21:05, gregory hainaut wrote:

On Fri, 31 Mar 2017 12:53:47 -0400
Ilia Mirkin  wrote:


On Fri, Mar 31, 2017 at 6:12 AM, Gregory Hainaut
 wrote:

Others have reported this crashing on Nouveau. I haven't seen the problem on 
radeonsi or i965.


Hello Timothy (sorry for the double mail, email is a complex tool:) )

Hum, tbh. I was quite surprised to hit this bug. I guess you save a
pre-optimized shader in the cache. So it could depends on optimization
passes.

From the top of my head, I think the "offending" line is this one
const ivec2 offsets[4] = {ivec2(...), ivec2(...), ivec2(...), ivec2(...)};

Strangely enough there are only 3 parameters without name in the
parameter list (signature is int, size 2 and CONTANT). Maybe one was
optimized away, I didn't look further.


Note that nouveau is unique in that it can process
textureGatherOffsets() directly, without lowering it to 4x
textureGatherOffset.

The relevant code is in st_glsl_to_tgsi.cpp

  if (!pscreen->get_param(pscreen, PIPE_CAP_TEXTURE_GATHER_OFFSETS))
 lower_offset_arrays(ir);

So I think with nouveau, you're seeing glsl ir that you wouldn't see otherwise.

  -ilia


Hello ilia

You're right. The issue appears in the texture gather 4 opcode.

I can see this path (st_glsl_to_tgsi.cpp) in GDB.
case ir_tg4:
   opcode = TGSI_OPCODE_TG4;


Thanks for the explanation!

So this definitely needs to be solved in the cache, but my concern with 
the current patch is that there might be code that behaves differently 
when the name is NULL vs. when the name is "". So I'd prefer if instead 
the caller of blob_write_string were changed accordingly (first write a 
flag of whether there is a string or not, etc.). Or maybe add a helper 
function blob_write_optional_string which does that.


Thanks,
Nicolai





Cheers,
Gregory




--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[gregory hainaut]

On Fri, 31 Mar 2017 12:53:47 -0400
Ilia Mirkin  wrote:

> On Fri, Mar 31, 2017 at 6:12 AM, Gregory Hainaut
>  wrote:
> >> Others have reported this crashing on Nouveau. I haven't seen the problem 
> >> on radeonsi or i965.
> >
> > Hello Timothy (sorry for the double mail, email is a complex tool:) )
> >
> > Hum, tbh. I was quite surprised to hit this bug. I guess you save a
> > pre-optimized shader in the cache. So it could depends on optimization
> > passes.
> >
> > From the top of my head, I think the "offending" line is this one
> > const ivec2 offsets[4] = {ivec2(...), ivec2(...), ivec2(...), ivec2(...)};
> >
> > Strangely enough there are only 3 parameters without name in the
> > parameter list (signature is int, size 2 and CONTANT). Maybe one was
> > optimized away, I didn't look further.
> 
> Note that nouveau is unique in that it can process
> textureGatherOffsets() directly, without lowering it to 4x
> textureGatherOffset.
> 
> The relevant code is in st_glsl_to_tgsi.cpp
> 
>   if (!pscreen->get_param(pscreen, PIPE_CAP_TEXTURE_GATHER_OFFSETS))
>  lower_offset_arrays(ir);
> 
> So I think with nouveau, you're seeing glsl ir that you wouldn't see 
> otherwise.
> 
>   -ilia

Hello ilia

You're right. The issue appears in the texture gather 4 opcode.

I can see this path (st_glsl_to_tgsi.cpp) in GDB.
case ir_tg4:
   opcode = TGSI_OPCODE_TG4;


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

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

[Ilia Mirkin]

On Fri, Mar 31, 2017 at 6:12 AM, Gregory Hainaut
 wrote:
>> Others have reported this crashing on Nouveau. I haven't seen the problem on 
>> radeonsi or i965.
>
> Hello Timothy (sorry for the double mail, email is a complex tool:) )
>
> Hum, tbh. I was quite surprised to hit this bug. I guess you save a
> pre-optimized shader in the cache. So it could depends on optimization
> passes.
>
> From the top of my head, I think the "offending" line is this one
> const ivec2 offsets[4] = {ivec2(...), ivec2(...), ivec2(...), ivec2(...)};
>
> Strangely enough there are only 3 parameters without name in the
> parameter list (signature is int, size 2 and CONTANT). Maybe one was
> optimized away, I didn't look further.

Note that nouveau is unique in that it can process
textureGatherOffsets() directly, without lowering it to 4x
textureGatherOffset.

The relevant code is in st_glsl_to_tgsi.cpp

  if (!pscreen->get_param(pscreen, PIPE_CAP_TEXTURE_GATHER_OFFSETS))
 lower_offset_arrays(ir);

So I think with nouveau, you're seeing glsl ir that you wouldn't see otherwise.

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