[PATCH] kernel.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[PATCH] kernel.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Fabio Estevam]

Building imx_v6_v7_defconfig generated the following build error:

arch/arm/mach-imx/cpu_op-mx51.c: In function 'mx51_get_cpu_op':
arch/arm/mach-imx/cpu_op-mx51.c:27: error: implicit declaration of function 'BUILD_BUG_ON_ZERO'

commit 6f863554 (kernel.h: doesn't explicitly use bug.h, so don't include it.)
has removed asm/debug.h from linux/kernel.h.

Quoting Russell King (http://www.spinics.net/lists/arm-kernel/msg161916.html)

"linux/kernel.h _does_ use bug stuff - in ARRAY_SIZE().

ARRAY_SIZE() uses __must_be_array(), which is defined in linux/compiler-gcc.h,
which is obtained via linux/compiler.h and linux/linkage.h.

linux/compiler-gcc.h defines __must_be_array() to be:
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))

and BUILD_BUG_ON_ZERO used to be in linux/kernel.h but got moved to
linux/bug.h.

Hence why people are seeing build breakage with ARRAY_SIZE()."

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 include/linux/kernel.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index ed9f92e..9c0a86b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -21,6 +21,7 @@
 #include <linux/typecheck.h>
 #include <linux/printk.h>
 #include <linux/dynamic_debug.h>
+#include <linux/bug.h>
 #include <asm/byteorder.h>
 
 #define USHRT_MAX	((u16)(~0U))
-- 
1.7.1

Re: [PATCH] kernel.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Sam Ravnborg]

On Tue, Feb 28, 2012 at 10:41:39AM -0300, Fabio Estevam wrote:
> Building imx_v6_v7_defconfig generated the following build error:
> 
> arch/arm/mach-imx/cpu_op-mx51.c: In function 'mx51_get_cpu_op':
> arch/arm/mach-imx/cpu_op-mx51.c:27: error: implicit declaration of function 'BUILD_BUG_ON_ZERO'
> 
> commit 6f863554 (kernel.h: doesn't explicitly use bug.h, so don't include it.)
> has removed asm/debug.h from linux/kernel.h.
> 
> Quoting Russell King (http://www.spinics.net/lists/arm-kernel/msg161916.html)
> 
> "linux/kernel.h _does_ use bug stuff - in ARRAY_SIZE().
> 
> ARRAY_SIZE() uses __must_be_array(), which is defined in linux/compiler-gcc.h,
> which is obtained via linux/compiler.h and linux/linkage.h.
> 
> linux/compiler-gcc.h defines __must_be_array() to be:
> #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))

In which case linux/compiler-gcc.h uses bug.h - so this is where you
should add the include.
Not in kernel.h that just happens to use __must_be_array()
We do not want to add "#include bug.h" to all header files
that happens to use __must_be_array.

	Sam

Re: [PATCH] kernel.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Paul Gortmaker]

On 12-02-28 10:51 AM, Sam Ravnborg wrote:
> On Tue, Feb 28, 2012 at 10:41:39AM -0300, Fabio Estevam wrote:
>> Building imx_v6_v7_defconfig generated the following build error:
>>
>> arch/arm/mach-imx/cpu_op-mx51.c: In function 'mx51_get_cpu_op':
>> arch/arm/mach-imx/cpu_op-mx51.c:27: error: implicit declaration of function 'BUILD_BUG_ON_ZERO'
>>
>> commit 6f863554 (kernel.h: doesn't explicitly use bug.h, so don't include it.)
>> has removed asm/debug.h from linux/kernel.h.
>>
>> Quoting Russell King (http://www.spinics.net/lists/arm-kernel/msg161916.html)
>>
>> "linux/kernel.h _does_ use bug stuff - in ARRAY_SIZE().
>>
>> ARRAY_SIZE() uses __must_be_array(), which is defined in linux/compiler-gcc.h,
>> which is obtained via linux/compiler.h and linux/linkage.h.
>>
>> linux/compiler-gcc.h defines __must_be_array() to be:
>> #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> 
> In which case linux/compiler-gcc.h uses bug.h - so this is where you
> should add the include.
> Not in kernel.h that just happens to use __must_be_array()
> We do not want to add "#include bug.h" to all header files
> that happens to use __must_be_array.

The approach I was taking was to consider something "used" if it was
actually in a static inline[1].  So in that sense, the kernel.h does
not "use" ARRAY_SIZE, but only defines it.  This seems to be what has
been done in the past, and folks have even converted trivial static
inline functions to macros just to use this concept of "used" to
help avoid tangling includes any further than they currently are.

If we expand the definition of used to cover the #define case, then
we'll pretty much have all headers implicitly present everywhere,
and folks won't have a clue what functions they are actually making
use of.  They'll be able to get away with including kernel.h and
largely nothing else - having everything implicitly present, I think.

rmk's example about the uncompressor code is a good one I think, in
terms of having to be explicit about what you are using/including,
since it forces people to think a bit more about what they are using.

So, sticking bug.h into another header that is largely present in
every compile was something that I was actually trying to avoid if
possible....  maybe I should abandon that goal?

Paul.

[1] https://lkml.org/lkml/2012/1/26/529

> 
> 	Sam

Re: [PATCH] kernel.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Sam Ravnborg]

On Tue, Feb 28, 2012 at 11:43:36AM -0500, Paul Gortmaker wrote:
> On 12-02-28 10:51 AM, Sam Ravnborg wrote:
> > On Tue, Feb 28, 2012 at 10:41:39AM -0300, Fabio Estevam wrote:
> >> Building imx_v6_v7_defconfig generated the following build error:
> >>
> >> arch/arm/mach-imx/cpu_op-mx51.c: In function 'mx51_get_cpu_op':
> >> arch/arm/mach-imx/cpu_op-mx51.c:27: error: implicit declaration of function 'BUILD_BUG_ON_ZERO'
> >>
> >> commit 6f863554 (kernel.h: doesn't explicitly use bug.h, so don't include it.)
> >> has removed asm/debug.h from linux/kernel.h.
> >>
> >> Quoting Russell King (http://www.spinics.net/lists/arm-kernel/msg161916.html)
> >>
> >> "linux/kernel.h _does_ use bug stuff - in ARRAY_SIZE().
> >>
> >> ARRAY_SIZE() uses __must_be_array(), which is defined in linux/compiler-gcc.h,
> >> which is obtained via linux/compiler.h and linux/linkage.h.
> >>
> >> linux/compiler-gcc.h defines __must_be_array() to be:
> >> #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> > 
> > In which case linux/compiler-gcc.h uses bug.h - so this is where you
> > should add the include.
> > Not in kernel.h that just happens to use __must_be_array()
> > We do not want to add "#include bug.h" to all header files
> > that happens to use __must_be_array.
> 
> The approach I was taking was to consider something "used" if it was
> actually in a static inline[1]. So in that sense, the kernel.h does
> not "use" ARRAY_SIZE, but only defines it.  This seems to be what has
> been done in the past, and folks have even converted trivial static
> inline functions to macros just to use this concept of "used" to
> help avoid tangling includes any further than they currently are.

We have a macro which uses a macro which uses a macro from bug.h.
Should we include bug.h with the first or the second macro?

I guess in this case it does not matter much.

	Sam

[PATCH v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Fabio Estevam]

Building imx_v6_v7_defconfig generated the following build error:

arch/arm/mach-imx/cpu_op-mx51.c: In function 'mx51_get_cpu_op':
arch/arm/mach-imx/cpu_op-mx51.c:27: error: implicit declaration of function 'BUILD_BUG_ON_ZERO'

commit 6f863554 (kernel.h: doesn't explicitly use bug.h, so don't include it.)
has removed asm/debug.h from linux/kernel.h.

Quoting Russell King (http://www.spinics.net/lists/arm-kernel/msg161916.html)

"linux/kernel.h _does_ use bug stuff - in ARRAY_SIZE().

ARRAY_SIZE() uses __must_be_array(), which is defined in linux/compiler-gcc.h,
which is obtained via linux/compiler.h and linux/linkage.h.

linux/compiler-gcc.h defines __must_be_array() to be:
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))

and BUILD_BUG_ON_ZERO used to be in linux/kernel.h but got moved to
linux/bug.h.

Hence why people are seeing build breakage with ARRAY_SIZE()."

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Move linux/bug.h to linux/compiler.h

 include/linux/compiler.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 4a24354..0300fb7 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -46,6 +46,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #ifdef __GNUC__
 #include <linux/compiler-gcc.h>
+#include <linux/bug.h>
 #endif
 
 #define notrace __attribute__((no_instrument_function))
-- 
1.7.1

Re: [PATCH v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Paul Gortmaker]

