[Mesa-dev] [PATCH] nvc0/ir: Fix assert accessing null pointer

[Eric Engestrom]

On Friday, 2019-07-19 21:23:24 +0200, Tobias Klausmann wrote:
> 
> Am 19.07.19 um 18:18 schrieb Ilia Mirkin:
> > On Fri, Jul 19, 2019 at 12:07 PM Tobias Klausmann
> > <tobias.johannes.klausmann at mni.thm.de> wrote:
> > > On 19.07.19 15:39, Eric Engestrom wrote:
> > > > On Friday, 2019-07-19 13:56:30 +0200, Mark Menzynski wrote:
> > > > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=111007
> > > > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=111167
> > > > `Fixes:` is used to indicate the commit that introduced the code being
> > > > fixed, such as:
> > > >     Fixes: 1c4e6d7ca83578caf521 ("nvc0/ir: propagate immediates to CALL input MOVs")
> > > > This tag is used by our tools to backport fixes to the relevant stable
> > > > releases.
> > > > 
> > > > Bugzilla entries are referenced using the `Bugzilla:` prefix.
> > > > 
> > > > > Signed-off-by: Mark Menzynski <mmenzyns at redhat.com>
> > > > > ---
> > > > >    src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> > > > > index aca3b0afb1e..1f702a987d8 100644
> > > > > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> > > > > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> > > > > @@ -51,12 +51,12 @@ NVC0LegalizeSSA::handleDIV(Instruction *i)
> > > > >       // Generate movs to the input regs for the call we want to generate
> > > > >       for (int s = 0; i->srcExists(s); ++s) {
> > > > >          Instruction *ld = i->getSrc(s)->getInsn();
> > > > > -      assert(ld->getSrc(0) != NULL);
> > > > I'll admit I don't know anything about this code, but it looks like
> > > > this might be a better fix?
> > > >      assert(ld == NULL || ld->getSrc(0) != NULL)
> > > > 
> > > > I cc'ed Tobias who wrote this code as he'll probably know best.
> > > Thanks for letting me know, but i'm kind of out of the loop and sadly
> > > don't remember too much about this one.
> > > 
> > > Yet while having a look at the code i somehow wonder if there is a
> > > possibility to have
> > > 
> > >          if (!ld || ld->fixed || (ld->op != OP_LOAD && ld->op != OP_MOV) ||
> > >                !(ld->src(0).getFile() == FILE_IMMEDIATE))
> > > 
> > > crash at ld->src(0), as this is only set when a value is added to the instruction. So in the case ld->src(0) is legal, ld->getSrc(0) should be as well and we would need the assert at the beginning...
> > > 
> > > PS: Did a functional change bring this to the light? (I ran piglit in front of every patch sumbission :/)
> > Nope. Just weird shaders that divide and/or mod 0 by a non-constant amount.
> > 
> > There's an argument to just remove that assert entirely - not sure
> > that it's adding a whole lot, except complication.
> 
> Ok,
> 
> in this case i'm happy with the patch, with the Bugzilla References and the
> proper Fixes tag added this is:
> 
> Reviewed-by: Tobias Klausmann<tobias.klausmann at freenet.de>
> (yes this mail address is different, but my uni mail address won't exist for much longer)

You might want to add a line to the .mailmap then ;)