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

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