On Thu, Mar 1, 2012 at 8:15 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Building imx_v6_v7_defconfig generated the following build error:
>
> arch/arm/mach-imx/cpu_op-mx51.c: In function 'mx51_get_cpu_op':
> arch/arm/mach-imx/cpu_op-mx51.c:27: error: implicit declaration of function 'BUILD_BUG_ON_ZERO'
>
> commit 6f863554 (kernel.h: doesn't explicitly use bug.h, so don't include it.)
> has removed asm/debug.h from linux/kernel.h.
>
> Quoting Russell King (http://www.spinics.net/lists/arm-kernel/msg161916.html)
>
> "linux/kernel.h _does_ use bug stuff - in ARRAY_SIZE().
>
> ARRAY_SIZE() uses __must_be_array(), which is defined in linux/compiler-gcc.h,
> which is obtained via linux/compiler.h and linux/linkage.h.
>
> linux/compiler-gcc.h defines __must_be_array() to be:
> #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>
> and BUILD_BUG_ON_ZERO used to be in linux/kernel.h but got moved to
> linux/bug.h.
>
> Hence why people are seeing build breakage with ARRAY_SIZE()."
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Thanks, but no.

You missed the whole point of my previous comments -- that being
that we don't want to just jam headers into always-used headers.

The compiler.h is going to be an "always used" header.  So if we
can avoid stuffing it full of extra crap, we need to really try.

Paul.

> ---
> Changes since v1:
> - Move linux/bug.h to linux/compiler.h
>
>  include/linux/compiler.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 4a24354..0300fb7 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -46,6 +46,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>
>  #ifdef __GNUC__
>  #include <linux/compiler-gcc.h>
> +#include <linux/bug.h>
>  #endif
>
>  #define notrace __attribute__((no_instrument_function))
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@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 v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Fabio Estevam]

On Thu, Mar 1, 2012 at 11:49 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:

> Thanks, but no.
>
> You missed the whole point of my previous comments -- that being
> that we don't want to just jam headers into always-used headers.

Yes, it is not clear for me how to fix this build error. I got
different feedbacks from you, Russell and Sam.

> The compiler.h is going to be an "always used" header.  So if we
> can avoid stuffing it full of extra crap, we need to really try.

So let me ask you: how do you want me to fix the build error I am seeing?

Re: [PATCH v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Paul Gortmaker]

On 12-03-01 10:13 PM, Fabio Estevam wrote:
> On Thu, Mar 1, 2012 at 11:49 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> 
>> Thanks, but no.
>>
>> You missed the whole point of my previous comments -- that being
>> that we don't want to just jam headers into always-used headers.
> 
> Yes, it is not clear for me how to fix this build error. I got
> different feedbacks from you, Russell and Sam.

Understood, there was some discussion there.  Anyways it is already
dealt with in yesterday's linux-next tree, so you won't have the
build failure anymore.

Thanks,
Paul.

> 
>> The compiler.h is going to be an "always used" header.  So if we
>> can avoid stuffing it full of extra crap, we need to really try.
> 
> So let me ask you: how do you want me to fix the build error I am seeing?

Re: [PATCH v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Uwe Kleine-König]

Hello,

On Fri, Mar 02, 2012 at 09:22:02AM -0500, Paul Gortmaker wrote:
> On 12-03-01 10:13 PM, Fabio Estevam wrote:
> > On Thu, Mar 1, 2012 at 11:49 PM, Paul Gortmaker
> > <paul.gortmaker@windriver.com> wrote:
> > 
> >> Thanks, but no.
> >>
> >> You missed the whole point of my previous comments -- that being
> >> that we don't want to just jam headers into always-used headers.
> > 
> > Yes, it is not clear for me how to fix this build error. I got
> > different feedbacks from you, Russell and Sam.
> 
> Understood, there was some discussion there.  Anyways it is already
> dealt with in yesterday's linux-next tree, so you won't have the
> build failure anymore.
I hit that same problem in an not yet mainlined source file. A simple
file containing only:

	#include <linux/kernel.h>

	int array[3];

	int func(void)
	{
		return ARRAY_SIZE(array);
	}

fails to build on top of v3.4-rc1. Am I right that you saying "you won't
have the build failure anymore" means that each of these is now needed
to be fixed individually by adding

	#include <linux/bug.h>

? Otherwise it's not fixed for me.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Paul Gortmaker]

On 12-04-04 10:29 AM, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Mar 02, 2012 at 09:22:02AM -0500, Paul Gortmaker wrote:
>> On 12-03-01 10:13 PM, Fabio Estevam wrote:
>>> On Thu, Mar 1, 2012 at 11:49 PM, Paul Gortmaker
>>> <paul.gortmaker@windriver.com> wrote:
>>>
>>>> Thanks, but no.
>>>>
>>>> You missed the whole point of my previous comments -- that being
>>>> that we don't want to just jam headers into always-used headers.
>>>
>>> Yes, it is not clear for me how to fix this build error. I got
>>> different feedbacks from you, Russell and Sam.
>>
>> Understood, there was some discussion there.  Anyways it is already
>> dealt with in yesterday's linux-next tree, so you won't have the
>> build failure anymore.
> I hit that same problem in an not yet mainlined source file. A simple
> file containing only:

