Bug 82963 - -Waddress too trigger happy
Summary: -Waddress too trigger happy
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2017-11-13 12:10 UTC by Michal Hocko
Modified: 2017-11-20 11:31 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Hocko 2017-11-13 12:10:06 UTC
Hi,
in the kernel we have a uggly^Wmacro to help printing numa mask defined as follows
#define nodemask_pr_args(maskp) MAX_NUMNODES : (maskp)->bits

I have updated it to allow NULL maskp as follows
#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL

but this has triggered warnings on usage where it is clear that maskp is never NULL. E.g.
In file included from include/linux/mmzone.h:17:0,                                                                                                                                                                 
                 from include/linux/mempolicy.h:10,                                                                                                                                                                
                 from mm/mempolicy.c:70:                                                                                                                                                                           
mm/mempolicy.c: In function 'mpol_to_str':                                                                                                                                                                         
include/linux/nodemask.h:107:41: warning: the address of 'nodes' will always evaluate as 'true' [-Waddress]                                                                                                        
 #define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL                                                                                                                        
                                         ^                                                                                                                                                                         
mm/mempolicy.c:2817:11: note: in expansion of macro 'nodemask_pr_args'
           nodemask_pr_args(&nodes));
           ^
include/linux/nodemask.h:107:69: warning: the address of 'nodes' will always evaluate as 'true' [-Waddress]
 #define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL
                                                                     ^
mm/mempolicy.c:2817:11: note: in expansion of macro 'nodemask_pr_args'
           nodemask_pr_args(&nodes));

While the warning is correct and the given mask will always resolve to the success path of the ternary operator I really fail to see why we should warn about this fact. I really do not see any potential problem which could be caused by this fact.

Moreover the warning itself is quite inconsistent. E.g. the following warns about the explicit &m but not for n. So I believe this is more of a suboptimal warning implementation than real intention.

#include <stdio.h>

#define MAX_NUMNODES 10
struct mask {
        void *bits;
};
#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL

int foo(void)
{
        struct mask m;
        struct mask *n = &m;

        printf("%*p\n", nodemask_pr_args(&m));
        printf("%*p\n", nodemask_pr_args(n));

        return 0;
}
Comment 1 Michal Hocko 2017-11-13 12:11:30 UTC
Btw. the compiler doesn't complain if I rewrite the macro to do an explicit NULL check
(maskp != NULL) ? MAX_NUMNODES : 0, (maskp != NULL) ? (maskp)->bits : NULL
Comment 2 Manuel López-Ibáñez 2017-11-15 16:10:31 UTC
(In reply to Michal Hocko from comment #0)
> While the warning is correct and the given mask will always resolve to the
> success path of the ternary operator I really fail to see why we should warn
> about this fact. I really do not see any potential problem which could be
> caused by this fact.

The source code says:

c-common.c-3292-            /* Common Ada/Pascal programmer's mistake.  */
c-common.c-3293-            warning_at (location,
c-common.c-3294-                        OPT_Waddress,
c-common.c:3295:                        "the address of %qD will always evaluate as %<true%>",

The work-around you found is probably the intended work-around. It would be good to document this. Care to send a patch? https://gcc.gnu.org/contribute.html#docchanges

> Moreover the warning itself is quite inconsistent. E.g. the following warns
> about the explicit &m but not for n. So I believe this is more of a
> suboptimal warning implementation than real intention.

This is because the warning is given in the front-end, which does not know the value of n. Not that it matters much, this is really trying to catch a typo, not the actual value of a pointer.
Comment 3 Michal Hocko 2017-11-16 08:06:40 UTC
(In reply to Manuel López-Ibáñez from comment #2)
[...]
> > Moreover the warning itself is quite inconsistent. E.g. the following warns
> > about the explicit &m but not for n. So I believe this is more of a
> > suboptimal warning implementation than real intention.
> 
> This is because the warning is given in the front-end, which does not know
> the value of n. Not that it matters much, this is really trying to catch a
> typo, not the actual value of a pointer.

Would it be possible to skip the warning for macros at least?
Comment 4 Arnd Bergmann 2017-11-17 11:09:37 UTC
(In reply to Michal Hocko from comment #1)
> Btw. the compiler doesn't complain if I rewrite the macro to do an explicit
> NULL check
> (maskp != NULL) ? MAX_NUMNODES : 0, (maskp != NULL) ? (maskp)->bits : NULL

As it turned out, that workaround only worked for gcc-7 and higher, as well as gcc-4.5 and lower. Anything in-between now prints a variation of "warning: the comparison will always evaluate as ‘true’ for the address of ‘nodes’ will never be NULL". I just sent another workaround to move the comparison into an inline function, which should always work.

It appears that at some point during the gcc-7 development, the "comparison will always evaluate as 'true'" logic was turned off for comparisons inside of macros, while the "the address of 'nodes' will always evaluate as 'true'" check did not get changed the same way.
Comment 5 Arnd Bergmann 2017-11-20 11:31:30 UTC
The change in gcc-7 was done in r235878 to address pr48778. From reading that pr, it sounds to me that both warnings should fall into the same category, it's just that this one is not as common, so nobody has complained before.