* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-17 16:51 ` Stephen Boyd 0 siblings, 0 replies; 26+ messages in thread From: Stephen Boyd @ 2017-02-17 16:51 UTC (permalink / raw) To: Catalin Marinas, Will Deacon Cc: linux-arm-kernel, linux-kernel, Punit Agrawal, Mark Rutland 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-17 16:51 ` Stephen Boyd 0 siblings, 0 replies; 26+ messages in thread From: Stephen Boyd @ 2017-02-17 16:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-17 16:51 ` Stephen Boyd @ 2017-02-17 17:31 ` Russell King - ARM Linux -1 siblings, 0 replies; 26+ messages in thread From: Russell King - ARM Linux @ 2017-02-17 17:31 UTC (permalink / raw) To: Stephen Boyd Cc: Catalin Marinas, Will Deacon, Mark Rutland, Punit Agrawal, linux-kernel, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-17 17:31 ` Russell King - ARM Linux 0 siblings, 0 replies; 26+ messages in thread From: Russell King - ARM Linux @ 2017-02-17 17:31 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-17 17:31 ` Russell King - ARM Linux (?) @ 2017-02-17 17:46 ` Stephen Boyd -1 siblings, 0 replies; 26+ messages in thread From: Stephen Boyd @ 2017-02-17 17:46 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-17 16:51 ` Stephen Boyd @ 2017-02-19 1:58 ` Luc Van Oostenryck -1 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-19 1:58 UTC (permalink / raw) To: Stephen Boyd Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Punit Agrawal, Mark Rutland 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-19 1:58 ` Luc Van Oostenryck 0 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-19 1:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-19 1:58 ` Luc Van Oostenryck (?) @ 2017-02-20 8:47 ` Stephen Boyd 2017-02-20 10:04 ` Luc Van Oostenryck 2017-02-20 10:49 ` Mark Rutland -1 siblings, 2 replies; 26+ messages in thread From: Stephen Boyd @ 2017-02-20 8:47 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-20 8:47 ` Stephen Boyd @ 2017-02-20 10:04 ` Luc Van Oostenryck 2017-02-20 10:49 ` Mark Rutland 1 sibling, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-20 10:04 UTC (permalink / raw) To: Stephen Boyd Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Punit Agrawal, Mark Rutland 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-20 10:04 ` Luc Van Oostenryck 0 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-20 10:04 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-20 8:47 ` Stephen Boyd @ 2017-02-20 10:49 ` Mark Rutland 2017-02-20 10:49 ` Mark Rutland 1 sibling, 0 replies; 26+ messages in thread From: Mark Rutland @ 2017-02-20 10:49 UTC (permalink / raw) To: Stephen Boyd Cc: Luc Van Oostenryck, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Punit Agrawal 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-20 10:49 ` Mark Rutland 0 siblings, 0 replies; 26+ messages in thread From: Mark Rutland @ 2017-02-20 10:49 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-20 10:49 ` Mark Rutland @ 2017-02-20 21:33 ` Luc Van Oostenryck -1 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-20 21:33 UTC (permalink / raw) To: Mark Rutland Cc: Stephen Boyd, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Punit Agrawal, linux-sparse 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-20 21:33 ` Luc Van Oostenryck 0 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-20 21:33 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-20 21:33 ` Luc Van Oostenryck @ 2017-02-21 11:03 ` Will Deacon -1 siblings, 0 replies; 26+ messages in thread From: Will Deacon @ 2017-02-21 11:03 UTC (permalink / raw) To: Luc Van Oostenryck Cc: Mark Rutland, Stephen Boyd, Catalin Marinas, linux-arm-kernel, linux-kernel, Punit Agrawal, linux-sparse 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-21 11:03 ` Will Deacon 0 siblings, 0 replies; 26+ messages in thread From: Will Deacon @ 2017-02-21 11:03 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly 2017-02-21 11:03 ` Will Deacon @ 2017-02-22 13:00 ` Luc Van Oostenryck -1 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 13:00 UTC (permalink / raw) To: Will Deacon Cc: Mark Rutland, Stephen Boyd, Catalin Marinas, linux-arm-kernel, linux-kernel, Punit Agrawal, linux-sparse 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly @ 2017-02-22 13:00 ` Luc Van Oostenryck 0 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 13:00 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/5] improve detection of overlapping initializers 2017-02-21 11:03 ` Will Deacon (?) (?) @ 2017-02-22 15:30 ` Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 1/5] use option: '-Woverride-init' Luc Van Oostenryck ` (4 more replies) -1 siblings, 5 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 15:30 UTC (permalink / raw) To: linux-sparse Cc: Christopher Li, Mark Rutland, Stephen Boyd, Will Deacon, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/5] use option: '-Woverride-init' 2017-02-22 15:30 ` [PATCH 0/5] improve detection of overlapping initializers Luc Van Oostenryck @ 2017-02-22 15:30 ` Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 2/5] add test case for warnings about overlapping initializers Luc Van Oostenryck ` (3 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 15:30 UTC (permalink / raw) To: linux-sparse Cc: Christopher Li, Mark Rutland, Stephen Boyd, Will Deacon, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/5] add test case for warnings about overlapping initializers 2017-02-22 15:30 ` [PATCH 0/5] improve detection of overlapping initializers Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 1/5] use option: '-Woverride-init' Luc Van Oostenryck @ 2017-02-22 15:30 ` Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 3/5] allow to warn on all " Luc Van Oostenryck ` (2 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 15:30 UTC (permalink / raw) To: linux-sparse Cc: Christopher Li, Mark Rutland, Stephen Boyd, Will Deacon, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/5] allow to warn on all overlapping initializers 2017-02-22 15:30 ` [PATCH 0/5] improve detection of overlapping initializers Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 1/5] use option: '-Woverride-init' Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 2/5] add test case for warnings about overlapping initializers Luc Van Oostenryck @ 2017-02-22 15:30 ` Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 4/5] fix checking of overlapping initializer Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 5/5] ignore whole-range " Luc Van Oostenryck 4 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 15:30 UTC (permalink / raw) To: linux-sparse Cc: Christopher Li, Mark Rutland, Stephen Boyd, Will Deacon, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/5] fix checking of overlapping initializer 2017-02-22 15:30 ` [PATCH 0/5] improve detection of overlapping initializers Luc Van Oostenryck ` (2 preceding siblings ...) 2017-02-22 15:30 ` [PATCH 3/5] allow to warn on all " Luc Van Oostenryck @ 2017-02-22 15:30 ` Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 5/5] ignore whole-range " Luc Van Oostenryck 4 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 15:30 UTC (permalink / raw) To: linux-sparse Cc: Christopher Li, Mark Rutland, Stephen Boyd, Will Deacon, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/5] ignore whole-range overlapping initializer 2017-02-22 15:30 ` [PATCH 0/5] improve detection of overlapping initializers Luc Van Oostenryck ` (3 preceding siblings ...) 2017-02-22 15:30 ` [PATCH 4/5] fix checking of overlapping initializer Luc Van Oostenryck @ 2017-02-22 15:30 ` Luc Van Oostenryck 2017-02-27 16:34 ` Christopher Li 4 siblings, 1 reply; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-22 15:30 UTC (permalink / raw) To: linux-sparse Cc: Christopher Li, Mark Rutland, Stephen Boyd, Will Deacon, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] ignore whole-range overlapping initializer 2017-02-22 15:30 ` [PATCH 5/5] ignore whole-range " Luc Van Oostenryck @ 2017-02-27 16:34 ` Christopher Li 2017-02-27 21:38 ` Luc Van Oostenryck 0 siblings, 1 reply; 26+ messages in thread From: Christopher Li @ 2017-02-27 16:34 UTC (permalink / raw) To: Luc Van Oostenryck; +Cc: Linux-Sparse, Mark Rutland, Stephen Boyd, Will Deacon 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] ignore whole-range overlapping initializer 2017-02-27 16:34 ` Christopher Li @ 2017-02-27 21:38 ` Luc Van Oostenryck 0 siblings, 0 replies; 26+ messages in thread From: Luc Van Oostenryck @ 2017-02-27 21:38 UTC (permalink / raw) To: Christopher Li; +Cc: Linux-Sparse, Mark Rutland, Stephen Boyd, Will Deacon >> + 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-02-27 22:05 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-17 16:51 [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly Stephen Boyd 2017-02-17 16:51 ` Stephen Boyd 2017-02-17 17:31 ` Russell King - ARM Linux 2017-02-17 17:31 ` Russell King - ARM Linux 2017-02-17 17:46 ` Stephen Boyd 2017-02-19 1:58 ` Luc Van Oostenryck 2017-02-19 1:58 ` Luc Van Oostenryck 2017-02-20 8:47 ` Stephen Boyd 2017-02-20 10:04 ` Luc Van Oostenryck 2017-02-20 10:04 ` Luc Van Oostenryck 2017-02-20 10:49 ` Mark Rutland 2017-02-20 10:49 ` Mark Rutland 2017-02-20 21:33 ` Luc Van Oostenryck 2017-02-20 21:33 ` Luc Van Oostenryck 2017-02-21 11:03 ` Will Deacon 2017-02-21 11:03 ` Will Deacon 2017-02-22 13:00 ` Luc Van Oostenryck 2017-02-22 13:00 ` Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 0/5] improve detection of overlapping initializers Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 1/5] use option: '-Woverride-init' Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 2/5] add test case for warnings about overlapping initializers Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 3/5] allow to warn on all " Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 4/5] fix checking of overlapping initializer Luc Van Oostenryck 2017-02-22 15:30 ` [PATCH 5/5] ignore whole-range " Luc Van Oostenryck 2017-02-27 16:34 ` Christopher Li 2017-02-27 21:38 ` Luc Van Oostenryck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.