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

[Tobias Klausmann]

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)

Thanks,
Tobias