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

[Tobias Klausmann]

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 :/)

Greetings,
Tobias


>
>>         // check if we are moving an immediate, propagate it in that case
>>         if (!ld || ld->fixed || (ld->op != OP_LOAD && ld->op != OP_MOV) ||
>>               !(ld->src(0).getFile() == FILE_IMMEDIATE))
>>            bld.mkMovToReg(s, i->getSrc(s));
>>         else {
>> +         assert(ld->getSrc(0) != NULL);
>>            bld.mkMovToReg(s, ld->getSrc(0));
>>            // Clear the src, to make code elimination possible here before we
>>            // delete the instruction i later
>> -- 
>> 2.21.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev