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

Martin Sebor msebor@gmail.com
Wed Nov 17 19:21:49 GMT 2021


On 11/17/21 11:31 AM, Jason Merrill wrote:
> 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.

Sorry, I don't know enough yet to judge this.

Since the error is unrelated to what I'm fixing I would prefer
not to introduce it in the same patch.  I'm happy to open
a separate bug for the missing error for the test case above,
look some more into why it isn't issued, and if it's decided
the error is intended either add the call back to trigger it
or do whatever else may be more appropriate).

Are you okay with me going ahead and committing the most recent
patch as is?

If not, do you want me to commit the previous version and change
the weak-3.c test to expect the error?

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



More information about the Gcc-patches mailing list