A quick check shows about 15000 instances in mainline.  Even if 1% of
those were blowing up, I'd expect a full mailbox.

> 
> 	#include <linux/kernel.h>
> 
> 	int array[3];
> 
> 	int func(void)
> 	{
> 		return ARRAY_SIZE(array);

Well, ARRAY_SIZE is just a convenient macro that uses BUG content.
Hiding it behind a name doesn't change the fact that you've
implicitly decided to use bug.h content.  Maybe you really don't
want to be using it.  Maybe we should have:

--------------------------------------
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -43,6 +43,7 @@
 #define PTR_ALIGN(p, a)                ((typeof(p))ALIGN((unsigned long)(p), (a)))
 #define IS_ALIGNED(x, a)               (((x) & ((typeof(x))(a) - 1)) == 0)
 
+#define __ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
 /*
------------------------------------

and give people a choice?

Don't get me wrong.  I'm not all extremist about this.  If it turns
out that it seems to cause way too much grief, and someone like Andrew
says "Yeah, lets put #include <linux/bug.h> back in kernel.h" then I
won't hesitate to do that.  But given that code currently in mainline
isn't blowing up all over, I wasn't yet convinced we needed to do that.

Paul.

> 	}
> 
> fails to build on top of v3.4-rc1. Am I right that you saying "you won't
> have the build failure anymore" means that each of these is now needed
> to be fixed individually by adding
> 
> 	#include <linux/bug.h>
> 
> ? Otherwise it's not fixed for me.
> 
> Thanks
> Uwe
> 

Re: [PATCH v2] compiler.h: Include <linux/bug.h> to avoid build breakage with ARRAY_SIZE()

[Uwe Kleine-König]

On Thu, Apr 05, 2012 at 02:35:55PM -0400, Paul Gortmaker wrote:
> On 12-04-04 10:29 AM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Mar 02, 2012 at 09:22:02AM -0500, Paul Gortmaker wrote:
> >> On 12-03-01 10:13 PM, Fabio Estevam wrote:
> >>> On Thu, Mar 1, 2012 at 11:49 PM, Paul Gortmaker
> >>> <paul.gortmaker@windriver.com> wrote:
> >>>
> >>>> Thanks, but no.
> >>>>
> >>>> You missed the whole point of my previous comments -- that being
> >>>> that we don't want to just jam headers into always-used headers.
> >>>
> >>> Yes, it is not clear for me how to fix this build error. I got
> >>> different feedbacks from you, Russell and Sam.
> >>
> >> Understood, there was some discussion there.  Anyways it is already
> >> dealt with in yesterday's linux-next tree, so you won't have the
> >> build failure anymore.
> > I hit that same problem in an not yet mainlined source file. A simple
> > file containing only:
> 
> A quick check shows about 15000 instances in mainline.  Even if 1% of
> those were blowing up, I'd expect a full mailbox.
> 
> > 
> > 	#include <linux/kernel.h>
> > 
> > 	int array[3];
> > 
> > 	int func(void)
> > 	{
> > 		return ARRAY_SIZE(array);
> 
> Well, ARRAY_SIZE is just a convenient macro that uses BUG content.
> Hiding it behind a name doesn't change the fact that you've
> implicitly decided to use bug.h content.  Maybe you really don't
> want to be using it.  Maybe we should have:
> 
> --------------------------------------
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -43,6 +43,7 @@
>  #define PTR_ALIGN(p, a)                ((typeof(p))ALIGN((unsigned long)(p), (a)))
>  #define IS_ALIGNED(x, a)               (((x) & ((typeof(x))(a) - 1)) == 0)
>  
> +#define __ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>  
>  /*
> ------------------------------------
> 
> and give people a choice?
> 
> Don't get me wrong.  I'm not all extremist about this.  If it turns
> out that it seems to cause way too much grief, and someone like Andrew
> says "Yeah, lets put #include <linux/bug.h> back in kernel.h" then I
> won't hesitate to do that.  But given that code currently in mainline
> isn't blowing up all over, I wasn't yet convinced we needed to do that.
I didn't intend to readd bug.h to kernel.h. Just wanted to know if
adding #include <linux/bug.h> to my source file is the right thing to
do after your change.

Another alternative is to put ARRAY_SIZE into it's own header and let
that include <linux/bug.h>. Not sure this is a sane approach though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |