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

[Jason Merrill]

On 11/17/21 20:27, Martin Sebor wrote:
> On 11/17/21 12:21 PM, Martin Sebor wrote:
>> 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.
> 
> I've created a test case involving just a weak symbol (no alias)
> that shows that the front end folds to true a test of the address
> of a symbol subsequently declared weak:
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103310
> 
> Clang and ICC do the same thing here; only Clang and GCC issue
> a warning that the inequality is folded to true (here's a live
> link to it: https://godbolt.org/z/a8Tx9Psee).
> 
> This doesn't seem ideal but I wouldn't call it a serious problem.
> 
> The case in weak-3.c is different: there the weak symbol is
> an alias for a locally defined function.  There, the alias cannot
> become null and so folding the test is safe and giving an error
> for it would be a regression.  I would tend to view issuing
> a hard error in this case a more serious problem than the first
> (especially after reading the discussion below), but YMMV.

Ah, very good point.

This issue seems to go back to Honza's r5-3627, which changed 
symtab_node::get to symtab_node::get_create in the code that became 
maybe_nonzero_address, so that we decide early whether a particular 
function is weak or not.

This was done so that constant-evaluation could properly decide that the 
address of a function is non-null.  But it's harmful when we do that for 
speculative folding; we should only return a definitive answer, and set 
refuse_visibility_changes, when a constant result is required.

I guess we need a way to tell fold that we really want a constant value, 
have the C++ front end set that for manifestly-constant-evaluated 
expressions, and only use get_create in that case.

> The weak-3.c test was added along with a fix for PR 6343.
> Here's a discussion of the problem:
>    https://gcc.gnu.org/legacy-ml/gcc/2002-04/msg00838.html
> 
> Please let me know which of the alternatives below you prefer
> or if you want something else.
> 
>> 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?

OK.  I'll copy my analysis above into PR103310.

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