Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[James Hogan]

Hi,

On 08/05/14 07:31, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@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.berg@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.
scripts/Makefile.build:465: recipe for target 'fs/proc' failed
make[1]: *** [fs/proc] Error 2
make[1]: Target '__build' not remade because of errors.
Makefile:878: recipe for target 'fs' failed
make: *** [fs] Error 2
mm/pgtable-generic.c: In function ‘pmdp_clear_flush_young’:
mm/pgtable-generic.c:104: error: size of array ‘type name’ is negative
scripts/Makefile.build:318: recipe for target 'mm/pgtable-generic.o' failed
make[1]: *** [mm/pgtable-generic.o] Error 1
make[1]: Target '__build' not remade because of errors.
Makefile:878: recipe for target 'mm' failed
make: *** [mm] Error 2
In file included from arch/metag/include/asm/fixmap.h:55,
                 from arch/metag/include/asm/highmem.h:7,
                 from arch/metag/kernel/setup.c:37:
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/kernel/setup.o' failed
make[1]: *** [arch/metag/kernel/setup.o] Error 1
make[1]: Target '__build' not remade because of errors.
Makefile:878: recipe for target 'arch/metag/kernel' failed
make: *** [arch/metag/kernel] Error 2
make: Target '_all' not remade because of errors.

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[Johannes Berg]

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

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[James Hogan]

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

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[Andrew Morton]

On Mon, 12 May 2014 14:42:04 +0100 James Hogan <james.hogan@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.

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[Johannes Berg]

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

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[James Hogan]

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.hogan@imgtec.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Daniel Santos <daniel.santos@pobox.com>
Cc: Luciano Coelho <luciano.coelho@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@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

Re: [PATCH] compiler.h: don't use temporary variable in __compiletime_assert()

[Johannes Berg]

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.hogan@imgtec.com>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Cc: Daniel Santos <daniel.santos@pobox.com>
> Cc: Luciano Coelho <luciano.coelho@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Johannes Berg <johannes@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