Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi James,
> Subject: [PATCH] compiler.h: avoid sparse errors in
> __compiletime_error_fallback()
>
> Usually, BUG_ON and friends aren't even evaluated in sparse, but
> recently compiletime_assert_atomic_type() was added, and that now
> results in a sparse warning every time it is used.
>
> The reason turns out to be the temporary variable, after it sparse no
> longer considers the value to be a constant, and results in a warning
> and an error. The error is the more annoying part of this as it
> suppresses any further warnings in the same file, hiding other problems.
>
> Unfortunately the condition cannot be simply expanded out to avoid the
> temporary variable since it breaks compiletime_assert on old versions of
> GCC such as GCC 4.2.4 which the latest metag compiler is based on.
>
> Therefore #ifndef __CHECKER__ out the __compiletime_error_fallback which
> uses the potentially negative size array to trigger a conditional
> compiler error, so that sparse doesn't see it.
>
> Signed-off-by: James Hogan
> Cc: Johannes Berg
> Cc: Daniel Santos
> Cc: Luciano Coelho
> Cc: Peter Zijlstra
> Cc: Paul E. McKenney
> Cc: Andrew Morton
Acked-by: Johannes Berg
> It's not particularly pretty, if you can think of a better solution that
> doesn't break old GCC I'm all ears.
> #ifndef __compiletime_error
> # define __compiletime_error(message)
> -# define __compiletime_error_fallback(condition) \
> +/*
> + * Sparse complains of variable sized arrays due to the temporary variable in
> + * __compiletime_assert. Unfortunately we can't just expand it out to make
> + * sparse see a constant array size without breaking compiletime_assert on
> old
> + * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> + */
> +# ifndef __CHECKER__
> +# define __compiletime_error_fallback(condition) \
> do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> -#else
> +# endif
> +#endif
> +#ifndef __compiletime_error_fallback
> # define __compiletime_error_fallback(condition) do { } while (0)
> #endif
That's pretty much what I had in mind, I may have expressed it a bit
differently but the end result is the same.
Thanks,
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi Johannes,
On 13/05/14 08:31, Johannes Berg wrote:
> On Mon, 2014-05-12 at 14:16 -0700, Andrew Morton wrote:
>
>>> I don't understand why your patch should break things, I suspect it's
>>> related to the sparse behaviour you're trying to work around, but can we
>>> please drop this patch until a more portable workaround can be found?
>>
>> Older gcc's often have this problem.
>>
>> I suppose that build bustage is more serious than sparse false
>> positives so yes, let's please try to find an alternative.
>
> Since most people probably don't use sparse without compiling (in fact
> it's pretty difficult to do so) I'll just send a patch to disable
> __compiletime_assert for sparse - any objections?
I came up with the hack below yesterday evening. Is that the sort of
thing you mean? (feel free to use this or do something else, just my
2p).
Cheers
James
Subject: [PATCH] compiler.h: avoid sparse errors in
__compiletime_error_fallback()
Usually, BUG_ON and friends aren't even evaluated in sparse, but
recently compiletime_assert_atomic_type() was added, and that now
results in a sparse warning every time it is used.
The reason turns out to be the temporary variable, after it sparse no
longer considers the value to be a constant, and results in a warning
and an error. The error is the more annoying part of this as it
suppresses any further warnings in the same file, hiding other problems.
Unfortunately the condition cannot be simply expanded out to avoid the
temporary variable since it breaks compiletime_assert on old versions of
GCC such as GCC 4.2.4 which the latest metag compiler is based on.
Therefore #ifndef __CHECKER__ out the __compiletime_error_fallback which
uses the potentially negative size array to trigger a conditional
compiler error, so that sparse doesn't see it.
Signed-off-by: James Hogan
Cc: Johannes Berg
Cc: Daniel Santos
Cc: Luciano Coelho
Cc: Peter Zijlstra
Cc: Paul E. McKenney
Cc: Andrew Morton
---
It's not particularly pretty, if you can think of a better solution that
doesn't break old GCC I'm all ears.
---
include/linux/compiler.h | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index ee7239ea1583..64fdfe1cfcf0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -323,9 +323,18 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
int val, int expect);
#endif
#ifndef __compiletime_error
# define __compiletime_error(message)
-# define __compiletime_error_fallback(condition) \
+/*
+ * Sparse complains of variable sized arrays due to the temporary variable in
+ * __compiletime_assert. Unfortunately we can't just expand it out to make
+ * sparse see a constant array size without breaking compiletime_assert on old
+ * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
+ */
+# ifndef __CHECKER__
+# define __compiletime_error_fallback(condition) \
do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
-#else
+# endif
+#endif
+#ifndef __compiletime_error_fallback
# define __compiletime_error_fallback(condition) do { } while (0)
#endif
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Mon, 2014-05-12 at 14:16 -0700, Andrew Morton wrote: > > I don't understand why your patch should break things, I suspect it's > > related to the sparse behaviour you're trying to work around, but can we > > please drop this patch until a more portable workaround can be found? > > Older gcc's often have this problem. > > I suppose that build bustage is more serious than sparse false > positives so yes, let's please try to find an alternative. Since most people probably don't use sparse without compiling (in fact it's pretty difficult to do so) I'll just send a patch to disable __compiletime_assert for sparse - any objections? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Mon, 2014-05-12 at 14:16 -0700, Andrew Morton wrote: I don't understand why your patch should break things, I suspect it's related to the sparse behaviour you're trying to work around, but can we please drop this patch until a more portable workaround can be found? Older gcc's often have this problem. I suppose that build bustage is more serious than sparse false positives so yes, let's please try to find an alternative. Since most people probably don't use sparse without compiling (in fact it's pretty difficult to do so) I'll just send a patch to disable __compiletime_assert for sparse - any objections? johannes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi Johannes,
On 13/05/14 08:31, Johannes Berg wrote:
On Mon, 2014-05-12 at 14:16 -0700, Andrew Morton wrote:
I don't understand why your patch should break things, I suspect it's
related to the sparse behaviour you're trying to work around, but can we
please drop this patch until a more portable workaround can be found?
Older gcc's often have this problem.
I suppose that build bustage is more serious than sparse false
positives so yes, let's please try to find an alternative.
Since most people probably don't use sparse without compiling (in fact
it's pretty difficult to do so) I'll just send a patch to disable
__compiletime_assert for sparse - any objections?
I came up with the hack below yesterday evening. Is that the sort of
thing you mean? (feel free to use this or do something else, just my
2p).
Cheers
James
Subject: [PATCH] compiler.h: avoid sparse errors in
__compiletime_error_fallback()
Usually, BUG_ON and friends aren't even evaluated in sparse, but
recently compiletime_assert_atomic_type() was added, and that now
results in a sparse warning every time it is used.
The reason turns out to be the temporary variable, after it sparse no
longer considers the value to be a constant, and results in a warning
and an error. The error is the more annoying part of this as it
suppresses any further warnings in the same file, hiding other problems.
Unfortunately the condition cannot be simply expanded out to avoid the
temporary variable since it breaks compiletime_assert on old versions of
GCC such as GCC 4.2.4 which the latest metag compiler is based on.
Therefore #ifndef __CHECKER__ out the __compiletime_error_fallback which
uses the potentially negative size array to trigger a conditional
compiler error, so that sparse doesn't see it.
Signed-off-by: James Hogan james.ho...@imgtec.com
Cc: Johannes Berg johannes.b...@intel.com
Cc: Daniel Santos daniel.san...@pobox.com
Cc: Luciano Coelho luciano.coe...@intel.com
Cc: Peter Zijlstra pet...@infradead.org
Cc: Paul E. McKenney paul...@linux.vnet.ibm.com
Cc: Andrew Morton a...@linux-foundation.org
---
It's not particularly pretty, if you can think of a better solution that
doesn't break old GCC I'm all ears.
---
include/linux/compiler.h | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index ee7239ea1583..64fdfe1cfcf0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -323,9 +323,18 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
int val, int expect);
#endif
#ifndef __compiletime_error
# define __compiletime_error(message)
-# define __compiletime_error_fallback(condition) \
+/*
+ * Sparse complains of variable sized arrays due to the temporary variable in
+ * __compiletime_assert. Unfortunately we can't just expand it out to make
+ * sparse see a constant array size without breaking compiletime_assert on old
+ * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
+ */
+# ifndef __CHECKER__
+# define __compiletime_error_fallback(condition) \
do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
-#else
+# endif
+#endif
+#ifndef __compiletime_error_fallback
# define __compiletime_error_fallback(condition) do { } while (0)
#endif
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi James,
Subject: [PATCH] compiler.h: avoid sparse errors in
__compiletime_error_fallback()
Usually, BUG_ON and friends aren't even evaluated in sparse, but
recently compiletime_assert_atomic_type() was added, and that now
results in a sparse warning every time it is used.
The reason turns out to be the temporary variable, after it sparse no
longer considers the value to be a constant, and results in a warning
and an error. The error is the more annoying part of this as it
suppresses any further warnings in the same file, hiding other problems.
Unfortunately the condition cannot be simply expanded out to avoid the
temporary variable since it breaks compiletime_assert on old versions of
GCC such as GCC 4.2.4 which the latest metag compiler is based on.
Therefore #ifndef __CHECKER__ out the __compiletime_error_fallback which
uses the potentially negative size array to trigger a conditional
compiler error, so that sparse doesn't see it.
Signed-off-by: James Hogan james.ho...@imgtec.com
Cc: Johannes Berg johannes.b...@intel.com
Cc: Daniel Santos daniel.san...@pobox.com
Cc: Luciano Coelho luciano.coe...@intel.com
Cc: Peter Zijlstra pet...@infradead.org
Cc: Paul E. McKenney paul...@linux.vnet.ibm.com
Cc: Andrew Morton a...@linux-foundation.org
Acked-by: Johannes Berg johan...@sipsolutions.net
It's not particularly pretty, if you can think of a better solution that
doesn't break old GCC I'm all ears.
#ifndef __compiletime_error
# define __compiletime_error(message)
-# define __compiletime_error_fallback(condition) \
+/*
+ * Sparse complains of variable sized arrays due to the temporary variable in
+ * __compiletime_assert. Unfortunately we can't just expand it out to make
+ * sparse see a constant array size without breaking compiletime_assert on
old
+ * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
+ */
+# ifndef __CHECKER__
+# define __compiletime_error_fallback(condition) \
do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
-#else
+# endif
+#endif
+#ifndef __compiletime_error_fallback
# define __compiletime_error_fallback(condition) do { } while (0)
#endif
That's pretty much what I had in mind, I may have expressed it a bit
differently but the end result is the same.
Thanks,
johannes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Mon, 12 May 2014 14:42:04 +0100 James Hogan wrote:
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -324,11 +324,10 @@ void ftrace_likely_update(struct ftrace_branch_data
> > *f, int val, int expect);
> >
> > #define __compiletime_assert(condition, msg, prefix, suffix)
> > \
> > do {\
> > - bool __cond = !(condition); \
> > extern void prefix ## suffix(void) __compiletime_error(msg); \
> > - if (__cond) \
> > + if (!(condition)) \
> > prefix ## suffix(); \
> > - __compiletime_error_fallback(__cond); \
> > + __compiletime_error_fallback(!(condition)); \
> > } while (0)
> >
> > #define _compiletime_assert(condition, msg, prefix, suffix) \
> >
>
> Unfortunately this breaks the build of today's linux-next for the Meta
> architecture (arch/metag), which happens to use a fairly old compiler
> (based on gcc 4.2.4) which I presume is the reason why.
>
> A bunch of compile time asserts fail, even in code which should be
> optimised out. E.g. here's one which I analysed:
>
> mm/gup.c: In function ___follow_page_mask___:
> mm/gup.c:208: error: size of array ___type name___ is negative
>
> Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which
> expands to a BUILD_BUG(). However that line is inside an if block
> conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h
> defines inline to return 0, so the whole block should already be being
> optimised out.
>
> I don't understand why your patch should break things, I suspect it's
> related to the sparse behaviour you're trying to work around, but can we
> please drop this patch until a more portable workaround can be found?
Older gcc's often have this problem.
I suppose that build bustage is more serious than sparse false
positives so yes, let's please try to find an alternative.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi Johannes, On 12/05/14 15:38, Johannes Berg wrote: >> Unfortunately this breaks the build of today's linux-next for the Meta >> architecture (arch/metag), which happens to use a fairly old compiler >> (based on gcc 4.2.4) which I presume is the reason why. > > That's very odd. > > Unfortunately, I don't have most of arch/metag, it seems, where could I > get it? In particular no gup.c exists for metag in Linus's current tree. Hmm, mm/gup.c appears to be a new addition in linux-next from commit 3284cee59933 (mm: move get_user_pages()-related code to separate file) so probably wasn't the best example. My build output was from commit 0bed496ac091 (compiler.h: don't use temporary variable in __compiletime_assert()) which is the first bad commit according to a bisection of linux-next/stable..linux-next/master. >> A bunch of compile time asserts fail, even in code which should be >> optimised out. E.g. here's one which I analysed: >> >> mm/gup.c: In function ‘follow_page_mask’: >> mm/gup.c:208: error: size of array ‘type name’ is negative >> >> Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which >> expands to a BUILD_BUG(). However that line is inside an if block >> conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h >> defines inline to return 0, so the whole block should already be being >> optimised out. >> >> I don't understand why your patch should break things, I suspect it's >> related to the sparse behaviour you're trying to work around, but can we >> please drop this patch until a more portable workaround can be found? >> I'm happy to test further patches with metag if it helps. > > I don't really understand that either - if the compiler could prove that > the assignment to __cond was a constant, and remember that __cond is now > constant, I don't really see why it can't follow that through and > consider "!(condition)" a const?? > > I suppose the other option for the original problem is to ignore > _compiletime_assert() for sparse, like we do for BUG_ON(), but it'd > probably be good to analyse more why this particular code is broken now. The first one I analysed was strange too (the fixmap.h one). It appears that this particular assert was questionable anyway for metag which is why I didn't mention it, the case above is much more clear cut. Given an unsigned int idx argument the inline function fix_to_virt basically did: BUILD_BUG_ON(idx >= __end_of_fixed_addresses) where __end_of_fixed_addresses is an enum value which is 0 when CONFIG_HIGHMEM=n. In that case it took your patch for the compiler to apparently realise that an unsigned int is always >= 0, therefore the BUILD_BUG_ON will always fire, even though nothing actually called fix_to_virt from that source file so the code wasn't being used. I briefly attempted to reproduce this issue on other arches with newer compilers without success. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi, > Unfortunately this breaks the build of today's linux-next for the Meta > architecture (arch/metag), which happens to use a fairly old compiler > (based on gcc 4.2.4) which I presume is the reason why. That's very odd. Unfortunately, I don't have most of arch/metag, it seems, where could I get it? In particular no gup.c exists for metag in Linus's current tree. > A bunch of compile time asserts fail, even in code which should be > optimised out. E.g. here's one which I analysed: > > mm/gup.c: In function ‘follow_page_mask’: > mm/gup.c:208: error: size of array ‘type name’ is negative > > Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which > expands to a BUILD_BUG(). However that line is inside an if block > conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h > defines inline to return 0, so the whole block should already be being > optimised out. > > I don't understand why your patch should break things, I suspect it's > related to the sparse behaviour you're trying to work around, but can we > please drop this patch until a more portable workaround can be found? > I'm happy to test further patches with metag if it helps. I don't really understand that either - if the compiler could prove that the assignment to __cond was a constant, and remember that __cond is now constant, I don't really see why it can't follow that through and consider "!(condition)" a const?? I suppose the other option for the original problem is to ignore _compiletime_assert() for sparse, like we do for BUG_ON(), but it'd probably be good to analyse more why this particular code is broken now. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi,
On 08/05/14 07:31, Johannes Berg wrote:
> From: Johannes Berg
>
> Usually, BUG_ON and friends aren't even evaluated in sparse, but
> recently compiletime_assert_atomic_type() was added, and that now
> results in a sparse warning every time it is used.
>
> The reason turns out to be the temporary variable, after it sparse
> no longer considers the value to be a constant, and results in a
> warning and an error. The error is the more annoying part of this
> as it suppresses any further warnings in the same file, hiding
> other problems.
>
> Since this is all about compile time and the condition should be
> side-effect free to start with, there's no downside (apart maybe
> from a slight compilation time penalty?) to just duplicating it,
> leaving sparse able to evaluate it at check time, getting rid of
> the warning and error.
>
> Signed-off-by: Johannes Berg
> ---
> include/linux/compiler.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 2472740d7ab2..38c0e00ddef8 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -324,11 +324,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
> int val, int expect);
>
> #define __compiletime_assert(condition, msg, prefix, suffix) \
> do {\
> - bool __cond = !(condition); \
> extern void prefix ## suffix(void) __compiletime_error(msg); \
> - if (__cond) \
> + if (!(condition)) \
> prefix ## suffix(); \
> - __compiletime_error_fallback(__cond); \
> + __compiletime_error_fallback(!(condition)); \
> } while (0)
>
> #define _compiletime_assert(condition, msg, prefix, suffix) \
>
Unfortunately this breaks the build of today's linux-next for the Meta
architecture (arch/metag), which happens to use a fairly old compiler
(based on gcc 4.2.4) which I presume is the reason why.
A bunch of compile time asserts fail, even in code which should be
optimised out. E.g. here's one which I analysed:
mm/gup.c: In function ‘follow_page_mask’:
mm/gup.c:208: error: size of array ‘type name’ is negative
Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which
expands to a BUILD_BUG(). However that line is inside an if block
conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h
defines inline to return 0, so the whole block should already be being
optimised out.
I don't understand why your patch should break things, I suspect it's
related to the sparse behaviour you're trying to work around, but can we
please drop this patch until a more portable workaround can be found?
I'm happy to test further patches with metag if it helps.
Full "make ARCH=metag -k -s" output below.
Cheers
James
mm/gup.c: In function ‘follow_page_mask’:
mm/gup.c:208: error: size of array ‘type name’ is negative
In file included from arch/metag/include/asm/fixmap.h:55,
from arch/metag/mm/init.c:26:
include/asm-generic/fixmap.h: In function ‘fix_to_virt’:
include/asm-generic/fixmap.h:31: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'arch/metag/mm/init.o' failed
make[1]: *** [arch/metag/mm/init.o] Error 1
make[1]: Target '__build' not remade because of errors.
Makefile:878: recipe for target 'arch/metag/mm' failed
make: *** [arch/metag/mm] Error 2
mm/memory.c: In function ‘copy_pmd_range’:
mm/memory.c:965: error: size of array ‘type name’ is negative
mm/memory.c: In function ‘zap_pmd_range’:
mm/memory.c:1232: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/memory.o' failed
make[1]: *** [mm/memory.o] Error 1
mm/mremap.c: In function ‘move_page_tables’:
mm/mremap.c:197: error: size of array ‘type name’ is negative
mm/mprotect.c: In function ‘change_pmd_range’:
mm/mprotect.c:164: error: size of array ‘type name’ is negative
mm/mprotect.c:171: error: size of array ‘type name’ is negative
mm/mprotect.c:172: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/mremap.o' failed
make[1]: *** [mm/mremap.o] Error 1
scripts/Makefile.build:318: recipe for target 'mm/mprotect.o' failed
make[1]: *** [mm/mprotect.o] Error 1
fs/proc/task_mmu.c: In function ‘smaps_pmd’:
fs/proc/task_mmu.c:502: error: size of array ‘type name’ is negative
fs/proc/task_mmu.c:504: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/gup.o' failed
make[1]: *** [mm/gup.o] Error 1
scripts/Makefile.build:318: recipe for target 'fs/proc/task_mmu.o' failed
make[2]: *** [fs/proc/task_mmu.o] Error 1
make[2]: Target '__build' not remade because of errors.
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi,
On 08/05/14 07:31, Johannes Berg wrote:
From: Johannes Berg johannes.b...@intel.com
Usually, BUG_ON and friends aren't even evaluated in sparse, but
recently compiletime_assert_atomic_type() was added, and that now
results in a sparse warning every time it is used.
The reason turns out to be the temporary variable, after it sparse
no longer considers the value to be a constant, and results in a
warning and an error. The error is the more annoying part of this
as it suppresses any further warnings in the same file, hiding
other problems.
Since this is all about compile time and the condition should be
side-effect free to start with, there's no downside (apart maybe
from a slight compilation time penalty?) to just duplicating it,
leaving sparse able to evaluate it at check time, getting rid of
the warning and error.
Signed-off-by: Johannes Berg johannes.b...@intel.com
---
include/linux/compiler.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 2472740d7ab2..38c0e00ddef8 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -324,11 +324,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
int val, int expect);
#define __compiletime_assert(condition, msg, prefix, suffix) \
do {\
- bool __cond = !(condition); \
extern void prefix ## suffix(void) __compiletime_error(msg); \
- if (__cond) \
+ if (!(condition)) \
prefix ## suffix(); \
- __compiletime_error_fallback(__cond); \
+ __compiletime_error_fallback(!(condition)); \
} while (0)
#define _compiletime_assert(condition, msg, prefix, suffix) \
Unfortunately this breaks the build of today's linux-next for the Meta
architecture (arch/metag), which happens to use a fairly old compiler
(based on gcc 4.2.4) which I presume is the reason why.
A bunch of compile time asserts fail, even in code which should be
optimised out. E.g. here's one which I analysed:
mm/gup.c: In function ‘follow_page_mask’:
mm/gup.c:208: error: size of array ‘type name’ is negative
Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which
expands to a BUILD_BUG(). However that line is inside an if block
conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h
defines inline to return 0, so the whole block should already be being
optimised out.
I don't understand why your patch should break things, I suspect it's
related to the sparse behaviour you're trying to work around, but can we
please drop this patch until a more portable workaround can be found?
I'm happy to test further patches with metag if it helps.
Full make ARCH=metag -k -s output below.
Cheers
James
mm/gup.c: In function ‘follow_page_mask’:
mm/gup.c:208: error: size of array ‘type name’ is negative
In file included from arch/metag/include/asm/fixmap.h:55,
from arch/metag/mm/init.c:26:
include/asm-generic/fixmap.h: In function ‘fix_to_virt’:
include/asm-generic/fixmap.h:31: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'arch/metag/mm/init.o' failed
make[1]: *** [arch/metag/mm/init.o] Error 1
make[1]: Target '__build' not remade because of errors.
Makefile:878: recipe for target 'arch/metag/mm' failed
make: *** [arch/metag/mm] Error 2
mm/memory.c: In function ‘copy_pmd_range’:
mm/memory.c:965: error: size of array ‘type name’ is negative
mm/memory.c: In function ‘zap_pmd_range’:
mm/memory.c:1232: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/memory.o' failed
make[1]: *** [mm/memory.o] Error 1
mm/mremap.c: In function ‘move_page_tables’:
mm/mremap.c:197: error: size of array ‘type name’ is negative
mm/mprotect.c: In function ‘change_pmd_range’:
mm/mprotect.c:164: error: size of array ‘type name’ is negative
mm/mprotect.c:171: error: size of array ‘type name’ is negative
mm/mprotect.c:172: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/mremap.o' failed
make[1]: *** [mm/mremap.o] Error 1
scripts/Makefile.build:318: recipe for target 'mm/mprotect.o' failed
make[1]: *** [mm/mprotect.o] Error 1
fs/proc/task_mmu.c: In function ‘smaps_pmd’:
fs/proc/task_mmu.c:502: error: size of array ‘type name’ is negative
fs/proc/task_mmu.c:504: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/gup.o' failed
make[1]: *** [mm/gup.o] Error 1
scripts/Makefile.build:318: recipe for target 'fs/proc/task_mmu.o' failed
make[2]: *** [fs/proc/task_mmu.o] Error 1
make[2]: Target '__build' not remade because of errors.
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi, Unfortunately this breaks the build of today's linux-next for the Meta architecture (arch/metag), which happens to use a fairly old compiler (based on gcc 4.2.4) which I presume is the reason why. That's very odd. Unfortunately, I don't have most of arch/metag, it seems, where could I get it? In particular no gup.c exists for metag in Linus's current tree. A bunch of compile time asserts fail, even in code which should be optimised out. E.g. here's one which I analysed: mm/gup.c: In function ‘follow_page_mask’: mm/gup.c:208: error: size of array ‘type name’ is negative Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which expands to a BUILD_BUG(). However that line is inside an if block conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h defines inline to return 0, so the whole block should already be being optimised out. I don't understand why your patch should break things, I suspect it's related to the sparse behaviour you're trying to work around, but can we please drop this patch until a more portable workaround can be found? I'm happy to test further patches with metag if it helps. I don't really understand that either - if the compiler could prove that the assignment to __cond was a constant, and remember that __cond is now constant, I don't really see why it can't follow that through and consider !(condition) a const?? I suppose the other option for the original problem is to ignore _compiletime_assert() for sparse, like we do for BUG_ON(), but it'd probably be good to analyse more why this particular code is broken now. johannes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
Hi Johannes, On 12/05/14 15:38, Johannes Berg wrote: Unfortunately this breaks the build of today's linux-next for the Meta architecture (arch/metag), which happens to use a fairly old compiler (based on gcc 4.2.4) which I presume is the reason why. That's very odd. Unfortunately, I don't have most of arch/metag, it seems, where could I get it? In particular no gup.c exists for metag in Linus's current tree. Hmm, mm/gup.c appears to be a new addition in linux-next from commit 3284cee59933 (mm: move get_user_pages()-related code to separate file) so probably wasn't the best example. My build output was from commit 0bed496ac091 (compiler.h: don't use temporary variable in __compiletime_assert()) which is the first bad commit according to a bisection of linux-next/stable..linux-next/master. A bunch of compile time asserts fail, even in code which should be optimised out. E.g. here's one which I analysed: mm/gup.c: In function ‘follow_page_mask’: mm/gup.c:208: error: size of array ‘type name’ is negative Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which expands to a BUILD_BUG(). However that line is inside an if block conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h defines inline to return 0, so the whole block should already be being optimised out. I don't understand why your patch should break things, I suspect it's related to the sparse behaviour you're trying to work around, but can we please drop this patch until a more portable workaround can be found? I'm happy to test further patches with metag if it helps. I don't really understand that either - if the compiler could prove that the assignment to __cond was a constant, and remember that __cond is now constant, I don't really see why it can't follow that through and consider !(condition) a const?? I suppose the other option for the original problem is to ignore _compiletime_assert() for sparse, like we do for BUG_ON(), but it'd probably be good to analyse more why this particular code is broken now. The first one I analysed was strange too (the fixmap.h one). It appears that this particular assert was questionable anyway for metag which is why I didn't mention it, the case above is much more clear cut. Given an unsigned int idx argument the inline function fix_to_virt basically did: BUILD_BUG_ON(idx = __end_of_fixed_addresses) where __end_of_fixed_addresses is an enum value which is 0 when CONFIG_HIGHMEM=n. In that case it took your patch for the compiler to apparently realise that an unsigned int is always = 0, therefore the BUILD_BUG_ON will always fire, even though nothing actually called fix_to_virt from that source file so the code wasn't being used. I briefly attempted to reproduce this issue on other arches with newer compilers without success. Cheers James -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Mon, 12 May 2014 14:42:04 +0100 James Hogan james.ho...@imgtec.com wrote:
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -324,11 +324,10 @@ void ftrace_likely_update(struct ftrace_branch_data
*f, int val, int expect);
#define __compiletime_assert(condition, msg, prefix, suffix)
\
do {\
- bool __cond = !(condition); \
extern void prefix ## suffix(void) __compiletime_error(msg); \
- if (__cond) \
+ if (!(condition)) \
prefix ## suffix(); \
- __compiletime_error_fallback(__cond); \
+ __compiletime_error_fallback(!(condition)); \
} while (0)
#define _compiletime_assert(condition, msg, prefix, suffix) \
Unfortunately this breaks the build of today's linux-next for the Meta
architecture (arch/metag), which happens to use a fairly old compiler
(based on gcc 4.2.4) which I presume is the reason why.
A bunch of compile time asserts fail, even in code which should be
optimised out. E.g. here's one which I analysed:
mm/gup.c: In function ___follow_page_mask___:
mm/gup.c:208: error: size of array ___type name___ is negative
Line 208 uses HPAGE_PMD_NR which expands to a HPAGE_PMD_SHIFT, which
expands to a BUILD_BUG(). However that line is inside an if block
conditioned on pmd_trans_huge(*pmd) which include/asm-generic/pgtable.h
defines inline to return 0, so the whole block should already be being
optimised out.
I don't understand why your patch should break things, I suspect it's
related to the sparse behaviour you're trying to work around, but can we
please drop this patch until a more portable workaround can be found?
Older gcc's often have this problem.
I suppose that build bustage is more serious than sparse false
positives so yes, let's please try to find an alternative.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Thu, May 08, 2014 at 08:31:25AM +0200, Johannes Berg wrote:
> From: Johannes Berg
>
> Usually, BUG_ON and friends aren't even evaluated in sparse, but
> recently compiletime_assert_atomic_type() was added, and that now
> results in a sparse warning every time it is used.
>
> The reason turns out to be the temporary variable, after it sparse
> no longer considers the value to be a constant, and results in a
> warning and an error. The error is the more annoying part of this
> as it suppresses any further warnings in the same file, hiding
> other problems.
>
> Since this is all about compile time and the condition should be
> side-effect free to start with, there's no downside (apart maybe
> from a slight compilation time penalty?) to just duplicating it,
> leaving sparse able to evaluate it at check time, getting rid of
> the warning and error.
>
> Signed-off-by: Johannes Berg
Acked-by: Paul E. McKenney
> ---
> include/linux/compiler.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 2472740d7ab2..38c0e00ddef8 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -324,11 +324,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
> int val, int expect);
>
> #define __compiletime_assert(condition, msg, prefix, suffix) \
> do {\
> - bool __cond = !(condition); \
> extern void prefix ## suffix(void) __compiletime_error(msg); \
> - if (__cond) \
> + if (!(condition)) \
> prefix ## suffix(); \
> - __compiletime_error_fallback(__cond); \
> + __compiletime_error_fallback(!(condition)); \
> } while (0)
>
> #define _compiletime_assert(condition, msg, prefix, suffix) \
> --
> 2.0.0.rc0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Thu, 2014-05-08 at 08:31 +0200, Johannes Berg wrote: > From: Johannes Berg > > Usually, BUG_ON and friends aren't even evaluated in sparse, but > recently compiletime_assert_atomic_type() was added, and that now > results in a sparse warning every time it is used. > > The reason turns out to be the temporary variable, after it sparse > no longer considers the value to be a constant, and results in a > warning and an error. The error is the more annoying part of this > as it suppresses any further warnings in the same file, hiding > other problems. > > Since this is all about compile time and the condition should be > side-effect free to start with, there's no downside (apart maybe > from a slight compilation time penalty?) to just duplicating it, > leaving sparse able to evaluate it at check time, getting rid of > the warning and error. > > Signed-off-by: Johannes Berg > --- Tested-by: Luciano Coelho -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Thu, May 08, 2014 at 08:31:25AM +0200, Johannes Berg wrote: > From: Johannes Berg > > Usually, BUG_ON and friends aren't even evaluated in sparse, but > recently compiletime_assert_atomic_type() was added, and that now > results in a sparse warning every time it is used. > > The reason turns out to be the temporary variable, after it sparse > no longer considers the value to be a constant, and results in a > warning and an error. The error is the more annoying part of this > as it suppresses any further warnings in the same file, hiding > other problems. > > Since this is all about compile time and the condition should be > side-effect free to start with, there's no downside (apart maybe > from a slight compilation time penalty?) to just duplicating it, > leaving sparse able to evaluate it at check time, getting rid of > the warning and error. > > Signed-off-by: Johannes Berg Acked-by: Peter Zijlstra -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Thu, May 08, 2014 at 08:31:25AM +0200, Johannes Berg wrote: From: Johannes Berg johannes.b...@intel.com Usually, BUG_ON and friends aren't even evaluated in sparse, but recently compiletime_assert_atomic_type() was added, and that now results in a sparse warning every time it is used. The reason turns out to be the temporary variable, after it sparse no longer considers the value to be a constant, and results in a warning and an error. The error is the more annoying part of this as it suppresses any further warnings in the same file, hiding other problems. Since this is all about compile time and the condition should be side-effect free to start with, there's no downside (apart maybe from a slight compilation time penalty?) to just duplicating it, leaving sparse able to evaluate it at check time, getting rid of the warning and error. Signed-off-by: Johannes Berg johannes.b...@intel.com Acked-by: Peter Zijlstra pet...@infradead.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Thu, 2014-05-08 at 08:31 +0200, Johannes Berg wrote: From: Johannes Berg johannes.b...@intel.com Usually, BUG_ON and friends aren't even evaluated in sparse, but recently compiletime_assert_atomic_type() was added, and that now results in a sparse warning every time it is used. The reason turns out to be the temporary variable, after it sparse no longer considers the value to be a constant, and results in a warning and an error. The error is the more annoying part of this as it suppresses any further warnings in the same file, hiding other problems. Since this is all about compile time and the condition should be side-effect free to start with, there's no downside (apart maybe from a slight compilation time penalty?) to just duplicating it, leaving sparse able to evaluate it at check time, getting rid of the warning and error. Signed-off-by: Johannes Berg johannes.b...@intel.com --- Tested-by: Luciano Coelho luciano.coe...@intel.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()
On Thu, May 08, 2014 at 08:31:25AM +0200, Johannes Berg wrote:
From: Johannes Berg johannes.b...@intel.com
Usually, BUG_ON and friends aren't even evaluated in sparse, but
recently compiletime_assert_atomic_type() was added, and that now
results in a sparse warning every time it is used.
The reason turns out to be the temporary variable, after it sparse
no longer considers the value to be a constant, and results in a
warning and an error. The error is the more annoying part of this
as it suppresses any further warnings in the same file, hiding
other problems.
Since this is all about compile time and the condition should be
side-effect free to start with, there's no downside (apart maybe
from a slight compilation time penalty?) to just duplicating it,
leaving sparse able to evaluate it at check time, getting rid of
the warning and error.
Signed-off-by: Johannes Berg johannes.b...@intel.com
Acked-by: Paul E. McKenney paul...@linux.vnet.ibm.com
---
include/linux/compiler.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 2472740d7ab2..38c0e00ddef8 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -324,11 +324,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
int val, int expect);
#define __compiletime_assert(condition, msg, prefix, suffix) \
do {\
- bool __cond = !(condition); \
extern void prefix ## suffix(void) __compiletime_error(msg); \
- if (__cond) \
+ if (!(condition)) \
prefix ## suffix(); \
- __compiletime_error_fallback(__cond); \
+ __compiletime_error_fallback(!(condition)); \
} while (0)
#define _compiletime_assert(condition, msg, prefix, suffix) \
--
2.0.0.rc0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
