| Message ID | 20171117101545.119689-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 1334be3657dd02af0591d6d8adf0e6a60a7710a6 |
| Headers | show |
| Series | mm: fix nodemask printing | expand |
On Fri 17-11-17 11:15:45, Arnd Bergmann wrote: > The cleanup caused build warnings for constant mask pointers: > > mm/mempolicy.c: In function ‘mpol_to_str’: > ./include/linux/nodemask.h:108:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘nodes’ will never be NULL [-Waddress] > > An earlier workaround I suggested was incorporated in the version that > got merged, but that only solved the problem for gcc-7 and higher, while > gcc-4.6 through gcc-6.x still warn. > > This changes the printing again to use inline functions that make it > clear to the compiler that the line that does the NULL check has no > idea whether the argument is a constant NULL. > > Fixes: 0205f75571e3 ("mm: simplify nodemask printing") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thanks for the fixup. It is sad how a questionable warning makes the code worse... Does it make sense to have the warning enabled? Gcc bug [1] suggests there is no great interest into fixing it. Anyway to the patch Acked-by: Michal Hocko <mhocko@suse.com> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82963 > --- > I've done only minimal build testing on this, but did check it with > all compiler versions this time. > --- > include/linux/nodemask.h | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > index 15cab3967d6d..1fbde8a880d9 100644 > --- a/include/linux/nodemask.h > +++ b/include/linux/nodemask.h > @@ -104,9 +104,16 @@ extern nodemask_t _unused_nodemask_arg_; > * > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > */ > -#define nodemask_pr_args(maskp) \ > - ((maskp) != NULL) ? MAX_NUMNODES : 0, \ > - ((maskp) != NULL) ? (maskp)->bits : NULL > +#define nodemask_pr_args(maskp) __nodemask_pr_numnodes(maskp), \ > + __nodemask_pr_bits(maskp) > +static inline unsigned int __nodemask_pr_numnodes(const nodemask_t *m) > +{ > + return m ? MAX_NUMNODES : 0; > +} > +static inline const unsigned long *__nodemask_pr_bits(const nodemask_t *m) > +{ > + return m ? m->bits : NULL; > +} > > /* > * The inline keyword gives the compiler room to decide to inline, or > -- > 2.9.0 -- Michal Hocko SUSE Labs
On Mon, Nov 20, 2017 at 9:22 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Fri 17-11-17 11:15:45, Arnd Bergmann wrote: >> The cleanup caused build warnings for constant mask pointers: >> >> mm/mempolicy.c: In function ‘mpol_to_str’: >> ./include/linux/nodemask.h:108:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘nodes’ will never be NULL [-Waddress] >> >> An earlier workaround I suggested was incorporated in the version that >> got merged, but that only solved the problem for gcc-7 and higher, while >> gcc-4.6 through gcc-6.x still warn. >> >> This changes the printing again to use inline functions that make it >> clear to the compiler that the line that does the NULL check has no >> idea whether the argument is a constant NULL. >> >> Fixes: 0205f75571e3 ("mm: simplify nodemask printing") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Thanks for the fixup. It is sad how a questionable warning makes the > code worse... Does it make sense to have the warning enabled? > Gcc bug [1] suggests there is no great interest into fixing it. > > Anyway to the patch > Acked-by: Michal Hocko <mhocko@suse.com> > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82963 I think I found some real bugs when I started seeing that warning elsewhere, so I'd rather keep it enabled in general. I'd still agree that the gcc behavior in macros is questionable though, in particular since they decided to stop warning for the "!= NULL" case. I've found the commit that changed gcc-7 behavior and commented again in the bugzilla, but did not reopen. The inline function was mentioned in bugzilla there[2] as the preferred workaround. Arnd [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48778
diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h index 15cab3967d6d..1fbde8a880d9 100644 --- a/include/linux/nodemask.h +++ b/include/linux/nodemask.h @@ -104,9 +104,16 @@ extern nodemask_t _unused_nodemask_arg_; * * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. */ -#define nodemask_pr_args(maskp) \ - ((maskp) != NULL) ? MAX_NUMNODES : 0, \ - ((maskp) != NULL) ? (maskp)->bits : NULL +#define nodemask_pr_args(maskp) __nodemask_pr_numnodes(maskp), \ + __nodemask_pr_bits(maskp) +static inline unsigned int __nodemask_pr_numnodes(const nodemask_t *m) +{ + return m ? MAX_NUMNODES : 0; +} +static inline const unsigned long *__nodemask_pr_bits(const nodemask_t *m) +{ + return m ? m->bits : NULL; +} /* * The inline keyword gives the compiler room to decide to inline, or
The cleanup caused build warnings for constant mask pointers: mm/mempolicy.c: In function ‘mpol_to_str’: ./include/linux/nodemask.h:108:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘nodes’ will never be NULL [-Waddress] An earlier workaround I suggested was incorporated in the version that got merged, but that only solved the problem for gcc-7 and higher, while gcc-4.6 through gcc-6.x still warn. This changes the printing again to use inline functions that make it clear to the compiler that the line that does the NULL check has no idea whether the argument is a constant NULL. Fixes: 0205f75571e3 ("mm: simplify nodemask printing") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- I've done only minimal build testing on this, but did check it with all compiler versions this time. --- include/linux/nodemask.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) -- 2.9.0