Running sparse on page_alloc.c today, it errors out:
include/linux/gfp.h:254:17: error: bad constant expression
include/linux/gfp.h:254:17: error: cannot size expression
which is a line in gfp_zone():
BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
That's really unfortunate, because it ends up hiding all of the other
legitimate sparse messages like this:
mm/page_alloc.c:5315:59: warning: incorrect type in argument 1 (different base types)
mm/page_alloc.c:5315:59: expected unsigned long [unsigned] [usertype] size
mm/page_alloc.c:5315:59: got restricted gfp_t [usertype] <noident>
...
Having sparse be able to catch these very oopsable bugs is a lot more
important than keeping a BUILD_BUG_ON(). Kill the BUILD_BUG_ON().
Compiles on x86_64 with and without CONFIG_DEBUG_VM=y. defconfig
boots fine for me.
Signed-off-by: Dave Hansen <[email protected]>
---
linux-2.6.git-dave/include/linux/gfp.h | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff -puN include/linux/gfp.h~make-sparse-happy-with-gfp_h include/linux/gfp.h
--- linux-2.6.git/include/linux/gfp.h~make-sparse-happy-with-gfp_h 2011-04-14 14:47:02.629275904 -0700
+++ linux-2.6.git-dave/include/linux/gfp.h 2011-04-14 14:47:38.813272674 -0700
@@ -249,14 +249,9 @@ static inline enum zone_type gfp_zone(gf
z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
((1 << ZONES_SHIFT) - 1);
-
- if (__builtin_constant_p(bit))
- BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
- else {
#ifdef CONFIG_DEBUG_VM
- BUG_ON((GFP_ZONE_BAD >> bit) & 1);
+ BUG_ON((GFP_ZONE_BAD >> bit) & 1);
#endif
- }
return z;
}
_
Hello,
> diff -puN include/linux/gfp.h~make-sparse-happy-with-gfp_h include/linux/gfp.h
> --- linux-2.6.git/include/linux/gfp.h~make-sparse-happy-with-gfp_h 2011-04-14 14:47:02.629275904 -0700
> +++ linux-2.6.git-dave/include/linux/gfp.h 2011-04-14 14:47:38.813272674 -0700
> @@ -249,14 +249,9 @@ static inline enum zone_type gfp_zone(gf
>
> z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
> ((1 << ZONES_SHIFT) - 1);
> -
> - if (__builtin_constant_p(bit))
> - BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> - else {
> #ifdef CONFIG_DEBUG_VM
> - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> #endif
> - }
> return z;
Why don't you use VM_BUG_ON?
On Fri, 2011-04-15 at 12:14 +0900, KOSAKI Motohiro wrote:
> > #ifdef CONFIG_DEBUG_VM
> > - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > #endif
> > - }
> > return z;
>
> Why don't you use VM_BUG_ON?
I was just trying to make a minimal patch that did a single thing.
Feel free to submit another one that does that. I'm sure there are a
couple more places that could use similar love.
-- Dave
> Hello,
>
> > diff -puN include/linux/gfp.h~make-sparse-happy-with-gfp_h include/linux/gfp.h
> > --- linux-2.6.git/include/linux/gfp.h~make-sparse-happy-with-gfp_h 2011-04-14 14:47:02.629275904 -0700
> > +++ linux-2.6.git-dave/include/linux/gfp.h 2011-04-14 14:47:38.813272674 -0700
> > @@ -249,14 +249,9 @@ static inline enum zone_type gfp_zone(gf
> >
> > z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
> > ((1 << ZONES_SHIFT) - 1);
> > -
> > - if (__builtin_constant_p(bit))
> > - BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > - else {
> > #ifdef CONFIG_DEBUG_VM
> > - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > #endif
> > - }
> > return z;
>
> Why don't you use VM_BUG_ON?
After while thinking, I decided to make another patch. If we take your
approach we will remove all BUILD_BUG_ON eventually. It's no happy result.
>From 2da32b2875a6bd0bb0166993b4663eac0c5d1d6d Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <[email protected]>
Date: Fri, 15 Apr 2011 13:37:24 +0900
Subject: [PATCH] define dummy BUILD_BUG_ON definition for sparse
BUILD_BUG_ON() makes syntax error to detect coding error. Then it
naturally makes sparse error too. It reduce sparse usefulness.
Then, this patch makes dummy BUILD_BUG_ON() definition for sparse.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
include/linux/kernel.h | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 00cec4d..9ac44b8 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -637,6 +637,14 @@ struct sysinfo {
char _f[20-2*sizeof(long)-sizeof(int)]; /* Padding: libc5 uses this.. */
};
+#ifdef __CHECKER__
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n)
+#define BUILD_BUG_ON_ZERO(e)
+#define BUILD_BUG_ON_NULL(e)
+#define BUILD_BUG_ON(condition)
+#else /* __CHECKER__ */
+
/* Force a compilation error if a constant expression is not a power of 2 */
#define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
@@ -673,6 +681,7 @@ extern int __build_bug_on_failed;
if (condition) __build_bug_on_failed = 1; \
} while(0)
#endif
+#endif /* __CHECKER__ */
/* Trap pasters of __FUNCTION__ at compile-time */
#define __FUNCTION__ (__func__)
--
1.7.3.1
This fixes another sparse splat.
>From 711131e2e16925970a67103156af1296993dbc93 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <[email protected]>
Date: Fri, 15 Apr 2011 13:28:16 +0900
Subject: [PATCH] define __must_be_array() for __CHECKER__
commit c5e631cf65f (ARRAY_SIZE: check for type) added __must_be_array().
but sparse can't parse this gcc extention.
Then, now make C=2 makes following sparse errors a lot.
kernel/futex.c:2699:25: error: No right hand side of '+'-expression
Because __must_be_array() is used for ARRAY_SIZE() macro and it is
used very widely.
This patch fixes it.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
include/linux/compiler-gcc.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index cb4c1eb..59e4028 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -34,8 +34,12 @@
__asm__ ("" : "=r"(__ptr) : "0"(ptr)); \
(typeof(ptr)) (__ptr + (off)); })
+#ifdef __CHECKER__
+#define __must_be_array(arr) 0
+#else
/* &a[0] degrades to a pointer: a different type from an array */
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
+#endif
/*
* Force always-inline if the user requests it so via the .config,
--
1.7.3.1
>From 4560a800c056714a7b5f9dc813460698717e6998 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <[email protected]>
Date: Fri, 15 Apr 2011 13:58:07 +0900
Subject: [PATCH] Undef __compiletime_{warning,error} if __CHECKER__ is defined
sparse can't parse warning and error attribute. then they should
be hidden from sparse.
Signed-off-by: KOSAKI Motohiro <[email protected]>
Cc: Arjan van de Ven <[email protected]>
---
include/linux/compiler-gcc4.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 64b7c00..dfadc96 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -51,7 +51,7 @@
#if __GNUC_MINOR__ > 0
#define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
#endif
-#if __GNUC_MINOR__ >= 4
+#if __GNUC_MINOR__ >= 4 && !defined(__CHECKER__)
#define __compiletime_warning(message) __attribute__((warning(message)))
#define __compiletime_error(message) __attribute__((error(message)))
#endif
--
1.7.3.1
Hello,
> On Fri, 2011-04-15 at 12:14 +0900, KOSAKI Motohiro wrote:
> > > #ifdef CONFIG_DEBUG_VM
> > > - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > > + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > > #endif
> > > - }
> > > return z;
> >
> > Why don't you use VM_BUG_ON?
>
> I was just trying to make a minimal patch that did a single thing.
>
> Feel free to submit another one that does that. I'm sure there are a
> couple more places that could use similar love.
I posted another approach patches a second ago. Could you please see it?
On Fri, 2011-04-15 at 14:33 +0900, KOSAKI Motohiro wrote:
> Hello,
> > On Fri, 2011-04-15 at 12:14 +0900, KOSAKI Motohiro wrote:
> > > > #ifdef CONFIG_DEBUG_VM
> > > > - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > > > + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > > > #endif
> > > > - }
> > > > return z;
> > >
> > > Why don't you use VM_BUG_ON?
> >
> > I was just trying to make a minimal patch that did a single thing.
> >
> > Feel free to submit another one that does that. I'm sure there are a
> > couple more places that could use similar love.
>
> I posted another approach patches a second ago. Could you please see it?
Those both look sane to me. Those weren't biting me in particular, and
they don't fix the issue I was seeing. But, they do seem necessary to
reduce some of the noise.
CC'ing the sparse mailing list. We're seeing a couple of cases where
some gcc-isms are either stopping sparse from finding real bugs:
http://marc.info/?l=linux-mm&m=130282454732689&w=2
or creating a lot of noise on some builds:
http://marc.info/?l=linux-mm&m=130284428614058&w=2
http://marc.info/?l=linux-mm&m=130284431014077&w=2
-- Dave