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

Martin Sebor msebor@gmail.com
Thu Nov 18 01:27:50 GMT 2021

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:

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.
The weak-3.c test was added along with a fix for PR 6343.
Here's a discussion of the problem:

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