[PATCH] restore ancient -Waddress for weak symbols [PR33925]

Jason Merrill jason@redhat.com
Wed Nov 17 18:31:24 GMT 2021


On 11/16/21 20:11, Martin Sebor wrote:
> 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.

Hmm, the error seems correct to me: we tested whether the address is 
nonzero in the dg-warning line, and presumably evaluating that test 
could depend on the absence of weak.

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



More information about the Gcc-patches mailing list