On 11/16/21 1:23 PM, Jason Merrill wrote:
> On 10/23/21 19:06, Martin Sebor wrote:
>> On 10/4/21 3:37 PM, Jason Merrill wrote:
>>> On 10/4/21 14:42, Martin Sebor wrote:
>>>> While resolving the recent -Waddress enhancement request (PR
>>>> PR102103) I came across a 2007 problem report about GCC 4 having
>>>> stopped warning for using the address of inline functions in
>>>> equality comparisons with null.  With inline functions being
>>>> commonplace in C++ this seems like an important use case for
>>>> the warning.
>>>>
>>>> The change that resulted in suppressing the warning in these
>>>> cases was introduced inadvertently in a fix for PR 22252.
>>>>
>>>> To restore the warning, the attached patch enhances
>>>> the decl_with_nonnull_addr_p() function to return true also for
>>>> weak symbols for which a definition has been provided.
>>>
>>> I think you probably want to merge this function with 
>>> fold-const.c:maybe_nonzero_address, which already handles more cases.
>>
>> maybe_nonzero_address() doesn't behave quite like
>> decl_with_nonnull_addr_p() expects and I'm reluctant to muck
>> around with the former too much since it's used for codegen,
>> while the latter just for warnings.  (There is even a case
>> where the functions don't behave the same, and would result
>> in different warnings between C and C++ without some extra
>> help.)
>>
>> So in the attached revision I just have maybe_nonzero_address()
>> call decl_with_nonnull_addr_p() and then refine the failing
>> (or uncertain) cases separately, with some overlap between
>> them.
>>
>> Since I worked on this someone complained that some instances
>> of the warning newly enhanced under PR102103 aren't suppresed
>> in code resulting from macro expansion.  Since it's trivial,
>> I include the fix for that report in this patch as well.
> 
>> +       allocated stroage might have a null address.  */
> 
> typo.
> 
> OK with that fixed.

After retesting the patch before committing I noticed it triggers
a regression in weak/weak-3.c that I missed the first time around.
Here's the test case:

extern void * ffoo1f (void);
void * foo1f (void)
{
   if (ffoo1f) /* { dg-warning "-Waddress" } */
     ffoo1f ();
   return 0;
}

void * ffoox1f (void) { return (void *)0; }
extern void * ffoo1f (void)  __attribute__((weak, alias ("ffoox1f")));

The unexpected error is:

a.c: At top level:
a.c:1:15: error: ‘ffoo1f’ declared weak after being used
     1 | extern void * ffoo1f (void);
       |               ^~~~~~

The error is caused by the new call to maybe_nonzero_address()
made from decl_with_nonnull_addr_p().  The call registers
the symbol as used.

So unless the error is desirable for this case I think it's
best to go back to the originally proposed solution.  I attach
it for reference and will plan to commit it tomorrow unless I
hear otherwise.

Martin

PS I don't know enough about the logic behind issuing this error
in other situations to tell for sure that it's wrong in this one
but I see no difference in the emitted code for a case in the same
test that declares the alias first, before taking its address and
that's accepted and this one.  I also checked that both Clang and
ICC accept the code either way, so I'm inclined to think the error
would be a bug.