[PATCH] avoid -Wnonull for dynamic_cast (PR 99251)

[Jason Merrill]

On 3/1/21 7:44 PM, Martin Sebor wrote:
> On 3/1/21 1:33 PM, Jason Merrill wrote:
>> On 3/1/21 12:10 PM, Martin Sebor wrote:
>>> On 2/24/21 8:13 PM, Jason Merrill wrote:
>>>> On 2/24/21 5:25 PM, Martin Sebor wrote:
>>>>> In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided
>>>>> that issuing -Wnonnull for dereferencing the result of dynamic_cast
>>>>> was helpful despite the false positives it causes when the pointer
>>>>> is guaranteed not to be null because of a prior test.
>>>>>
>>>>> The test case in PR 99251 along with the feedback I got from Martin
>>>>> Liska have convinced me it was not the right decision.
>>>>>
>>>>> The attached patch arranges for dynamic_cast to also suppress 
>>>>> -Wnonnull
>>>>> analogously to static_cast.  Since there already is a helper function
>>>>> that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING,
>>>>> I factored out the corresponding code from build_base_path that sets
>>>>> the additional TREE_NO_WARNING bit for static_cast into the function
>>>>> and called it from both places.  I also renamed the function to make
>>>>> its purpose clearer and for consistency with other build_xxx APIs.
>>>>
>>>> Let's call it build_if_nonnull, as it builds the COND_EXPR as well 
>>>> as the test.
>>>
>>> Done.
>>>
>>>>
>>>>> +  /* The dynamic_cast might fail but so a warning might be justified
>>>>> +     but not when the operand is guarded.  See pr99251.  */
>>>>> +  if (B *q = p->bptr ())
>>>>> +    dynamic_cast<C*>(q)->g ();                // { dg-bogus 
>>>>> "\\\[-Wnonnull" }
>>>>
>>>> This guard is no more necessary than it is for static_cast; both 
>>>> cases deal with null arguments.  Let's not add these checks to the 
>>>> testcases.
>>>
>>> Done.
>>>
>>>>
>>>> This guard doesn't check for the mentioned case of dynamic_cast 
>>>> failing because the B* does not in fact point to a C.
>>>>
>>>> I think we can just change the dg-warning to dg-bogus.  Sure, 
>>>> dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn 
>>>> about arguments that *might* be null, only arguments that are 
>>>> *known* to be null.
>>>
>>> Done.
>>>
>>> I had added the 'if' to the test to make it clear why the warning
>>> isn't expected.  I replaced it with a comment to that effect.
>>
>>> FWIW, I do think a warning for dynamic_cast to a pointer would be
>>> helpful in the absence of an if statement in these cases:
>>>
>>>    void f (B *p)
>>>    {
>>>      dynamic_cast<D*>(p)->g ();
>>>    }
>>>
>>> Not because p may be null but because the result of the cast may be
>>> for a nonnull p.  If it's f's precondition that D is derived from B
>>> (in addition to p being nonnull) the cast would be better written as
>>> one to a reference to D.
>>
>> Agreed, or if it isn't a precondition,
>>
>> if (D* dp = dynamic_cast<D*>(p))
>>    dp->g();
>>
>>> Such a warning would need to be issued by the middle end and although
>>> it could be controlled by a new option (say -Wdynamic-cast, along with
>>> the cases discussed in PR 12277)
>>
>> Sounds good.
>>
>>> I don't think issuing -Wnonnull in this case would be inappropriate.
>>
>> I disagree; issuing -Wnonnull for this case would be wrong.  Again, 
>> -Wnonnull is supposed to warn when an argument *is* null, not when it 
>> *might* be null.
>>
>> I think warning about this case is a good idea, just not as part of 
>> -Wnonnull.
>>
>>> Anyway, that's something to consider for GCC 12.  For now, please see
>>> the revised patch.
>>
>>>     * rtti.c (ifnonnull): Rename...
>>>     (build_nonnull_test): ...to this.  Set no-warning bit on COND_EXPR.
>>
>> The ChangeLog needs updating.
>>
>>> +  /* Unlike static_cast, dynamic cast may fail for a nonnull operand 
>>> but
>>
>> Yes, but...
>>
>>> +     since the front end can't see if the cast is guarded against being
>>> +     invoked with a null
>>
>> No; my point was that whether the cast is guarded against being 
>> invoked with a null is no more relevant for dynamic_cast than it is 
>> for static_cast, as both casts give a null result for a null argument.
>>
>> For this test, dynamic_cast is not significantly different from 
>> static_cast.  For both casts, the bug was the compiler warning about a 
>> nullptr that it introduced itself.
> 
> It seems like splitting hairs.  The comment (much as the original
> if guard) is just meant to explain why -Wnonnull isn't expected in
> case a new warning is added that detects the bad assumption above.
> I want to make it clear that if/when that happens a failure here
> doesn't mislead the author into thinking we don't want any warning
> there at all.
> 
> I have reworded the comments yet again.
> 
>>
>>> verify there's no warning.  See also pr99251.  */
>>
>> Yes.
>>
>>> -  dynamic_cast<const C*>(p->bptr ())->g ();   // { dg-warning 
>>> "\\\[-Wnonnull" }
>>> +  dynamic_cast<const C*>(p)->g ();            // { dg-bogus 
>>> "\\\[-Wnonnull" }
>>
>> Please put back the ->bptr()s.
> 
> Done.

OK, thanks.

Jason