[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Stephen Boyd]

Sparse complains a bit on this file about endian issues and
__user casting:

arch/arm64/kernel/traps.c:87:37: warning: incorrect type in argument 1 (different address spaces)
arch/arm64/kernel/traps.c:87:37:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm64/kernel/traps.c:87:37:    got unsigned long *<noident>
arch/arm64/kernel/traps.c:116:23: warning: incorrect type in argument 1 (different address spaces)
arch/arm64/kernel/traps.c:116:23:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm64/kernel/traps.c:116:23:    got unsigned int [usertype] *
arch/arm64/kernel/traps.c:346:25: warning: cast to restricted __le16
arch/arm64/kernel/traps.c:352:34: warning: cast to restricted __le16
arch/arm64/kernel/traps.c:359:25: warning: cast to restricted __le32

Mark the types appropriately, and force the cast in get_user()
when assigning to 0 so sparse doesn't complain. The resulting
object code is the same before and after this commit.

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---

Noticed while making other changes to this file. There are other issues still
about marking symbols static, but I'm not sure we want to introduce another
header file for the asmlinkage functions?

arch/arm64/kernel/traps.c:429:29: warning: symbol 'do_undefinstr' was not declared. Should it be static?
arch/arm64/kernel/traps.c:529:29: warning: symbol 'do_sysinstr' was not declared. Should it be static?
arch/arm64/kernel/traps.c:544:17: warning: symbol 'do_ni_syscall' was not declared. Should it be static?
arch/arm64/kernel/traps.c:615:17: warning: symbol 'bad_mode' was not declared. Should it be static?
arch/arm64/kernel/traps.c:632:17: warning: symbol 'bad_el0_sync' was not declared. Should it be static?
arch/arm64/kernel/traps.c:722:12: warning: symbol 'early_brk64' was not declared. Should it be static?
arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
arch/arm64/kernel/traps.c:568:10:   also defined here

 arch/arm64/include/asm/uaccess.h |  2 +-
 arch/arm64/kernel/traps.c        | 23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 46da3ea638bb..2f5b4ae98ee0 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -287,7 +287,7 @@ do {									\
 	might_fault();							\
 	access_ok(VERIFY_READ, __p, sizeof(*__p)) ?			\
 		__get_user((x), __p) :					\
-		((x) = 0, -EFAULT);					\
+		((x) = (__force __typeof__(*(ptr)))0, -EFAULT);		\
 })
 
 #define __put_user_asm(instr, alt_instr, reg, x, addr, err, feature)	\
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 659b2e6b6cf7..23959cb70ded 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 			if (p >= bottom && p < top) {
 				unsigned long val;
 
-				if (__get_user(val, (unsigned long *)p) == 0)
+				if (__get_user(val, (unsigned long __user *)p) == 0)
 					sprintf(str + i * 17, " %016lx", val);
 				else
 					sprintf(str + i * 17, " ????????????????");
@@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
 	for (i = -4; i < 1; i++) {
 		unsigned int val, bad;
 
-		bad = __get_user(val, &((u32 *)addr)[i]);
+		bad = __get_user(val, &((u32 __user *)addr)[i]);
 
 		if (!bad)
 			p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
@@ -340,23 +340,28 @@ static int call_undef_hook(struct pt_regs *regs)
 		return 1;
 
 	if (compat_thumb_mode(regs)) {
+		__le16 tinst;
+
 		/* 16-bit Thumb instruction */
-		if (get_user(instr, (u16 __user *)pc))
+		if (get_user(tinst, (__le16 __user *)pc))
 			goto exit;
-		instr = le16_to_cpu(instr);
+		instr = le16_to_cpu(tinst);
 		if (aarch32_insn_is_wide(instr)) {
-			u32 instr2;
+			__le16 tinstr2;
+			u16 instr2;
 
-			if (get_user(instr2, (u16 __user *)(pc + 2)))
+			if (get_user(tinstr2, (__le16 __user *)(pc + 2)))
 				goto exit;
-			instr2 = le16_to_cpu(instr2);
+			instr2 = le16_to_cpu(tinstr2);
 			instr = (instr << 16) | instr2;
 		}
 	} else {
+		__le32 ainst;
+
 		/* 32-bit ARM instruction */
-		if (get_user(instr, (u32 __user *)pc))
+		if (get_user(ainst, (__le32 __user *)pc))
 			goto exit;
-		instr = le32_to_cpu(instr);
+		instr = le32_to_cpu(ainst);
 	}
 
 	raw_spin_lock_irqsave(&undef_lock, flags);
-- 
2.10.0.297.gf6727b0

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Stephen Boyd]

Sparse complains a bit on this file about endian issues and
__user casting:

arch/arm64/kernel/traps.c:87:37: warning: incorrect type in argument 1 (different address spaces)
arch/arm64/kernel/traps.c:87:37:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm64/kernel/traps.c:87:37:    got unsigned long *<noident>
arch/arm64/kernel/traps.c:116:23: warning: incorrect type in argument 1 (different address spaces)
arch/arm64/kernel/traps.c:116:23:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm64/kernel/traps.c:116:23:    got unsigned int [usertype] *
arch/arm64/kernel/traps.c:346:25: warning: cast to restricted __le16
arch/arm64/kernel/traps.c:352:34: warning: cast to restricted __le16
arch/arm64/kernel/traps.c:359:25: warning: cast to restricted __le32

Mark the types appropriately, and force the cast in get_user()
when assigning to 0 so sparse doesn't complain. The resulting
object code is the same before and after this commit.

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---

Noticed while making other changes to this file. There are other issues still
about marking symbols static, but I'm not sure we want to introduce another
header file for the asmlinkage functions?

arch/arm64/kernel/traps.c:429:29: warning: symbol 'do_undefinstr' was not declared. Should it be static?
arch/arm64/kernel/traps.c:529:29: warning: symbol 'do_sysinstr' was not declared. Should it be static?
arch/arm64/kernel/traps.c:544:17: warning: symbol 'do_ni_syscall' was not declared. Should it be static?
arch/arm64/kernel/traps.c:615:17: warning: symbol 'bad_mode' was not declared. Should it be static?
arch/arm64/kernel/traps.c:632:17: warning: symbol 'bad_el0_sync' was not declared. Should it be static?
arch/arm64/kernel/traps.c:722:12: warning: symbol 'early_brk64' was not declared. Should it be static?
arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
arch/arm64/kernel/traps.c:568:10:   also defined here

 arch/arm64/include/asm/uaccess.h |  2 +-
 arch/arm64/kernel/traps.c        | 23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 46da3ea638bb..2f5b4ae98ee0 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -287,7 +287,7 @@ do {									\
 	might_fault();							\
 	access_ok(VERIFY_READ, __p, sizeof(*__p)) ?			\
 		__get_user((x), __p) :					\
-		((x) = 0, -EFAULT);					\
+		((x) = (__force __typeof__(*(ptr)))0, -EFAULT);		\
 })
 
 #define __put_user_asm(instr, alt_instr, reg, x, addr, err, feature)	\
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 659b2e6b6cf7..23959cb70ded 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 			if (p >= bottom && p < top) {
 				unsigned long val;
 
-				if (__get_user(val, (unsigned long *)p) == 0)
+				if (__get_user(val, (unsigned long __user *)p) == 0)
 					sprintf(str + i * 17, " %016lx", val);
 				else
 					sprintf(str + i * 17, " ????????????????");
@@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
 	for (i = -4; i < 1; i++) {
 		unsigned int val, bad;
 
-		bad = __get_user(val, &((u32 *)addr)[i]);
+		bad = __get_user(val, &((u32 __user *)addr)[i]);
 
 		if (!bad)
 			p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
@@ -340,23 +340,28 @@ static int call_undef_hook(struct pt_regs *regs)
 		return 1;
 
 	if (compat_thumb_mode(regs)) {
+		__le16 tinst;
+
 		/* 16-bit Thumb instruction */
-		if (get_user(instr, (u16 __user *)pc))
+		if (get_user(tinst, (__le16 __user *)pc))
 			goto exit;
-		instr = le16_to_cpu(instr);
+		instr = le16_to_cpu(tinst);
 		if (aarch32_insn_is_wide(instr)) {
-			u32 instr2;
+			__le16 tinstr2;
+			u16 instr2;
 
-			if (get_user(instr2, (u16 __user *)(pc + 2)))
+			if (get_user(tinstr2, (__le16 __user *)(pc + 2)))
 				goto exit;
-			instr2 = le16_to_cpu(instr2);
+			instr2 = le16_to_cpu(tinstr2);
 			instr = (instr << 16) | instr2;
 		}
 	} else {
+		__le32 ainst;
+
 		/* 32-bit ARM instruction */
-		if (get_user(instr, (u32 __user *)pc))
+		if (get_user(ainst, (__le32 __user *)pc))
 			goto exit;
-		instr = le32_to_cpu(instr);
+		instr = le32_to_cpu(ainst);
 	}
 
 	raw_spin_lock_irqsave(&undef_lock, flags);
-- 
2.10.0.297.gf6727b0

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 659b2e6b6cf7..23959cb70ded 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
>  			if (p >= bottom && p < top) {
>  				unsigned long val;
>  
> -				if (__get_user(val, (unsigned long *)p) == 0)
> +				if (__get_user(val, (unsigned long __user *)p) == 0)
>  					sprintf(str + i * 17, " %016lx", val);
>  				else
>  					sprintf(str + i * 17, " ????????????????");
> @@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
>  	for (i = -4; i < 1; i++) {
>  		unsigned int val, bad;
>  
> -		bad = __get_user(val, &((u32 *)addr)[i]);
> +		bad = __get_user(val, &((u32 __user *)addr)[i]);
>  
>  		if (!bad)
>  			p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
> @@ -340,23 +340,28 @@ static int call_undef_hook(struct pt_regs *regs)
>  		return 1;
>  
>  	if (compat_thumb_mode(regs)) {
> +		__le16 tinst;
> +
>  		/* 16-bit Thumb instruction */
> -		if (get_user(instr, (u16 __user *)pc))
> +		if (get_user(tinst, (__le16 __user *)pc))
>  			goto exit;
> -		instr = le16_to_cpu(instr);
> +		instr = le16_to_cpu(tinst);
>  		if (aarch32_insn_is_wide(instr)) {
> -			u32 instr2;
> +			__le16 tinstr2;
> +			u16 instr2;
>  
> -			if (get_user(instr2, (u16 __user *)(pc + 2)))
> +			if (get_user(tinstr2, (__le16 __user *)(pc + 2)))
>  				goto exit;
> -			instr2 = le16_to_cpu(instr2);
> +			instr2 = le16_to_cpu(tinstr2);
>  			instr = (instr << 16) | instr2;
>  		}
>  	} else {
> +		__le32 ainst;
> +
>  		/* 32-bit ARM instruction */
> -		if (get_user(instr, (u32 __user *)pc))
> +		if (get_user(ainst, (__le32 __user *)pc))
>  			goto exit;
> -		instr = le32_to_cpu(instr);
> +		instr = le32_to_cpu(ainst);

For the majority of causes, these are _not_ user addresses, they are
kernel addresses.  The use of get_user() at these locations is a way
to safely access these kernel addresses when something has gone wrong
without causing a further oops.

Annotating them with __user to shut up sparse is incorrect.  The point
with sparse is _not_ to end up with a warning free kernel, but for it
to warn where things are not quite right in terms of semantics.  These
warnings should stay.

So, the warnings about lack of __user should stay.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 659b2e6b6cf7..23959cb70ded 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
>  			if (p >= bottom && p < top) {
>  				unsigned long val;
>  
> -				if (__get_user(val, (unsigned long *)p) == 0)
> +				if (__get_user(val, (unsigned long __user *)p) == 0)
>  					sprintf(str + i * 17, " %016lx", val);
>  				else
>  					sprintf(str + i * 17, " ????????????????");
> @@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
>  	for (i = -4; i < 1; i++) {
>  		unsigned int val, bad;
>  
> -		bad = __get_user(val, &((u32 *)addr)[i]);
> +		bad = __get_user(val, &((u32 __user *)addr)[i]);
>  
>  		if (!bad)
>  			p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
> @@ -340,23 +340,28 @@ static int call_undef_hook(struct pt_regs *regs)
>  		return 1;
>  
>  	if (compat_thumb_mode(regs)) {
> +		__le16 tinst;
> +
>  		/* 16-bit Thumb instruction */
> -		if (get_user(instr, (u16 __user *)pc))
> +		if (get_user(tinst, (__le16 __user *)pc))
>  			goto exit;
> -		instr = le16_to_cpu(instr);
> +		instr = le16_to_cpu(tinst);
>  		if (aarch32_insn_is_wide(instr)) {
> -			u32 instr2;
> +			__le16 tinstr2;
> +			u16 instr2;
>  
> -			if (get_user(instr2, (u16 __user *)(pc + 2)))
> +			if (get_user(tinstr2, (__le16 __user *)(pc + 2)))
>  				goto exit;
> -			instr2 = le16_to_cpu(instr2);
> +			instr2 = le16_to_cpu(tinstr2);
>  			instr = (instr << 16) | instr2;
>  		}
>  	} else {
> +		__le32 ainst;
> +
>  		/* 32-bit ARM instruction */
> -		if (get_user(instr, (u32 __user *)pc))
> +		if (get_user(ainst, (__le32 __user *)pc))
>  			goto exit;
> -		instr = le32_to_cpu(instr);
> +		instr = le32_to_cpu(ainst);

For the majority of causes, these are _not_ user addresses, they are
kernel addresses.  The use of get_user() at these locations is a way
to safely access these kernel addresses when something has gone wrong
without causing a further oops.

Annotating them with __user to shut up sparse is incorrect.  The point
with sparse is _not_ to end up with a warning free kernel, but for it
to warn where things are not quite right in terms of semantics.  These
warnings should stay.

So, the warnings about lack of __user should stay.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Stephen Boyd]

Quoting Russell King - ARM Linux (2017-02-17 09:31:01)
> On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 659b2e6b6cf7..23959cb70ded 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
> >                       if (p >= bottom && p < top) {
> >                               unsigned long val;
> >  
> > -                             if (__get_user(val, (unsigned long *)p) == 0)
> > +                             if (__get_user(val, (unsigned long __user *)p) == 0)
> >                                       sprintf(str + i * 17, " %016lx", val);
> >                               else
> >                                       sprintf(str + i * 17, " ????????????????");
> > @@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
> >       for (i = -4; i < 1; i++) {
> >               unsigned int val, bad;
> >  
> > -             bad = __get_user(val, &((u32 *)addr)[i]);
> > +             bad = __get_user(val, &((u32 __user *)addr)[i]);
> >  
> >               if (!bad)
> >                       p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
> 
> For the majority of causes, these are _not_ user addresses, they are
> kernel addresses.  The use of get_user() at these locations is a way
> to safely access these kernel addresses when something has gone wrong
> without causing a further oops.

Understood. It looks like a custom version of probe_kernel_read() (where
we force the pointer to __user BTW). Is that to only set_fs() once
instead of many times? Maybe we need a ____probe_kernel_read() that
forces it to __user under the covers and doesn't do any set_fs() and
also indicates we know what we're doing?

> 
> Annotating them with __user to shut up sparse is incorrect.  The point
> with sparse is _not_ to end up with a warning free kernel, but for it
> to warn where things are not quite right in terms of semantics.  These
> warnings should stay.
> 
> So, the warnings about lack of __user should stay.
> 

Ok. I usually compile things with 'make -s' and C=2 and then if sparse
complains I want to keep it quiet so I don't have to worry about looking
through the warnings. So in my workflow I _do_ want a warning free
kernel.

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> Sparse complains a bit on this file about endian issues and
> __user casting:
> 
> arch/arm64/kernel/traps.c:87:37: warning: incorrect type in argument 1 (different address spaces)
> arch/arm64/kernel/traps.c:87:37:    expected void const volatile [noderef] <asn:1>*<noident>
> arch/arm64/kernel/traps.c:87:37:    got unsigned long *<noident>
> arch/arm64/kernel/traps.c:116:23: warning: incorrect type in argument 1 (different address spaces)
> arch/arm64/kernel/traps.c:116:23:    expected void const volatile [noderef] <asn:1>*<noident>
> arch/arm64/kernel/traps.c:116:23:    got unsigned int [usertype] *

The fact that __get_user() can and is used for both __kernel & __user pointers
defeat any sensible annotation. The proper way would be to have a special
version of __get_user() which would ignore the __user part of the pointer,
something like "__get_user_but_accept_any_pointer()" ...


> arch/arm64/kernel/traps.c:346:25: warning: cast to restricted __le16
> arch/arm64/kernel/traps.c:352:34: warning: cast to restricted __le16
> arch/arm64/kernel/traps.c:359:25: warning: cast to restricted __le32

Your patch looked correct to me for those.
 
> Mark the types appropriately, and force the cast in get_user()
> when assigning to 0 so sparse doesn't complain.
I didn't looked deeply at this one but I don't think it is needed.
Care to give more details?


> Noticed while making other changes to this file. There are other issues still
> about marking symbols static, but I'm not sure we want to introduce another
> header file for the asmlinkage functions?
Probably not, indeed.

> arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> arch/arm64/kernel/traps.c:568:10:   also defined here
This one I find strange. Can you tell which are those two entries?

 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 46da3ea638bb..2f5b4ae98ee0 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -287,7 +287,7 @@ do {									\
>  	might_fault();							\
>  	access_ok(VERIFY_READ, __p, sizeof(*__p)) ?			\
>  		__get_user((x), __p) :					\
> -		((x) = 0, -EFAULT);					\
> +		((x) = (__force __typeof__(*(ptr)))0, -EFAULT);		\
>  })

As said above, this one is dubious.


Luc Van Oostenryck

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> Sparse complains a bit on this file about endian issues and
> __user casting:
> 
> arch/arm64/kernel/traps.c:87:37: warning: incorrect type in argument 1 (different address spaces)
> arch/arm64/kernel/traps.c:87:37:    expected void const volatile [noderef] <asn:1>*<noident>
> arch/arm64/kernel/traps.c:87:37:    got unsigned long *<noident>
> arch/arm64/kernel/traps.c:116:23: warning: incorrect type in argument 1 (different address spaces)
> arch/arm64/kernel/traps.c:116:23:    expected void const volatile [noderef] <asn:1>*<noident>
> arch/arm64/kernel/traps.c:116:23:    got unsigned int [usertype] *

The fact that __get_user() can and is used for both __kernel & __user pointers
defeat any sensible annotation. The proper way would be to have a special
version of __get_user() which would ignore the __user part of the pointer,
something like "__get_user_but_accept_any_pointer()" ...


> arch/arm64/kernel/traps.c:346:25: warning: cast to restricted __le16
> arch/arm64/kernel/traps.c:352:34: warning: cast to restricted __le16
> arch/arm64/kernel/traps.c:359:25: warning: cast to restricted __le32

Your patch looked correct to me for those.
 
> Mark the types appropriately, and force the cast in get_user()
> when assigning to 0 so sparse doesn't complain.
I didn't looked deeply at this one but I don't think it is needed.
Care to give more details?


> Noticed while making other changes to this file. There are other issues still
> about marking symbols static, but I'm not sure we want to introduce another
> header file for the asmlinkage functions?
Probably not, indeed.

> arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> arch/arm64/kernel/traps.c:568:10:   also defined here
This one I find strange. Can you tell which are those two entries?

 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 46da3ea638bb..2f5b4ae98ee0 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -287,7 +287,7 @@ do {									\
>  	might_fault();							\
>  	access_ok(VERIFY_READ, __p, sizeof(*__p)) ?			\
>  		__get_user((x), __p) :					\
> -		((x) = 0, -EFAULT);					\
> +		((x) = (__force __typeof__(*(ptr)))0, -EFAULT);		\
>  })

As said above, this one is dubious.


Luc Van Oostenryck

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Stephen Boyd]

Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
>  
> > Mark the types appropriately, and force the cast in get_user()
> > when assigning to 0 so sparse doesn't complain.
> I didn't looked deeply at this one but I don't think it is needed.
> Care to give more details?

I think my sparse is old. I was using v0.5.0 from my distro, but using
sparse-next branch from git.kernel.org doesn't show the problem anymore.
I suppose I'll upgrade this machine's sparse version manually to avoid
this problem in the future and withdraw my other patch to asm-generic.

> 
> 
> > Noticed while making other changes to this file. There are other issues still
> > about marking symbols static, but I'm not sure we want to introduce another
> > header file for the asmlinkage functions?
> Probably not, indeed.
> 
> > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > arch/arm64/kernel/traps.c:568:10:   also defined here
> This one I find strange. Can you tell which are those two entries?
> 

This is:

 static const char *esr_class_str[] = {
         [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
         [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",

where we initialize the entire array to an "unknown" value once, and
then fill in the known values after that. This isn't a very common
pattern, but it is used from time to time to avoid having lots of lines
to do the same thing.

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Mon, Feb 20, 2017 at 9:47 AM, Stephen Boyd <stephen.boyd@linaro.org> wrote:
> Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
>> On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
>> > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
>> > arch/arm64/kernel/traps.c:568:10:   also defined here
>> This one I find strange. Can you tell which are those two entries?
>>
>
> This is:
>
>  static const char *esr_class_str[] = {
>          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
>          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
>
> where we initialize the entire array to an "unknown" value once, and
> then fill in the known values after that. This isn't a very common
> pattern, but it is used from time to time to avoid having lots of lines
> to do the same thing.

I see, yes can be handy but indeed spaese doesn't know about it yet.

Luc

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Mon, Feb 20, 2017 at 9:47 AM, Stephen Boyd <stephen.boyd@linaro.org> wrote:
> Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
>> On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
>> > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
>> > arch/arm64/kernel/traps.c:568:10:   also defined here
>> This one I find strange. Can you tell which are those two entries?
>>
>
> This is:
>
>  static const char *esr_class_str[] = {
>          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
>          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
>
> where we initialize the entire array to an "unknown" value once, and
> then fill in the known values after that. This isn't a very common
> pattern, but it is used from time to time to avoid having lots of lines
> to do the same thing.

I see, yes can be handy but indeed spaese doesn't know about it yet.

Luc

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Mark Rutland]

On Mon, Feb 20, 2017 at 12:47:57AM -0800, Stephen Boyd wrote:
> Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> > On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> >  
> > > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > > arch/arm64/kernel/traps.c:568:10:   also defined here
> > This one I find strange. Can you tell which are those two entries?
> 
> This is:
> 
>  static const char *esr_class_str[] = {
>          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
>          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
> 
> where we initialize the entire array to an "unknown" value once, and
> then fill in the known values after that. This isn't a very common
> pattern, but it is used from time to time to avoid having lots of lines
> to do the same thing.

FWIW, it's a fairly common trick for syscall tables, which is where I
copied it from for the above. Certainly it's not that common elsewhere.

[mark@leverpostej:~/src/linux]% tail -n 11  arch/arm64/kernel/sys.c
#undef __SYSCALL
#define __SYSCALL(nr, sym)      [nr] = sym,

/*
 * The sys_call_table array must be 4K aligned to be accessible from
 * kernel/entry.S.
 */
void * const sys_call_table[__NR_syscalls] __aligned(4096) = {
        [0 ... __NR_syscalls - 1] = sys_ni_syscall,
#include <asm/unistd.h>
};

[mark@leverpostej:~/src/linux]% git grep '\[0 \.\.\. ' | grep sys
arch/arc/kernel/sys.c:  [0 ... NR_syscalls-1] = sys_ni_syscall,
arch/arm64/kernel/sys.c:        [0 ... __NR_syscalls - 1] = sys_ni_syscall,
arch/arm64/kernel/sys32.c:      [0 ... __NR_compat_syscalls - 1] = sys_ni_syscall,
arch/c6x/kernel/sys_c6x.c:      [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/metag/kernel/sys_metag.c:  [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/tile/kernel/compat.c:      [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/tile/kernel/sys.c: [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/unicore32/kernel/sys.c:    [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/x86/entry/syscall_32.c:    [0 ... __NR_syscall_compat_max] = &sys_ni_syscall,
arch/x86/entry/syscall_64.c:    [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/x86/um/sys_call_table_32.c:        [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/x86/um/sys_call_table_64.c:        [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/xtensa/kernel/syscall.c:   [0 ... __NR_syscall_count - 1] = (syscall_t)&sys_ni_syscall,

It would be nice to make sparse aware of this somehow, even if it
requires some annotation.

Thanks,
Mark.

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Mark Rutland]

On Mon, Feb 20, 2017 at 12:47:57AM -0800, Stephen Boyd wrote:
> Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> > On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> >  
> > > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > > arch/arm64/kernel/traps.c:568:10:   also defined here
> > This one I find strange. Can you tell which are those two entries?
> 
> This is:
> 
>  static const char *esr_class_str[] = {
>          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
>          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
> 
> where we initialize the entire array to an "unknown" value once, and
> then fill in the known values after that. This isn't a very common
> pattern, but it is used from time to time to avoid having lots of lines
> to do the same thing.

FWIW, it's a fairly common trick for syscall tables, which is where I
copied it from for the above. Certainly it's not that common elsewhere.

[mark at leverpostej:~/src/linux]% tail -n 11  arch/arm64/kernel/sys.c
#undef __SYSCALL
#define __SYSCALL(nr, sym)      [nr] = sym,

/*
 * The sys_call_table array must be 4K aligned to be accessible from
 * kernel/entry.S.
 */
void * const sys_call_table[__NR_syscalls] __aligned(4096) = {
        [0 ... __NR_syscalls - 1] = sys_ni_syscall,
#include <asm/unistd.h>
};

[mark at leverpostej:~/src/linux]% git grep '\[0 \.\.\. ' | grep sys
arch/arc/kernel/sys.c:  [0 ... NR_syscalls-1] = sys_ni_syscall,
arch/arm64/kernel/sys.c:        [0 ... __NR_syscalls - 1] = sys_ni_syscall,
arch/arm64/kernel/sys32.c:      [0 ... __NR_compat_syscalls - 1] = sys_ni_syscall,
arch/c6x/kernel/sys_c6x.c:      [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/metag/kernel/sys_metag.c:  [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/tile/kernel/compat.c:      [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/tile/kernel/sys.c: [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/unicore32/kernel/sys.c:    [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/x86/entry/syscall_32.c:    [0 ... __NR_syscall_compat_max] = &sys_ni_syscall,
arch/x86/entry/syscall_64.c:    [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/x86/um/sys_call_table_32.c:        [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/x86/um/sys_call_table_64.c:        [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/xtensa/kernel/syscall.c:   [0 ... __NR_syscall_count - 1] = (syscall_t)&sys_ni_syscall,

It would be nice to make sparse aware of this somehow, even if it
requires some annotation.

Thanks,
Mark.

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Mon, Feb 20, 2017 at 10:49:47AM +0000, Mark Rutland wrote:
> On Mon, Feb 20, 2017 at 12:47:57AM -0800, Stephen Boyd wrote:
> > Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> > > On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> > >  
> > > > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > > > arch/arm64/kernel/traps.c:568:10:   also defined here
> > > This one I find strange. Can you tell which are those two entries?
> > 
> > This is:
> > 
> >  static const char *esr_class_str[] = {
> >          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
> >          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
> > 
> > where we initialize the entire array to an "unknown" value once, and
> > then fill in the known values after that. This isn't a very common
> > pattern, but it is used from time to time to avoid having lots of lines
> > to do the same thing.
> 
> FWIW, it's a fairly common trick for syscall tables, which is where I
> copied it from for the above. Certainly it's not that common elsewhere.
> 
> ...
> 
> It would be nice to make sparse aware of this somehow, even if it
> requires some annotation.
> 
> Thanks,
> Mark.

I just checked this and I'm not very sure what's best.
Sparse is very well aware of the '...' to specify a range
in an array initializer or in switch-case. The warning
is there only because those entries are later overridden
with some value.
When checking what GCC is doing in this situation is saw
that by default even in cases like:
	static in ref[] = {
		[1] = 1,
		[2] = 2,
	};
GCC doesn't issue a warning. You need to use the flag
-Woverride-init for that. But then you also have a warning
for our current case:
	static in foo[] = {
		[0 ... 3] = 1,
		[0] = 2,
	};

It's easy enough to patch sparse to not issue a warning when the
override concerns a range (which would be perfect for the situation here),
Controlled or not by a new warning flag. But I'm far from convinced
that all uses of such "ranged-initialization" is used for default values
that may be later overridden.

I'm also reluctant to introduce yet another special annotation.

Any thoughts anyone about what we woudl like?


Note: sparse is quite incomplete regarding these overridden entries
  since it issues a warning only when the override is on the first
  element of the range. In others words, the range itself is not
  taken in account. So, no warnigs for code like:
	static in foo[] = {
		[0 ... 3] = 1,
		[1] = 2,
	};


-- Luc Van Oostenryck

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Mon, Feb 20, 2017 at 10:49:47AM +0000, Mark Rutland wrote:
> On Mon, Feb 20, 2017 at 12:47:57AM -0800, Stephen Boyd wrote:
> > Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> > > On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> > >  
> > > > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > > > arch/arm64/kernel/traps.c:568:10:   also defined here
> > > This one I find strange. Can you tell which are those two entries?
> > 
> > This is:
> > 
> >  static const char *esr_class_str[] = {
> >          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
> >          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
> > 
> > where we initialize the entire array to an "unknown" value once, and
> > then fill in the known values after that. This isn't a very common
> > pattern, but it is used from time to time to avoid having lots of lines
> > to do the same thing.
> 
> FWIW, it's a fairly common trick for syscall tables, which is where I
> copied it from for the above. Certainly it's not that common elsewhere.
> 
> ...
> 
> It would be nice to make sparse aware of this somehow, even if it
> requires some annotation.
> 
> Thanks,
> Mark.

I just checked this and I'm not very sure what's best.
Sparse is very well aware of the '...' to specify a range
in an array initializer or in switch-case. The warning
is there only because those entries are later overridden
with some value.
When checking what GCC is doing in this situation is saw
that by default even in cases like:
	static in ref[] = {
		[1] = 1,
		[2] = 2,
	};
GCC doesn't issue a warning. You need to use the flag
-Woverride-init for that. But then you also have a warning
for our current case:
	static in foo[] = {
		[0 ... 3] = 1,
		[0] = 2,
	};

It's easy enough to patch sparse to not issue a warning when the
override concerns a range (which would be perfect for the situation here),
Controlled or not by a new warning flag. But I'm far from convinced
that all uses of such "ranged-initialization" is used for default values
that may be later overridden.

I'm also reluctant to introduce yet another special annotation.

Any thoughts anyone about what we woudl like?


Note: sparse is quite incomplete regarding these overridden entries
  since it issues a warning only when the override is on the first
  element of the range. In others words, the range itself is not
  taken in account. So, no warnigs for code like:
	static in foo[] = {
		[0 ... 3] = 1,
		[1] = 2,
	};


-- Luc Van Oostenryck

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Will Deacon]

On Mon, Feb 20, 2017 at 10:33:45PM +0100, Luc Van Oostenryck wrote:
> I just checked this and I'm not very sure what's best.
> Sparse is very well aware of the '...' to specify a range
> in an array initializer or in switch-case. The warning
> is there only because those entries are later overridden
> with some value.
> When checking what GCC is doing in this situation is saw
> that by default even in cases like:
> 	static in ref[] = {
> 		[1] = 1,
> 		[2] = 2,
> 	};
> GCC doesn't issue a warning. You need to use the flag
> -Woverride-init for that. But then you also have a warning
> for our current case:
> 	static in foo[] = {
> 		[0 ... 3] = 1,
> 		[0] = 2,
> 	};
> 
> It's easy enough to patch sparse to not issue a warning when the
> override concerns a range (which would be perfect for the situation here),
> Controlled or not by a new warning flag. But I'm far from convinced
> that all uses of such "ranged-initialization" is used for default values
> that may be later overridden.

How about not warning only when the overridden range covers the entire
length of the array? The only broken case I can think of that slips
through the cracks then is if somebody typoed the range so that it
accidentally covered the whole array and therefore suppressed the override
warning.

Will

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Will Deacon]

On Mon, Feb 20, 2017 at 10:33:45PM +0100, Luc Van Oostenryck wrote:
> I just checked this and I'm not very sure what's best.
> Sparse is very well aware of the '...' to specify a range
> in an array initializer or in switch-case. The warning
> is there only because those entries are later overridden
> with some value.
> When checking what GCC is doing in this situation is saw
> that by default even in cases like:
> 	static in ref[] = {
> 		[1] = 1,
> 		[2] = 2,
> 	};
> GCC doesn't issue a warning. You need to use the flag
> -Woverride-init for that. But then you also have a warning
> for our current case:
> 	static in foo[] = {
> 		[0 ... 3] = 1,
> 		[0] = 2,
> 	};
> 
> It's easy enough to patch sparse to not issue a warning when the
> override concerns a range (which would be perfect for the situation here),
> Controlled or not by a new warning flag. But I'm far from convinced
> that all uses of such "ranged-initialization" is used for default values
> that may be later overridden.

How about not warning only when the overridden range covers the entire
length of the array? The only broken case I can think of that slips
through the cracks then is if somebody typoed the range so that it
accidentally covered the whole array and therefore suppressed the override
warning.

Will

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Tue, Feb 21, 2017 at 11:03:56AM +0000, Will Deacon wrote:
> On Mon, Feb 20, 2017 at 10:33:45PM +0100, Luc Van Oostenryck wrote:
> > It's easy enough to patch sparse to not issue a warning when the
> > override concerns a range (which would be perfect for the situation here),
> > Controlled or not by a new warning flag. But I'm far from convinced
> > that all uses of such "ranged-initialization" is used for default values
> > that may be later overridden.
> 
> How about not warning only when the overridden range covers the entire
> length of the array? The only broken case I can think of that slips
> through the cracks then is if somebody typoed the range so that it
> accidentally covered the whole array and therefore suppressed the override
> warning.
> 
> Will

I like it. Patch is coming.

Luc

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Tue, Feb 21, 2017 at 11:03:56AM +0000, Will Deacon wrote:
> On Mon, Feb 20, 2017 at 10:33:45PM +0100, Luc Van Oostenryck wrote:
> > It's easy enough to patch sparse to not issue a warning when the
> > override concerns a range (which would be perfect for the situation here),
> > Controlled or not by a new warning flag. But I'm far from convinced
> > that all uses of such "ranged-initialization" is used for default values
> > that may be later overridden.
> 
> How about not warning only when the overridden range covers the entire
> length of the array? The only broken case I can think of that slips
> through the cracks then is if somebody typoed the range so that it
> accidentally covered the whole array and therefore suppressed the override
> warning.
> 
> Will

I like it. Patch is coming.

Luc

[PATCH 0/5] improve detection of overlapping initializers

[Luc Van Oostenryck]

This aims at improving the detection of overlapping initializers by
- introduce some warning flags for finer control
- fix the detection when the overlap is not on the first element
- ann an exception when the initializer cover the whole range.

For convenience, thsi series can also be found at:
	git://github.com/lucvoo/sparse.git sent/array-range-init


Luc Van Oostenryck (5):
  use option: '-Woverride-init'
  add test case for warnings about overlapping initializers
  allow to warn on all overlapping initializers
  fix checking of overlapping initializer
  ignore whole-range overlapping initializer

 expand.c                        |  44 +++++++++++++++--
 lib.c                           |   5 ++
 lib.h                           |   3 ++
 validation/Woverride-init-def.c |  14 ++++++
 validation/Woverride-init-no.c  |  12 +++++
 validation/Woverride-init-yes.c |  14 ++++++
 validation/field-override.c     | 101 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 188 insertions(+), 5 deletions(-)
 create mode 100644 validation/Woverride-init-def.c
 create mode 100644 validation/Woverride-init-no.c
 create mode 100644 validation/Woverride-init-yes.c
 create mode 100644 validation/field-override.c

-- 
2.11.1

[PATCH 1/5] use option: '-Woverride-init'

[Luc Van Oostenryck]

Sparse warns unconditionally about overlapping initilalizers.

Introduces a warning flag to control this, use the same name
as GCC's and enabled it by default.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 expand.c                        |  3 +++
 lib.c                           |  2 ++
 lib.h                           |  1 +
 validation/Woverride-init-def.c | 14 ++++++++++++++
 validation/Woverride-init-no.c  | 12 ++++++++++++
 validation/Woverride-init-yes.c | 14 ++++++++++++++
 6 files changed, 46 insertions(+)
 create mode 100644 validation/Woverride-init-def.c
 create mode 100644 validation/Woverride-init-no.c
 create mode 100644 validation/Woverride-init-yes.c

diff --git a/expand.c b/expand.c
index 7af12707e..7457c94dd 100644
--- a/expand.c
+++ b/expand.c
@@ -916,6 +916,9 @@ static void verify_nonoverlapping(struct expression_list **list)
 	struct expression *a = NULL;
 	struct expression *b;
 
+	if (!Woverride_init)
+		return;
+
 	FOR_EACH_PTR(*list, b) {
 		if (!b->ctype || !b->ctype->bit_size)
 			continue;
diff --git a/lib.c b/lib.c
index d47325243..a20f68aa2 100644
--- a/lib.c
+++ b/lib.c
@@ -231,6 +231,7 @@ int Wsparse_error = 0;
 int Wnon_pointer_null = 1;
 int Wold_initializer = 1;
 int Wone_bit_signed_bitfield = 1;
+int Woverride_init = 1;
 int Wparen_string = 0;
 int Wptr_subtraction_blows = 0;
 int Wreturn_void = 0;
@@ -480,6 +481,7 @@ static const struct warning {
 	{ "non-pointer-null", &Wnon_pointer_null },
 	{ "old-initializer", &Wold_initializer },
 	{ "one-bit-signed-bitfield", &Wone_bit_signed_bitfield },
+	{ "override-init", &Woverride_init },
 	{ "paren-string", &Wparen_string },
 	{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
 	{ "return-void", &Wreturn_void },
diff --git a/lib.h b/lib.h
index 095b1f517..35edd3217 100644
--- a/lib.h
+++ b/lib.h
@@ -117,6 +117,7 @@ extern int Winit_cstring;
 extern int Wnon_pointer_null;
 extern int Wold_initializer;
 extern int Wone_bit_signed_bitfield;
+extern int Woverride_init;
 extern int Wparen_string;
 extern int Wptr_subtraction_blows;
 extern int Wreturn_void;
diff --git a/validation/Woverride-init-def.c b/validation/Woverride-init-def.c
new file mode 100644
index 000000000..95ecf33be
--- /dev/null
+++ b/validation/Woverride-init-def.c
@@ -0,0 +1,14 @@
+static int array[] = {
+	[1] = 3,
+	[1] = 1,		/* check-should-warn */
+};
+
+/*
+ * check-name: Woverride-init-def
+ * check-command: sparse $file
+ *
+ * check-error-start
+Woverride-init-def.c:2:10: warning: Initializer entry defined twice
+Woverride-init-def.c:3:10:   also defined here
+ * check-error-end
+ */
diff --git a/validation/Woverride-init-no.c b/validation/Woverride-init-no.c
new file mode 100644
index 000000000..ba4d82b9f
--- /dev/null
+++ b/validation/Woverride-init-no.c
@@ -0,0 +1,12 @@
+static int array[] = {
+	[1] = 3,
+	[1] = 1,		/* check-should-warn */
+};
+
+/*
+ * check-name: Woverride-init-no
+ * check-command: sparse -Wno-override-init $file
+ *
+ * check-error-start
+ * check-error-end
+ */
diff --git a/validation/Woverride-init-yes.c b/validation/Woverride-init-yes.c
new file mode 100644
index 000000000..c04a836be
--- /dev/null
+++ b/validation/Woverride-init-yes.c
@@ -0,0 +1,14 @@
+static int array[] = {
+	[1] = 3,
+	[1] = 1,		/* check-should-warn */
+};
+
+/*
+ * check-name: Woverride-init-yes
+ * check-command: sparse -Woverride-init $file
+ *
+ * check-error-start
+Woverride-init-yes.c:2:10: warning: Initializer entry defined twice
+Woverride-init-yes.c:3:10:   also defined here
+ * check-error-end
+ */
-- 
2.11.1

[PATCH 2/5] add test case for warnings about overlapping initializers

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/field-override.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 validation/field-override.c

diff --git a/validation/field-override.c b/validation/field-override.c
new file mode 100644
index 000000000..d5d00dfa8
--- /dev/null
+++ b/validation/field-override.c
@@ -0,0 +1,88 @@
+static int ref[] = {
+	[1] = 3,
+	[2] = 3,
+	[3] = 3,
+	[2] = 2,		/* check-should-warn */
+	[1] = 1,		/* check-should-warn */
+};
+
+static int foo[] = {
+	[1 ... 3] = 3,
+};
+
+static int foz[4] = {
+	[0 ... 3] = 3,
+	[0] = 0,
+	[1] = 0,
+	[2 ... 3] = 1,
+	[2] = 3,		/* check-should-warn */
+	[3] = 3,		/* check-should-warn */
+};
+
+static int bar[] = {
+	[1 ... 3] = 3,
+	[1]       = 1,		/* check-should-warn */
+	[2]       = 2,		/* check-should-warn */
+	[2 ... 4] = 2,		/* check-should-warn */
+	[2 ... 3] = 2,		/* check-should-warn */
+	[4] = 4,		/* check-should-warn */
+	[0] = 0,
+	[5] = 5,
+};
+
+static int baz[3][3] = {
+	[0 ... 2][0 ... 2] = 0,
+	[0] = { 0, 0, 0, },	/* check-should-warn */
+	[0][0] = 1,		/* check-should-warn */
+	[1] = { 0, 0, 0, },	/* check-should-warn */
+	[1][0] = 1,		/* check-should-warn */
+	[1][1] = 1,		/* check-should-warn */
+	[1 ... 2][1 ... 2] = 2,
+};
+
+
+struct s {
+	int i;
+	int a[2];
+};
+
+static struct s s = {
+	.a[0] = 0,
+	.a[1] = 1,
+};
+
+static struct s a[2] = {
+	[0].i = 0,
+	[1].i = 1,
+	[0].a[0] = 2,
+	[0].a[1] = 3,
+};
+
+static struct s b[2] = {
+	[0 ... 1] = { 0, { 1, 2 }, },
+	[0].i = 0,
+	[1].i = 1,
+	[0].a[0] = 2,
+	[0].a[1] = 3,
+};
+
+/*
+ * check-name: field-override
+ * check-command: sparse -Woverride-init $file
+ * check-known-to-fail
+ *
+ * check-error-start
+field-override.c:2:10: warning: Initializer entry defined twice
+field-override.c:6:10:   also defined here
+field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:15:10:   also defined here
+field-override.c:23:10: warning: Initializer entry defined twice
+field-override.c:24:10:   also defined here
+field-override.c:23:10: warning: Initializer entry defined twice
+field-override.c:25:10:   also defined here
+field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:35:10:   also defined here
+field-override.c:62:10: warning: Initializer entry defined twice
+field-override.c:63:10:   also defined here
+ * check-error-end
+ */
-- 
2.11.1

[PATCH 3/5] allow to warn on all overlapping initializers

[Luc Van Oostenryck]

By default, sparse only warns on the first overlapping initialier.

While this may be sensible for most situation, it's not always wanted
to hide those others errors. This is especially annoying when testing.

Change this by introducing a new warning flag '-Woverride-init-all',
disabled by default and whose intented use is sparse's testsuite.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 expand.c                    |  3 ++-
 lib.c                       |  2 ++
 lib.h                       |  1 +
 validation/field-override.c | 34 +++++++++++++++++++++++++++++++++-
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/expand.c b/expand.c
index 7457c94dd..48cfa33d8 100644
--- a/expand.c
+++ b/expand.c
@@ -925,7 +925,8 @@ static void verify_nonoverlapping(struct expression_list **list)
 		if (a && bit_offset(a) == bit_offset(b)) {
 			warning(a->pos, "Initializer entry defined twice");
 			info(b->pos, "  also defined here");
-			return;
+			if (!Woverride_init_all)
+				return;
 		}
 		a = b;
 	} END_FOR_EACH_PTR(b);
diff --git a/lib.c b/lib.c
index a20f68aa2..b3b38a43f 100644
--- a/lib.c
+++ b/lib.c
@@ -232,6 +232,7 @@ int Wnon_pointer_null = 1;
 int Wold_initializer = 1;
 int Wone_bit_signed_bitfield = 1;
 int Woverride_init = 1;
+int Woverride_init_all = 0;
 int Wparen_string = 0;
 int Wptr_subtraction_blows = 0;
 int Wreturn_void = 0;
@@ -482,6 +483,7 @@ static const struct warning {
 	{ "old-initializer", &Wold_initializer },
 	{ "one-bit-signed-bitfield", &Wone_bit_signed_bitfield },
 	{ "override-init", &Woverride_init },
+	{ "override-init-all", &Woverride_init_all },
 	{ "paren-string", &Wparen_string },
 	{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
 	{ "return-void", &Wreturn_void },
diff --git a/lib.h b/lib.h
index 35edd3217..265c5ec7f 100644
--- a/lib.h
+++ b/lib.h
@@ -118,6 +118,7 @@ extern int Wnon_pointer_null;
 extern int Wold_initializer;
 extern int Wone_bit_signed_bitfield;
 extern int Woverride_init;
+extern int Woverride_init_all;
 extern int Wparen_string;
 extern int Wptr_subtraction_blows;
 extern int Wreturn_void;
diff --git a/validation/field-override.c b/validation/field-override.c
index d5d00dfa8..cae30b4a2 100644
--- a/validation/field-override.c
+++ b/validation/field-override.c
@@ -68,21 +68,53 @@ static struct s b[2] = {
 
 /*
  * check-name: field-override
- * check-command: sparse -Woverride-init $file
+ * check-command: sparse -Woverride-init -Woverride-init-all $file
  * check-known-to-fail
  *
  * check-error-start
 field-override.c:2:10: warning: Initializer entry defined twice
 field-override.c:6:10:   also defined here
+field-override.c:3:10: warning: Initializer entry defined twice
+field-override.c:5:10:   also defined here
 field-override.c:14:10: warning: Initializer entry defined twice
 field-override.c:15:10:   also defined here
+field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:16:10:   also defined here
+field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:17:10:   also defined here
+field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:18:10:   also defined here
+field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:19:10:   also defined here
 field-override.c:23:10: warning: Initializer entry defined twice
 field-override.c:24:10:   also defined here
 field-override.c:23:10: warning: Initializer entry defined twice
 field-override.c:25:10:   also defined here
+field-override.c:23:10: warning: Initializer entry defined twice
+field-override.c:26:10:   also defined here
+field-override.c:26:10: warning: Initializer entry defined twice
+field-override.c:27:10:   also defined here
+field-override.c:26:10: warning: Initializer entry defined twice
+field-override.c:28:10:   also defined here
 field-override.c:34:10: warning: Initializer entry defined twice
 field-override.c:35:10:   also defined here
+field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:36:10:   also defined here
+field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:37:10:   also defined here
+field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:38:10:   also defined here
+field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:39:10:   also defined here
+field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:40:10:   also defined here
 field-override.c:62:10: warning: Initializer entry defined twice
 field-override.c:63:10:   also defined here
+field-override.c:62:10: warning: Initializer entry defined twice
+field-override.c:65:10:   also defined here
+field-override.c:62:10: warning: Initializer entry defined twice
+field-override.c:66:10:   also defined here
+field-override.c:62:10: warning: Initializer entry defined twice
+field-override.c:64:10:   also defined here
  * check-error-end
  */
-- 
2.11.1

[PATCH 4/5] fix checking of overlapping initializer

[Luc Van Oostenryck]

The current routine checking if some initializers overlap with each
others only check the offset of the initialierd fields, not taking
in account that array elements can be initialized by range with the
'[a ... b]' notation.

Fix this by changing the check so that now we compare the offset of
the current field with the end of the previous one.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 expand.c                    | 25 +++++++++++++++++++++++--
 validation/field-override.c |  1 -
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/expand.c b/expand.c
index 48cfa33d8..80699c4d6 100644
--- a/expand.c
+++ b/expand.c
@@ -896,6 +896,20 @@ static unsigned long bit_offset(const struct expression *expr)
 	return offset;
 }
 
+static unsigned long bit_range(const struct expression *expr)
+{
+	unsigned long range = 0;
+	unsigned long size = 0;
+	while (expr->type == EXPR_POS) {
+		unsigned long nr = expr->init_nr;
+		size = expr->ctype->bit_size;
+		range += (nr - 1) * size;
+		expr = expr->init_expr;
+	}
+	range += size;
+	return range;
+}
+
 static int compare_expressions(const void *_a, const void *_b)
 {
 	const struct expression *a = _a;
@@ -914,21 +928,28 @@ static void sort_expression_list(struct expression_list **list)
 static void verify_nonoverlapping(struct expression_list **list)
 {
 	struct expression *a = NULL;
+	unsigned long max = 0;
 	struct expression *b;
 
 	if (!Woverride_init)
 		return;
 
 	FOR_EACH_PTR(*list, b) {
+		unsigned long off, end;
 		if (!b->ctype || !b->ctype->bit_size)
 			continue;
-		if (a && bit_offset(a) == bit_offset(b)) {
+		off = bit_offset(b);
+		if (a && off < max) {
 			warning(a->pos, "Initializer entry defined twice");
 			info(b->pos, "  also defined here");
 			if (!Woverride_init_all)
 				return;
 		}
-		a = b;
+		end = off + bit_range(b);
+		if (end > max) {
+			max = end;
+			a = b;
+		}
 	} END_FOR_EACH_PTR(b);
 }
 
diff --git a/validation/field-override.c b/validation/field-override.c
index cae30b4a2..5b77af73e 100644
--- a/validation/field-override.c
+++ b/validation/field-override.c
@@ -69,7 +69,6 @@ static struct s b[2] = {
 /*
  * check-name: field-override
  * check-command: sparse -Woverride-init -Woverride-init-all $file
- * check-known-to-fail
  *
  * check-error-start
 field-override.c:2:10: warning: Initializer entry defined twice
-- 
2.11.1

[PATCH 5/5] ignore whole-range overlapping initializer

[Luc Van Oostenryck]

When an array is initialized, it may be convenient to first
initialize all entries with some default value via the
'[a ... b]' notation and then override some of these entries
with a non-default value. Unfortunately, this, of course,
is not compatible with the default warning flag '-Woverride-init'.

Fix this by introducing an exception to the usual detection
of overlapping initializers which, only for what concerns this
warning, ignore an '[a ... b]' entry if:
- it is the first one
- it covers the whole range of the array.

If needed, the previous ehaviour can be restored by using a new
warning flag, disabled by default: '-Woverride-init-whole-range'.

Suggested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 expand.c                    | 13 +++++++++++--
 lib.c                       |  1 +
 lib.h                       |  1 +
 validation/field-override.c | 30 ++++++------------------------
 4 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/expand.c b/expand.c
index 80699c4d6..e6928bdc4 100644
--- a/expand.c
+++ b/expand.c
@@ -925,10 +925,11 @@ static void sort_expression_list(struct expression_list **list)
 	sort_list((struct ptr_list **)list, compare_expressions);
 }
 
-static void verify_nonoverlapping(struct expression_list **list)
+static void verify_nonoverlapping(struct expression_list **list, struct expression *expr)
 {
 	struct expression *a = NULL;
 	unsigned long max = 0;
+	unsigned long whole = expr->ctype->bit_size;
 	struct expression *b;
 
 	if (!Woverride_init)
@@ -946,6 +947,14 @@ static void verify_nonoverlapping(struct expression_list **list)
 				return;
 		}
 		end = off + bit_range(b);
+		if (!a && !Woverride_init_whole_range) {
+			// If first entry is the whole range, do not let
+			// any warning about it (this allow to initialize
+			// an array with some default value and then override
+			// some specific entries).
+			if (off == 0 && end == whole)
+				continue;
+		}
 		if (end > max) {
 			max = end;
 			a = b;
@@ -1019,7 +1028,7 @@ static int expand_expression(struct expression *expr)
 
 	case EXPR_INITIALIZER:
 		sort_expression_list(&expr->expr_list);
-		verify_nonoverlapping(&expr->expr_list);
+		verify_nonoverlapping(&expr->expr_list, expr);
 		return expand_expression_list(expr->expr_list);
 
 	case EXPR_IDENTIFIER:
diff --git a/lib.c b/lib.c
index b3b38a43f..95a4c461d 100644
--- a/lib.c
+++ b/lib.c
@@ -233,6 +233,7 @@ int Wold_initializer = 1;
 int Wone_bit_signed_bitfield = 1;
 int Woverride_init = 1;
 int Woverride_init_all = 0;
+int Woverride_init_whole_range = 0;
 int Wparen_string = 0;
 int Wptr_subtraction_blows = 0;
 int Wreturn_void = 0;
diff --git a/lib.h b/lib.h
index 265c5ec7f..134e56040 100644
--- a/lib.h
+++ b/lib.h
@@ -119,6 +119,7 @@ extern int Wold_initializer;
 extern int Wone_bit_signed_bitfield;
 extern int Woverride_init;
 extern int Woverride_init_all;
+extern int Woverride_init_whole_range;
 extern int Wparen_string;
 extern int Wptr_subtraction_blows;
 extern int Wreturn_void;
diff --git a/validation/field-override.c b/validation/field-override.c
index 5b77af73e..ec6987df7 100644
--- a/validation/field-override.c
+++ b/validation/field-override.c
@@ -75,15 +75,9 @@ field-override.c:2:10: warning: Initializer entry defined twice
 field-override.c:6:10:   also defined here
 field-override.c:3:10: warning: Initializer entry defined twice
 field-override.c:5:10:   also defined here
-field-override.c:14:10: warning: Initializer entry defined twice
-field-override.c:15:10:   also defined here
-field-override.c:14:10: warning: Initializer entry defined twice
-field-override.c:16:10:   also defined here
-field-override.c:14:10: warning: Initializer entry defined twice
-field-override.c:17:10:   also defined here
-field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:17:10: warning: Initializer entry defined twice
 field-override.c:18:10:   also defined here
-field-override.c:14:10: warning: Initializer entry defined twice
+field-override.c:17:10: warning: Initializer entry defined twice
 field-override.c:19:10:   also defined here
 field-override.c:23:10: warning: Initializer entry defined twice
 field-override.c:24:10:   also defined here
@@ -95,25 +89,13 @@ field-override.c:26:10: warning: Initializer entry defined twice
 field-override.c:27:10:   also defined here
 field-override.c:26:10: warning: Initializer entry defined twice
 field-override.c:28:10:   also defined here
-field-override.c:34:10: warning: Initializer entry defined twice
-field-override.c:35:10:   also defined here
-field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:35:10: warning: Initializer entry defined twice
 field-override.c:36:10:   also defined here
-field-override.c:34:10: warning: Initializer entry defined twice
-field-override.c:37:10:   also defined here
-field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:37:10: warning: Initializer entry defined twice
 field-override.c:38:10:   also defined here
-field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:37:10: warning: Initializer entry defined twice
 field-override.c:39:10:   also defined here
-field-override.c:34:10: warning: Initializer entry defined twice
+field-override.c:37:10: warning: Initializer entry defined twice
 field-override.c:40:10:   also defined here
-field-override.c:62:10: warning: Initializer entry defined twice
-field-override.c:63:10:   also defined here
-field-override.c:62:10: warning: Initializer entry defined twice
-field-override.c:65:10:   also defined here
-field-override.c:62:10: warning: Initializer entry defined twice
-field-override.c:66:10:   also defined here
-field-override.c:62:10: warning: Initializer entry defined twice
-field-override.c:64:10:   also defined here
  * check-error-end
  */
-- 
2.11.1

Re: [PATCH 5/5] ignore whole-range overlapping initializer

[Christopher Li]

On Wed, Feb 22, 2017 at 11:30 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> When an array is initialized, it may be convenient to first
> initialize all entries with some default value via the
> '[a ... b]' notation and then override some of these entries
> with a non-default value. Unfortunately, this, of course,
> is not compatible with the default warning flag '-Woverride-init'.

Looks good to me.


> +               if (!a && !Woverride_init_whole_range) {
> +                       // If first entry is the whole range, do not let
> +                       // any warning about it (this allow to initialize
> +                       // an array with some default value and then override
> +                       // some specific entries).

You might want to use some C style multi line comment /* ... */
Using // for multiple line is a bit strange.

Applied to sparse-next, but if you have update version I will
apply that as well.

Chris

Re: [PATCH 5/5] ignore whole-range overlapping initializer

[Luc Van Oostenryck]

>> +               if (!a && !Woverride_init_whole_range) {
>> +                       // If first entry is the whole range, do not let
>> +                       // any warning about it (this allow to initialize
>> +                       // an array with some default value and then override
>> +                       // some specific entries).
>
> You might want to use some C style multi line comment /* ... */
> Using // for multiple line is a bit strange.
>
> Applied to sparse-next, but if you have update version I will
> apply that as well.

Yes, I tend to always use the C++ style because I'm a lazy typer :)

I'll change it since I've others changes for this serie.

Luc