Date: Mon, 17 Oct 2016 09:04:37 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Vegard Nossum <vegard.nossum@oracle.com>
Cc: linux-kernel@vger.kernel.org, Jiri Slaby <jslaby@suse.cz>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        "Luis R . Rodriguez" <mcgrof@kernel.org>, stable@vger.kernel.org,
        Ming Lei <ming.lei@canonical.com>,
        Steven Rostedt <srostedt@redhat.com>
Subject: Re: [PATCH 01/12] extarray: define helpers for arrays defined in
 linker scripts
Message-ID: <20161017070437.GB25077@kroah.com>
References: <20161016151616.31451-1-vegard.nossum@oracle.com>
 <20161016151616.31451-2-vegard.nossum@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161016151616.31451-2-vegard.nossum@oracle.com>
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4456
Lines: 122

On Sun, Oct 16, 2016 at 05:16:05PM +0200, Vegard Nossum wrote:
> The test in this loop:
> 
> 	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
> 
> was getting completely compiled out by my gcc, 7.0.0 20160520. The result
> was that the loop was going beyond the end of the builtin_fw array and
> giving me a page fault when trying to dereference b_fw->name.
> 
> This is because __start_builtin_fw and __end_builtin_fw are both declared
> as (separate) arrays, and so gcc conludes that b_fw can never point to
> __end_builtin_fw.
> 
> Apparently similar optimizations were observed on NetBSD for GCC 5.4:
> http://mail-index.netbsd.org/netbsd-bugs/2016/06/22/msg047136.html
> 
> We can lose the array information about a pointer using
> OPTIMIZER_HIDE_VAR().
> 
> Additional points on the design of this interface:
> 
> 1) DECLARE_*() follows the kernel convention (you get what you expect,
>    more or less)
> 
> 2) the real variables defined in the linker script are hidden behind
>    some generated names so you don't use them by accident
> 
> 3) no need to sprinkle your code with OPTIMIZER_HIDE_VAR() or anything
>    else, but you do need to use function calls to access the variables
>    (e.g. ext_start(builtin_fw) and ext_end(builtin_fw)).
> 
> Reported-by: Jiri Slaby <jslaby@suse.cz>
> Cc: stable@vger.kernel.org
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Steven Rostedt <srostedt@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  include/linux/extarray.h | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 include/linux/extarray.h
> 
> diff --git a/include/linux/extarray.h b/include/linux/extarray.h
> new file mode 100644
> index 0000000..1185abc
> --- /dev/null
> +++ b/include/linux/extarray.h
> @@ -0,0 +1,65 @@
> +#ifndef LINUX_EXTARRAY_H
> +#define LINUX_EXTARRAY_H
> +
> +#include <linux/compiler.h>
> +
> +/*
> + * A common pattern in the kernel is to put certain objects in a specific
> + * named section and then create variables in the linker script pointing
> + * to the start and the end of this section. These variables are declared
> + * as extern arrays to allow C code to iterate over the list of objects.
> + *
> + * In C, comparing pointers to objects in two different arrays is undefined.
> + * GCC version 7.0 and newer (commit 73447cc5d17) will aggressively optimize
> + * out such comparisons if it can prove that the two pointers point to
> + * different arrays (which is the case when the arrays are declared as two
> + * separate variables). This breaks the typical code used to iterate over
> + * such arrays.
> + *
> + * One way to get around this limitation is to force GCC to lose any array
> + * information about the pointers before we compare them. We can use e.g.
> + * OPTIMIZER_HIDE_VAR() for this.
> + *
> + * This file defines a few helpers to deal with declaring and accessing
> + * such linker-script-defined arrays.
> + */
> +
> +
> +#define DECLARE_EXTARRAY(type, name)					\
> +	extern type __start_##name[];					\
> +	extern type __stop_##name[];					\

"EXTARRAY" kind of gives a good hint as to what is going on, but why not
just spell the thing out, "DECLARE_EXTERNAL_ARRAY()"?

> +#define _ext_start(name, tmp) \
> +	({								\
> +		typeof(*__start_##name) *tmp = __start_##name;		\
> +		OPTIMIZER_HIDE_VAR(tmp);				\
> +		tmp;							\
> +	})
> +
> +#define ext_start(name) _ext_start(name, __UNIQUE_ID(ext_start_))
> +
> +#define _ext_end(name, tmp)						\
> +	({								\
> +		typeof(*__stop_##name) *tmp = __stop_##name;		\
> +		OPTIMIZER_HIDE_VAR(tmp);				\
> +		tmp;							\
> +	})
> +
> +#define ext_end(name) _ext_end(name, __UNIQUE_ID(ext_end_))
> +
> +#define _ext_size(name, tmp1, tmp2)					\
> +	({								\
> +		typeof(*__start_##name) *tmp1 = __start_##name;		\
> +		typeof(*__stop_##name) *tmp2 = __stop_##name;		\
> +		OPTIMIZER_HIDE_VAR(tmp1);				\
> +		OPTIMIZER_HIDE_VAR(tmp2);				\
> +		tmp2 - tmp1;						\
> +	})
> +
> +#define ext_size(name) \
> +	_ext_size(name, __UNIQUE_ID(ext_size1_), __UNIQUE_ID(ext_size2_))
> +
> +#define ext_for_each(var, name) \
> +	for (var = ext_start(name); var != ext_end(name); var++)

"ext_" is also vague, and hard to know what this is at first glance when
reading code.  Again, we have lots of characters, let's use them.
"external_array_for_each()"?

thanks,

greg k-h