Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320406 > unrolled thread

[PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-01-28 09:30 +0100
    Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more  types Andrew Morton <akpm@linux-foundation.org> - 2016-02-02 07:30 +0100
      Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-02 09:30 +0100
    Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more  types Andrew Morton <akpm@linux-foundation.org> - 2016-02-03 01:40 +0100
      Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-03 12:00 +0100
      [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-03 14:20 +0100
        Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Arnd Bergmann <arnd@arndb.de> - 2016-02-04 13:50 +0100
          Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-04 15:50 +0100
            Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Arnd Bergmann <arnd@arndb.de> - 2016-02-04 16:10 +0100
              Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Arnd Bergmann <arnd@arndb.de> - 2016-02-04 16:20 +0100
          Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more  types Andrew Morton <akpm@linux-foundation.org> - 2016-02-04 20:00 +0100
            Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Arnd Bergmann <arnd@arndb.de> - 2016-02-05 12:00 +0100
              Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-08 09:50 +0100
                Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Arnd Bergmann <arnd@arndb.de> - 2016-02-08 13:10 +0100
                  Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more  types Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-09 02:50 +0100
                  Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-09 09:50 +0100
        Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-02-05 00:40 +0100

#1320406 — [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

Current implementation of IS_ERR_VALUE works correctly only with
following types:
- unsigned long,
- short, int, long.
Other types are handled incorrectly either on 32-bit either on 64-bit
either on both architectures.
The patch fixes it by comparing argument with MAX_ERRNO casted
to argument's type for unsigned types and comparing with zero for signed
types. As a result all integer types bigger than char are handled properly.

I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
architecture. Instead of fixing usage I propose to enhance the macro
to cover more types.
And just for the record: the macro is used 101 times with signed variables,
I am not sure if it should be preferred over simple comparison "ret < 0",
but the new version can do it as well.

And below list of detected potential errors:
drivers/char/mem.c:698:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(( unsigned long long ) offset)
drivers/media/platform/soc_camera/atmel-isi.c:1089:21-22: WARNING: incorrect argument type in IS_ERR_VALUE(irq)
drivers/net/ethernet/freescale/fs_enet/mac-scc.c:149:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(fep -> ring_mem_addr)
drivers/net/ethernet/freescale/ucc_geth.c:2237:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_bd_ring_offset [ j ])
drivers/net/ethernet/freescale/ucc_geth.c:2314:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_ring_offset [ j ])
drivers/net/ethernet/freescale/ucc_geth.c:2524:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_glbl_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2544:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_tx_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2571:46-47: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> send_q_mem_reg_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2612:42-43: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> scheduler_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2659:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_fw_statistics_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2696:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_glbl_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2715:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_rx_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2736:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_fw_statistics_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2756:53-54: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_irq_coalescing_tbl_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2822:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_qs_tbl_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2908:47-48: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> exf_glbl_param_offset)
drivers/net/ethernet/freescale/ucc_geth.c:292:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_offset)
drivers/net/ethernet/freescale/ucc_geth.c:3042:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_pram_offset)
drivers/soc/fsl/qe/ucc_fast.c:271:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_tx_virtual_fifo_base_offset)
drivers/soc/fsl/qe/ucc_fast.c:284:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_rx_virtual_fifo_base_offset)
drivers/soc/fsl/qe/ucc_slow.c:186:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> us_pram_offset)
drivers/soc/fsl/qe/ucc_slow.c:213:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> rx_base_offset)
drivers/soc/fsl/qe/ucc_slow.c:224:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> tx_base_offset)
drivers/tty/serial/clps711x.c:471:29-30: WARNING: incorrect argument type in IS_ERR_VALUE(s -> port . irq)
drivers/tty/serial/digicolor-usart.c:485:30-31: WARNING: incorrect argument type in IS_ERR_VALUE(dp -> port . irq)
drivers/usb/gadget/udc/fsl_qe_udc.c:2369:26-27: WARNING: incorrect argument type in IS_ERR_VALUE(tmp_addr)
drivers/video/fbdev/exynos/exynos_mipi_dsi.c:406:27-28: WARNING: incorrect argument type in IS_ERR_VALUE(dsim -> irq)
net/ipv4/netfilter/arp_tables.c:1427:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(iter1 -> counters . pcnt)
net/ipv4/netfilter/arp_tables.c:530:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:1614:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:674:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:1624:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:687:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v2:
 - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
 - expand commit message.
---
 include/linux/err.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index 56762ab..43a6adb 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,9 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
+				? unlikely((x) < 0) \
+				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

[toc] | [next] | [standalone]

#1323822 — Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:

> Current implementation of IS_ERR_VALUE works correctly only with
> following types:
> - unsigned long,
> - short, int, long.
> Other types are handled incorrectly either on 32-bit either on 64-bit
> either on both architectures.
> The patch fixes it by comparing argument with MAX_ERRNO casted
> to argument's type for unsigned types and comparing with zero for signed
> types. As a result all integer types bigger than char are handled properly.
> 
> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
> architecture. Instead of fixing usage I propose to enhance the macro
> to cover more types.
> And just for the record: the macro is used 101 times with signed variables,
> I am not sure if it should be preferred over simple comparison "ret < 0",
> but the new version can do it as well.
> 
> And below list of detected potential errors:
>
> ...
>
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) < 0) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  

hm, seems complicated.  Can we simply cast the value to long?

#define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO)

and simplify that to

#define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO)

or something like that.

[toc] | [prev] | [next] | [standalone]

#1323883

On 02/02/2016 07:23 AM, Andrew Morton wrote:
> On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> Current implementation of IS_ERR_VALUE works correctly only with
>> following types:
>> - unsigned long,
>> - short, int, long.
>> Other types are handled incorrectly either on 32-bit either on 64-bit
>> either on both architectures.
>> The patch fixes it by comparing argument with MAX_ERRNO casted
>> to argument's type for unsigned types and comparing with zero for signed
>> types. As a result all integer types bigger than char are handled properly.
>>
>> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
>> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
>> architecture. Instead of fixing usage I propose to enhance the macro
>> to cover more types.
>> And just for the record: the macro is used 101 times with signed variables,
>> I am not sure if it should be preferred over simple comparison "ret < 0",
>> but the new version can do it as well.
>>
>> And below list of detected potential errors:
>>
>> ...
>>
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,9 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>> +				? unlikely((x) < 0) \
>> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>  
> hm, seems complicated.  Can we simply cast the value to long?
>
> #define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO)
>
> and simplify that to
>
> #define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO)
>
> or something like that.
It will not work with u32 on 64bit systems.

Short rationales behind my implementation:

1. Typical usage pattern of the macro looks like:
T x;
...
x = -ESOME_ERROR;
...
if (IS_ERR_VALUE(x))
    ...

In error assignment we have casting of -ESOME_ERROR to type T.
Casting of -MAX_ERRNO to the same type in the macro assures that
comparison will be sane, at least for types big enough. In short we ends
at following expression (for unsigned types):
    (T)-ESOME_ERROR >= (T)-MAX_ERRNO
In old implementation we ended at:
    (unsigned)(T)-ESOME_ERROR >= (unsigned)-MAX_ERRNO
Different castings for -ESOME_ERROR and for -MAX_ERRNO makes this
comparison incorrect for some types T.

2. Error checking is completely different for signed and unsigned vars:
a. signed are compared to 0: ret < 0.
b. unsigned are compared with some high value: ret >= (-MAX_ERRNO).
This dualism is clearly visible and emphasized in this implementation.
In old implementation IS_ERR_VALUE works correctly for some signed types
due to obscure C casting rules.

Summarizing: current implementation is short but tricky, answering why
it works/fails for certain types is quite challenging. On the other side
proposed implementation is longer but more straightforward, and of course
is correct for more types :)

Maybe, to make it more clear, it could be good to use separate macro for
signedness:

#define IS_SIGNED_TYPE(t) ((t)(-1) <= 0)

#define IS_ERR_VALUE(x) (IS_SIGNED_TYPE(typeof(x)) \
                ? unlikely((x) < 0) \
                : unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1324783 — Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:

>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>  - expand commit message.
> ---
>  include/linux/err.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..43a6adb 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) < 0) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))

I'm still getting a bunch of 

include/linux/err.h: In function 'IS_ERR':
include/linux/err.h:37: warning: comparison of unsigned expression < 0 is always false
include/linux/err.h: In function 'IS_ERR_OR_NULL':
include/linux/err.h:42: warning: comparison of unsigned expression < 0 is always false

with gcc-4.4.4.

[toc] | [prev] | [next] | [standalone]

#1325196

On 02/03/2016 01:33 AM, Andrew Morton wrote:
> On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>>  - expand commit message.
>> ---
>>  include/linux/err.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/err.h b/include/linux/err.h
>> index 56762ab..43a6adb 100644
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,9 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>> +				? unlikely((x) < 0) \
>> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
> I'm still getting a bunch of 
>
> include/linux/err.h: In function 'IS_ERR':
> include/linux/err.h:37: warning: comparison of unsigned expression < 0 is always false
> include/linux/err.h: In function 'IS_ERR_OR_NULL':
> include/linux/err.h:42: warning: comparison of unsigned expression < 0 is always false
>
> with gcc-4.4.4.
>
>

These warnings are false positives and gcc up to 4.7 emits them,
gcc 4.8(which I use) behaves correctly (at least on x86 and arm64). I
have tried
to use __builtin_choose_expr instead of ?: operator but it did not help,
although documentation says "the built-in function does not evaluate
the expression that is not chosen"[1].

The sanest gcc silencer I see for now is to replace:
        ? unlikely((x) < 0) \
with
        ? unlikely((x) <= -1) \

On the other side these warnings are caused by -Wtype-limits switch which
is disabled by default in kernel build and treated as broken by Linus [2].
Maybe it is good enough reason to disregard them? :)

Anyway, I will post another iteration.

[1]:
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-g_t_005f_005fbuiltin_005fchoose_005fexpr-4184
[2]: http://permalink.gmane.org/gmane.linux.kernel/2053963

Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1325409 — [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

Current implementation of IS_ERR_VALUE works correctly only with
following types:
- unsigned long,
- short, int, long.
Other types are handled incorrectly either on 32-bit either on 64-bit
either on both architectures.
The patch fixes it by comparing argument with MAX_ERRNO casted
to argument's type for unsigned types and comparing with zero for signed
types. As a result all integer types bigger than char are handled properly.

I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
architecture. Instead of fixing usage I propose to enhance the macro
to cover more types.
And just for the record: the macro is used 101 times with signed variables,
I am not sure if it should be preferred over simple comparison "ret < 0",
but the new version can do it as well.

And below list of detected potential errors:
drivers/char/mem.c:698:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(( unsigned long long ) offset)
drivers/media/platform/soc_camera/atmel-isi.c:1089:21-22: WARNING: incorrect argument type in IS_ERR_VALUE(irq)
drivers/net/ethernet/freescale/fs_enet/mac-scc.c:149:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(fep -> ring_mem_addr)
drivers/net/ethernet/freescale/ucc_geth.c:2237:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_bd_ring_offset [ j ])
drivers/net/ethernet/freescale/ucc_geth.c:2314:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_ring_offset [ j ])
drivers/net/ethernet/freescale/ucc_geth.c:2524:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_glbl_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2544:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_tx_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2571:46-47: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> send_q_mem_reg_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2612:42-43: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> scheduler_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2659:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_fw_statistics_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2696:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_glbl_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2715:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_rx_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2736:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_fw_statistics_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2756:53-54: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_irq_coalescing_tbl_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2822:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_qs_tbl_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2908:47-48: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> exf_glbl_param_offset)
drivers/net/ethernet/freescale/ucc_geth.c:292:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_offset)
drivers/net/ethernet/freescale/ucc_geth.c:3042:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_pram_offset)
drivers/soc/fsl/qe/ucc_fast.c:271:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_tx_virtual_fifo_base_offset)
drivers/soc/fsl/qe/ucc_fast.c:284:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_rx_virtual_fifo_base_offset)
drivers/soc/fsl/qe/ucc_slow.c:186:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> us_pram_offset)
drivers/soc/fsl/qe/ucc_slow.c:213:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> rx_base_offset)
drivers/soc/fsl/qe/ucc_slow.c:224:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> tx_base_offset)
drivers/tty/serial/clps711x.c:471:29-30: WARNING: incorrect argument type in IS_ERR_VALUE(s -> port . irq)
drivers/tty/serial/digicolor-usart.c:485:30-31: WARNING: incorrect argument type in IS_ERR_VALUE(dp -> port . irq)
drivers/usb/gadget/udc/fsl_qe_udc.c:2369:26-27: WARNING: incorrect argument type in IS_ERR_VALUE(tmp_addr)
drivers/video/fbdev/exynos/exynos_mipi_dsi.c:406:27-28: WARNING: incorrect argument type in IS_ERR_VALUE(dsim -> irq)
net/ipv4/netfilter/arp_tables.c:1427:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(iter1 -> counters . pcnt)
net/ipv4/netfilter/arp_tables.c:530:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:1614:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:674:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:1624:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:687:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v3:
 - use '<= -1' instead of '< 0' to silence verbose warnings for gcc
   older than 4.8,
v2:
 - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
 - expand commit message.
---
 include/linux/err.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index 56762ab..b7d4a9f 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,9 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
+				? unlikely((x) <= -1) \
+				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1326750 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Wednesday 03 February 2016 14:15:28 Andrzej Hajda wrote:
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..b7d4a9f 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +                               ? unlikely((x) <= -1) \
> +                               : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  
>  static inline void * __must_check ERR_PTR(long error)
>  {
> 

This has caused a warning to reappear that I had fixed before:

fs/gfs2/dir.c: In function 'get_first_leaf':
fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
   error = get_leaf(dip, leaf_no, bh_out);
         ^
fs/gfs2/dir.c: In function 'dir_split_leaf':
fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
  error = get_leaf(dip, leaf_no, &obh);

See my original patch that was applied at
http://www.gossamer-threads.com/lists/linux/kernel/2353964

Apparently the new version is complex enough to prevent gcc from doing
some optimizations it should do.

I have tried to come up with a new variant that does not bring
the warning back and that should work in all cases:

diff --git a/include/linux/err.h b/include/linux/err.h
index b7d4a9ff6342..bd4936a2c352 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,9 +18,7 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
-				? unlikely((x) <= -1) \
-				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
+#define IS_ERR_VALUE(x)	(unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {


I'm not sure if the cast to 'unsigned long long' might cause less
efficient code to be generated by gcc. I would hope that it is smart
enough to not actually extend shorter variables to 64 bit before
doing the comparison but I have not checked yet.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1326886 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On 02/04/2016 01:40 PM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 14:15:28 Andrzej Hajda wrote:
>> diff --git a/include/linux/err.h b/include/linux/err.h
>> index 56762ab..b7d4a9f 100644
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,9 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>> +                               ? unlikely((x) <= -1) \
>> +                               : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>  
>>  static inline void * __must_check ERR_PTR(long error)
>>  {
>>
> This has caused a warning to reappear that I had fixed before:
>
> fs/gfs2/dir.c: In function 'get_first_leaf':
> fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    error = get_leaf(dip, leaf_no, bh_out);
>          ^
> fs/gfs2/dir.c: In function 'dir_split_leaf':
> fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
>   error = get_leaf(dip, leaf_no, &obh);

What gcc/arch/build options do you use? I cannot reproduce it in my
environment.

Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1326905 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Thursday 04 February 2016 15:44:51 Andrzej Hajda wrote:
> On 02/04/2016 01:40 PM, Arnd Bergmann wrote:
> > On Wednesday 03 February 2016 14:15:28 Andrzej Hajda wrote:
> >> diff --git a/include/linux/err.h b/include/linux/err.h
> >> index 56762ab..b7d4a9f 100644
> >> --- a/include/linux/err.h
> >> +++ b/include/linux/err.h
> >> @@ -18,7 +18,9 @@
> >>  
> >>  #ifndef __ASSEMBLY__
> >>  
> >> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> >> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> >> +                               ? unlikely((x) <= -1) \
> >> +                               : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> >>  
> >>  static inline void * __must_check ERR_PTR(long error)
> >>  {
> >>
> > This has caused a warning to reappear that I had fixed before:
> >
> > fs/gfs2/dir.c: In function 'get_first_leaf':
> > fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >    error = get_leaf(dip, leaf_no, bh_out);
> >          ^
> > fs/gfs2/dir.c: In function 'dir_split_leaf':
> > fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >   error = get_leaf(dip, leaf_no, &obh);
> 
> What gcc/arch/build options do you use? I cannot reproduce it in my
> environment.
> 

I use an ARM gcc-5.3 with an allmodconfig kernel and CONFIG_CC_OPTIMIZE_FOR_SIZE
disabled. I see the same warning with any gcc version since 4.9, but not earlier.

With the IS_ERR_VALUE() macro I sent, I don't see the warning on any gcc
version. I have now also checked that the behavior on x86 gcc-.4.9 is the
same that I see on ARM (I don't have a large collection of x86 gcc versions
though): same warning with linux-next, no warning with my version or after
reverting your patch.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1326912 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Thursday 04 February 2016 16:00:09 Arnd Bergmann wrote:
> With the IS_ERR_VALUE() macro I sent, I don't see the warning on any gcc
> version. I have now also checked that the behavior on x86 gcc-.4.9 is the
> same that I see on ARM (I don't have a large collection of x86 gcc versions
> though): same warning with linux-next, no warning with my version or after
> reverting your patch.
> 

Sorry, scratch that: I don't get this warning at all on x86 with gcc-4.9,
with any version of err.h.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1327104 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:

> diff --git a/include/linux/err.h b/include/linux/err.h
> index b7d4a9ff6342..bd4936a2c352 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,9 +18,7 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> -				? unlikely((x) <= -1) \
> -				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
> +#define IS_ERR_VALUE(x)	(unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>  
>  static inline void * __must_check ERR_PTR(long error)
>  {
> 
> 
> I'm not sure if the cast to 'unsigned long long' might cause less
> efficient code to be generated by gcc. I would hope that it is smart
> enough to not actually extend shorter variables to 64 bit before
> doing the comparison but I have not checked yet.

I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
the text size from 9098 bytes to 9133.  That file has 11 instances of
IS_ERR_VALUE().  

[toc] | [prev] | [next] | [standalone]

#1327603 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > index b7d4a9ff6342..bd4936a2c352 100644
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -18,9 +18,7 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> > -                             ? unlikely((x) <= -1) \
> > -                             : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> > +#define IS_ERR_VALUE(x)      (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
> >  
> >  static inline void * __must_check ERR_PTR(long error)
> >  {
> > 
> > 
> > I'm not sure if the cast to 'unsigned long long' might cause less
> > efficient code to be generated by gcc. I would hope that it is smart
> > enough to not actually extend shorter variables to 64 bit before
> > doing the comparison but I have not checked yet.
> 
> I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
> the text size from 9098 bytes to 9133.  That file has 11 instances of
> IS_ERR_VALUE().  

This seems to be because it brings back the logic to what it was before
in case of 'int' arguments. I checked the assembly output and found mine
to be identical to v4.4 in this case:

	 text    data     bss     dec     hex filename
v4.4	 9942    1872    2856   14670    394e drivers/nvmem/core.o
a.hajda  9922    1872    2856   14650    393a drivers/nvmem/core.o
arnd	 9942    1872    2856   14670    394e drivers/nvmem/core.o

Andrzej's version is a little shorter on ARM because in case of signed numbers
it only checks for negative values, rather than checking for values in the
[-MAX_ERRNO..-1] range. I think the original behavior is more logical
in this case, and my version restores it.

Looking at drivers/char/mem.o, which had an actual bug that was fixed by
Andrzej's patch, the output with my version and his is identical (failing
an lseek on /dev/mem to offset 0xfffffffffffffe00 or higher, instead of
failing for offset 0x00000000fffffe00-0x00000000ffffffff).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1328827 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
>>
>>> diff --git a/include/linux/err.h b/include/linux/err.h
>>> index b7d4a9ff6342..bd4936a2c352 100644
>>> --- a/include/linux/err.h
>>> +++ b/include/linux/err.h
>>> @@ -18,9 +18,7 @@
>>>  
>>>  #ifndef __ASSEMBLY__
>>>  
>>> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>>> -                             ? unlikely((x) <= -1) \
>>> -                             : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>> +#define IS_ERR_VALUE(x)      (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>>>  
>>>  static inline void * __must_check ERR_PTR(long error)
>>>  {
>>>
>>>
>>> I'm not sure if the cast to 'unsigned long long' might cause less
>>> efficient code to be generated by gcc. I would hope that it is smart
>>> enough to not actually extend shorter variables to 64 bit before
>>> doing the comparison but I have not checked yet.
>> I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
>> the text size from 9098 bytes to 9133.  That file has 11 instances of
>> IS_ERR_VALUE().  
> This seems to be because it brings back the logic to what it was before
> in case of 'int' arguments. I checked the assembly output and found mine
> to be identical to v4.4 in this case:
>
> 	 text    data     bss     dec     hex filename
> v4.4	 9942    1872    2856   14670    394e drivers/nvmem/core.o
> a.hajda  9922    1872    2856   14650    393a drivers/nvmem/core.o
> arnd	 9942    1872    2856   14670    394e drivers/nvmem/core.o

I have compared all proposed version with all compilers I have:

            text       data        bss        dec        hex    filename
gcc-4.4
old         8188       1016       2968      12172       2f8c   
.x86/drivers/nvmem/core.o
andrzej     8155       1016       2968      12139       2f6b   
.x86/drivers/nvmem/core.o
arnd        8188       1016       2968      12172       2f8c   
.x86/drivers/nvmem/core.o
rasmus      8266       1016       2968      12250       2fda   
.x86/drivers/nvmem/core.o
 ---
gcc-4.7
old         7642       3816       3248      14706       3972   
.x86/drivers/nvmem/core.o
andrzej     7606       3816       3248      14670       394e   
.x86/drivers/nvmem/core.o
arnd        7642       3816       3248      14706       3972   
.x86/drivers/nvmem/core.o
rasmus      7719       3816       3248      14783       39bf   
.x86/drivers/nvmem/core.o
 ---
gcc-4.8
old         7735       3888       3272      14895       3a2f   
.x86/drivers/nvmem/core.o
andrzej     7698       3888       3272      14858       3a0a   
.x86/drivers/nvmem/core.o
arnd        7735       3888       3272      14895       3a2f   
.x86/drivers/nvmem/core.o
rasmus      7812       3888       3272      14972       3a7c   
.x86/drivers/nvmem/core.o
 ---
arm-linux-gnueabi-gcc-4.7
old        12776       1680       3432      17888       45e0   
.arm/drivers/nvmem/core.o
andrzej    12772       1680       3432      17884       45dc   
.arm/drivers/nvmem/core.o
arnd       12776       1680       3432      17888       45e0   
.arm/drivers/nvmem/core.o
rasmus     12948       1680       3432      18060       468c   
.arm/drivers/nvmem/core.o
 ---
aarch64-linux-gnu-gcc-4.8
old         5967        440         48       6455       1937   
.arm64/drivers/nvmem/core.o
andrzej     5947        440         48       6435       1923   
.arm64/drivers/nvmem/core.o
arnd        5967        440         48       6455       1937   
.arm64/drivers/nvmem/core.o
rasmus      5991        440         48       6479       194f   
.arm64/drivers/nvmem/core.o
 ---

My version produces shortest code, Arnd's is the same as the old one.
On the other side Rasmus proposition seems to be the most straightforward
to me. Anyway I am not sure if the code length is the most important here.

By the way .data segment size grows almost 4 times between gcc 4.4 and
4.8 :)
Also numbers for arm64 looks interesting.

Just for the record below all proposed implementations:
#define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
#define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
                                ? unlikely((x) <= -1) \
                                : unlikely((x) >= (typeof(x))-MAX_ERRNO))
#define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
(unsigned long long)(typeof(x))-MAX_ERRNO))
#define IS_ERR_VALUE_rasmus(x) ({\
        typeof(x) _x = (x);\
        unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
})

>
> Andrzej's version is a little shorter on ARM because in case of signed numbers
> it only checks for negative values, rather than checking for values in the
> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
> in this case, and my version restores it.

As I looked at the usage of the macro in the kernel I have not found any
code
which could benefit from the original behavior, except some buggy code in
staging which have already pending fix[1].
But maybe it would be better to use IS_ERR_VALUE to always check if err
is in
range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
signed
types.

[1]: http://permalink.gmane.org/gmane.comp.file-systems.lustre.devel/4164

Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1328979 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> > On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:

> My version produces shortest code, Arnd's is the same as the old one.
> On the other side Rasmus proposition seems to be the most straightforward
> to me. Anyway I am not sure if the code length is the most important here.
> 
> By the way .data segment size grows almost 4 times between gcc 4.4 and
> 4.8 :)
> Also numbers for arm64 looks interesting.
> 
> Just for the record below all proposed implementations:
> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
>                                 ? unlikely((x) <= -1) \
>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
> (unsigned long long)(typeof(x))-MAX_ERRNO))
> #define IS_ERR_VALUE_rasmus(x) ({\
>         typeof(x) _x = (x);\
>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
> })
> 
> >
> > Andrzej's version is a little shorter on ARM because in case of signed numbers
> > it only checks for negative values, rather than checking for values in the
> > [-MAX_ERRNO..-1] range. I think the original behavior is more logical
> > in this case, and my version restores it.
> 
> As I looked at the usage of the macro in the kernel I have not found any
> code
> which could benefit from the original behavior, except some buggy code in
> staging which have already pending fix[1].
> But maybe it would be better to use IS_ERR_VALUE to always check if err
> is in
> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
> signed types.

If we do that, should we also make it illegal to use an invalid type
for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
char' that are still broken.

diff --git a/include/linux/err.h b/include/linux/err.h
index 039b80bb56ae..6700ad40f73f 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,7 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
+#define IS_ERR_VALUE(x) ({typeof(x) max_errno = -MAX_ERRNO; compiletime_assert(max_errno > 0, "IS_ERR_VALUE takes an unsigned argument"); (x >= max_errno); })
 
 static inline void * __must_check ERR_PTR(long error)
 {


Unfortunately, we get a significant number of errors if we do that, I guess the
list is what you had before

	Arnd

/git/arm-soc/drivers/irqchip/irq-hip04.c: In function 'hip04_of_init':                                   
/git/arm-soc/drivers/irqchip/irq-hip04.c:405:203: error: call to '__compiletime_assert_405' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/irqchip/irq-hip04.o] Error 1
/git/arm-soc/drivers/irqchip/irq-gic.c: In function '__gic_init_bases':
/git/arm-soc/drivers/irqchip/irq-gic.c:1106:205: error: call to '__compiletime_assert_1106' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/irqchip/irq-gic.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/irqchip] Error 2
/git/arm-soc/kernel/pid.c: In function 'alloc_pid':
/git/arm-soc/kernel/pid.c:314:198: error: call to '__compiletime_assert_314' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/kernel/pid.c:314:198: error: call to '__compiletime_assert_314' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [kernel/pid.o] Error 1
In function 'arm_smmu_init_domain_context',
    inlined from 'arm_smmu_attach_dev' at /git/arm-soc/drivers/iommu/arm-smmu.c:1138:6:
/git/arm-soc/drivers/iommu/arm-smmu.c:877:198: error: call to '__compiletime_assert_877' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/iommu/arm-smmu.c:916:198: error: call to '__compiletime_assert_916' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/iommu/arm-smmu.c: In function 'arm_smmu_attach_dev':
/git/arm-soc/drivers/iommu/arm-smmu.c:1139:199: error: call to '__compiletime_assert_1139' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/iommu/arm-smmu.o] Error 1
/git/arm-soc/drivers/dma/sun4i-dma.c: In function 'generate_ndma_promise':
/git/arm-soc/drivers/dma/sun4i-dma.c:464:198: error: call to '__compiletime_assert_464' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/dma/sun4i-dma.c: In function 'generate_ddma_promise':
/git/arm-soc/drivers/dma/sun4i-dma.c:521:198: error: call to '__compiletime_assert_521' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/dma/sun4i-dma.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/iommu] Error 2
In function 'pl011_probe_dt_alias',
    inlined from 'pl011_setup_port' at /git/arm-soc/drivers/tty/serial/amba-pl011.c:2414:8:
/git/arm-soc/drivers/tty/serial/amba-pl011.c:2360:199: error: call to '__compiletime_assert_2360' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/tty/serial/amba-pl011.o] Error 1
/git/arm-soc/drivers/tty/serial/clps711x.c: In function 'uart_clps711x_probe':
/git/arm-soc/drivers/tty/serial/clps711x.c:475:204: error: call to '__compiletime_assert_475' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/tty/serial/clps711x.o] Error 1
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:252: error: data definition has no type or storage class [-Werror]
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:252: error: type defaults to 'int' in declaration of 'device_initcall' [-Werror=implicit-int]
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:1: error: parameter names (without types) in function declaration [-Werror]
 builtin_platform_driver(ingenic_uart_platform_driver);
 ^
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:122: error: 'ingenic_uart_platform_driver_init' defined but not used [-Werror=unused-function]
cc1: all warnings being treated as errors
make[5]: *** [drivers/tty/serial/8250/8250_ingenic.o] Error 1
make[5]: Target `__build' not remade because of errors.
make[4]: *** [drivers/tty/serial/8250] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/dma] Error 2
/git/arm-soc/drivers/mfd/twl4030-irq.c: In function 'twl4030_init_irq':
/git/arm-soc/drivers/mfd/twl4030-irq.c:699:203: error: call to '__compiletime_assert_699' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/mfd/twl4030-irq.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/tty/serial] Error 2
/git/arm-soc/fs/afs/write.c: In function 'afs_file_write':
/git/arm-soc/fs/afs/write.c:646:201: error: call to '__compiletime_assert_646' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [fs/afs/write.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [fs/afs] Error 2
In function 'da8xx_fb_config_clk_divider',
    inlined from 'da8xx_fb_calc_config_clk_divider' at /git/arm-soc/drivers/video/fbdev/da8xx-fb.c:769:9:
/git/arm-soc/drivers/video/fbdev/da8xx-fb.c:717:199: error: call to '__compiletime_assert_717' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
In function 'lcd_init',
    inlined from 'da8xxfb_set_par' at /git/arm-soc/drivers/video/fbdev/da8xx-fb.c:1284:6:
/git/arm-soc/drivers/video/fbdev/da8xx-fb.c:788:198: error: call to '__compiletime_assert_788' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/video/fbdev/da8xx-fb.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/tty] Error 2
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/video/fbdev] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/video] Error 2
/git/arm-soc/drivers/cpufreq/omap-cpufreq.c: In function 'omap_target':
/git/arm-soc/drivers/cpufreq/omap-cpufreq.c:54:197: error: call to '__compiletime_assert_54' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/cpufreq/omap-cpufreq.o] Error 1
In function 'mmc_select_hs200',
    inlined from 'mmc_select_timing' at /git/arm-soc/drivers/mmc/core/mmc.c:1310:7,
    inlined from 'mmc_init_card' at /git/arm-soc/drivers/mmc/core/mmc.c:1565:6:
/git/arm-soc/drivers/mmc/core/mmc.c:1271:200: error: call to '__compiletime_assert_1271' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/core/mmc.c: In function 'mmc_init_card':
/git/arm-soc/drivers/mmc/core/mmc.c:1580:201: error: call to '__compiletime_assert_1580' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/core/mmc.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/cpufreq] Error 2
/git/arm-soc/drivers/crypto/caam/ctrl.c: In function 'caam_get_era':
/git/arm-soc/drivers/crypto/caam/ctrl.c:405:201: error: call to '__compiletime_assert_405' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/crypto/caam/ctrl.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/mmc/core] Error 2
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/crypto/caam] Error 2
In file included from /git/arm-soc/include/linux/clk.h:15:0,
                 from /git/arm-soc/drivers/crypto/ux500/cryp/cryp_core.c:12:
/git/arm-soc/include/linux/err.h: In function 'IS_ERR':
/git/arm-soc/include/linux/err.h:35:215: error: call to '__compiletime_assert_35' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[5]: *** [drivers/crypto/ux500/cryp/cryp_core.o] Error 1
make[5]: Target `__build' not remade because of errors.
make[4]: *** [drivers/crypto/ux500/cryp] Error 2
/git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_do_get_cd':
/git/arm-soc/drivers/mmc/host/sdhci.c:1628:204: error: call to '__compiletime_assert_1628' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_add_host':
/git/arm-soc/drivers/mmc/host/sdhci.c:3117:222: error: call to '__compiletime_assert_3117' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/host/sdhci.o] Error 1
In file included from /git/arm-soc/include/linux/clk.h:15:0,
                 from /git/arm-soc/drivers/crypto/ux500/hash/hash_core.c:16:
/git/arm-soc/include/linux/err.h: In function 'IS_ERR':
/git/arm-soc/include/linux/err.h:35:215: error: call to '__compiletime_assert_35' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[5]: *** [drivers/crypto/ux500/hash/hash_core.o] Error 1
make[5]: Target `__build' not remade because of errors.
make[4]: *** [drivers/crypto/ux500/hash] Error 2
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/crypto/ux500] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/crypto] Error 2
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x01.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x01.o] Error 1
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x02.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x02.o] Error 1
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x04.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x04.o] Error 1
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x05.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x05.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/gpu/host1x] Error 2
/git/arm-soc/drivers/mmc/host/dw_mmc.c: In function 'dw_mci_get_ro':
/git/arm-soc/drivers/mmc/host/dw_mmc.c:1434:204: error: call to '__compiletime_assert_1434' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/host/dw_mmc.c: In function 'dw_mci_get_cd':
/git/arm-soc/drivers/mmc/host/dw_mmc.c:1457:209: error: call to '__compiletime_assert_1457' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/host/dw_mmc.c: In function 'dw_mci_enable_cd':
/git/arm-soc/drivers/mmc/host/dw_mmc.c:2930:223: error: call to '__compiletime_assert_2930' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/host/dw_mmc.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/mfd] Error 2
/git/arm-soc/fs/gfs2/dir.c: In function 'get_first_leaf':
/git/arm-soc/fs/gfs2/dir.c:801:201: error: call to '__compiletime_assert_801' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/fs/gfs2/dir.c: In function 'dir_split_leaf':
/git/arm-soc/fs/gfs2/dir.c:1017:201: error: call to '__compiletime_assert_1017' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [fs/gfs2/dir.o] Error 1
In function 'sdhci_esdhc_imx_probe_dt',
    inlined from 'sdhci_esdhc_imx_probe' at /git/arm-soc/drivers/mmc/host/sdhci-esdhc-imx.c:1213:7:
/git/arm-soc/drivers/mmc/host/sdhci-esdhc-imx.c:1014:223: error: call to '__compiletime_assert_1014' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/host/sdhci-esdhc-imx.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/mmc/host] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/mmc] Error 2
In function 'highbank_set_em_messages',
    inlined from 'ahci_highbank_probe' at /git/arm-soc/drivers/ata/sata_highbank.c:537:2:
/git/arm-soc/drivers/ata/sata_highbank.c:200:199: error: call to '__compiletime_assert_200' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/ata/sata_highbank.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [fs/gfs2] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [kernel] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/ata] Error 2
/git/arm-soc/drivers/nvmem/core.c: In function 'bin_attr_nvmem_write':
/git/arm-soc/drivers/nvmem/core.c:105:197: error: call to '__compiletime_assert_105' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'bin_attr_nvmem_read':
/git/arm-soc/drivers/nvmem/core.c:80:196: error: call to '__compiletime_assert_80' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function '__nvmem_cell_read':
/git/arm-soc/drivers/nvmem/core.c:824:197: error: call to '__compiletime_assert_824' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_cell_read':
/git/arm-soc/drivers/nvmem/core.c:972:197: error: call to '__compiletime_assert_972' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c:976:197: error: call to '__compiletime_assert_976' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_read':
/git/arm-soc/drivers/nvmem/core.c:1031:198: error: call to '__compiletime_assert_1031' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_write':
/git/arm-soc/drivers/nvmem/core.c:1059:198: error: call to '__compiletime_assert_1059' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_cell_write':
/git/arm-soc/drivers/nvmem/core.c:944:197: error: call to '__compiletime_assert_944' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_cell_write':
/git/arm-soc/drivers/nvmem/core.c:1002:198: error: call to '__compiletime_assert_1002' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
In function 'nvmem_add_cells',
    inlined from 'nvmem_register' at /git/arm-soc/drivers/nvmem/core.c:365:3:
/git/arm-soc/drivers/nvmem/core.c:277:200: error: call to '__compiletime_assert_277' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_cell_read':
/git/arm-soc/drivers/nvmem/core.c:859:197: error: call to '__compiletime_assert_859' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/nvmem/core.o] Error 1
make[3]: Target `__build' not remade because of errors.

[toc] | [prev] | [next] | [standalone]

#1329840 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Mon, Feb 08, 2016 at 01:01:32PM +0100, Arnd Bergmann wrote:

> /git/arm-soc/fs/afs/write.c: In function 'afs_file_write':
> /git/arm-soc/fs/afs/write.c:646:201: error: call to '__compiletime_assert_646' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
> make[4]: *** [fs/afs/write.o] Error 1
> make[4]: Target `__build' not remade because of errors.
> make[3]: *** [fs/afs] Error 2

... and taking a look at that code, we see this:
        if (IS_ERR_VALUE(result)) {
                _leave(" = %zd", result);
                return result;
        }

        _leave(" = %zd", result);
        return result;

Further grep for that shows e.g.
        /* determine the length of the filename */
        nlen = romfs_dev_strnlen(sb, pos + ROMFH_SIZE, ROMFS_MAXFN);
        if (IS_ERR_VALUE(nlen))
                goto eio;
which is also fucked in head, seeing that nlen should simply have been
int there, with check being if (nlen < 0).  But that would've been
insufficiently future-proof, I suppose - what if somebody wishes to modify
romfs to allow 2Gb-long file names?

For sarcasm-impaired: use of IS_ERR_VALUE turns out to be a nice red flag.
Often of the "I'm too lazy to check the calling conventions, let's gamble on
IS_ERR_VALUE doing no harm and to hell with whoever will be scratching head
reading the results" variety...

[toc] | [prev] | [next] | [standalone]

#1329965 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

+cc Rasmus Villemoes, I forgot to add him earlier.

On 02/08/2016 01:01 PM, Arnd Bergmann wrote:
> On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
>> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
>>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>> My version produces shortest code, Arnd's is the same as the old one.
>> On the other side Rasmus proposition seems to be the most straightforward
>> to me. Anyway I am not sure if the code length is the most important here.
>>
>> By the way .data segment size grows almost 4 times between gcc 4.4 and
>> 4.8 :)
>> Also numbers for arm64 looks interesting.
>>
>> Just for the record below all proposed implementations:
>> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
>>                                 ? unlikely((x) <= -1) \
>>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
>> (unsigned long long)(typeof(x))-MAX_ERRNO))
>> #define IS_ERR_VALUE_rasmus(x) ({\
>>         typeof(x) _x = (x);\
>>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
>> })
>>
>>> Andrzej's version is a little shorter on ARM because in case of signed numbers
>>> it only checks for negative values, rather than checking for values in the
>>> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
>>> in this case, and my version restores it.
>> As I looked at the usage of the macro in the kernel I have not found any
>> code
>> which could benefit from the original behavior, except some buggy code in
>> staging which have already pending fix[1].
>> But maybe it would be better to use IS_ERR_VALUE to always check if err
>> is in
>> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
>> signed types.
> If we do that, should we also make it illegal to use an invalid type
> for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
> char' that are still broken.
I meant rather to make such 'policy' for future code by adding some
comment to the macro. Optionally adding compile time warning
to encourage developers to change current usage, however I am
not sure if it is not too harsh.
This way it could be also good to use your version of the macro.
It could be also good to add compiletime_assert to prevent char types
as suggested by Rasmus.

Finally it could look like:
/*
 * Use IS_ERR_VALUE only on unsigned types of at least two bytes size.
 * For signed types use '< 0' comparison.
 */
#define IS_ERR_VALUE(x)\
({\
        compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle
byte-size types");\
        compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE
should be called on unsigned types only, use '< 0' instead");\
        (unlikely((unsigned long long)(x) >= (unsigned long
long)(typeof(x))-MAX_ERRNO));\
})

Minor issue: there are no compile-time warning macros in kernel.
Helper provided by gcc (warning attribute) is not so nice:
optimizations removes the warning itself, preventing optimization
influences final code.
I have put my proposition of workaround below:

#define compiletime_assert_warning(cond, msg) \
({ \
        __maybe_unused void const *p = (cond) ? 0 : 1; \
})

On older compilers it issues just warning:

drivers/nvmem/core.c:1059: warning: initialization makes pointer from
integer without a cast

Since gcc 4.8 it is more verbose:

drivers/nvmem/core.c: In function ‘nvmem_device_write’:
include/linux/err.h:33:33: warning: initialization makes pointer from
integer without a cast [enabled by default]
  __maybe_unused void const *p = (cond) ? 0 : 1; \
                                 ^
include/linux/err.h:41:2: note: in expansion of macro
‘compiletime_assert_warning’
  compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE should
be called on unsigned types only, use '< 0' instead");\
  ^
drivers/nvmem/core.c:1059:6: note: in expansion of macro ‘IS_ERR_VALUE’
  if (IS_ERR_VALUE(rc))


Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1327352 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Wed, Feb 03 2016, Andrzej Hajda <a.hajda@samsung.com> wrote:

> Current implementation of IS_ERR_VALUE works correctly only with
> following types:
> - unsigned long,
> - short, int, long.
> Other types are handled incorrectly either on 32-bit either on 64-bit
> either on both architectures.
> The patch fixes it by comparing argument with MAX_ERRNO casted
> to argument's type for unsigned types and comparing with zero for signed
> types. As a result all integer types bigger than char are handled properly.
>
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> v3:
>  - use '<= -1' instead of '< 0' to silence verbose warnings for gcc
>    older than 4.8,
> v2:
>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>  - expand commit message.
> ---
>  include/linux/err.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..b7d4a9f 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) <= -1) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  

I'm a bit worried that you consider any negative value an error when x
is signed - at least that's a change which deserves some comment why
that's ok. For example, I could imagine someone using e.g. INT_MIN as a
sentinel return value meaning 'not an error, but something special
still'.

I think that, since we're there, one should stick in a BUILD_BUG_ON to
catch people passing in a u8 or s8.

Something that seems that it would work for all (wide enough) types
is

({
typeof(x) _x = (x);
BUILD_BUG_ON(sizeof(_x) < 2);
unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);
})

Whether x is signed or not we want the first condition. If x is unsigned
and at least as wide as int, the -1 in the second condition will be
promoted to the max value typeof(x) can hold, so gcc should trivially
remove the check, and if x is signed, this keeps the previous
semantics. It's also a pattern I expect gcc to be able to optimize
pretty well (that is, if x has type signed int, I expect it'll recognize
that these tests are better done as a single unsigned comparison). Maybe
it'll still give a -Wtype-limit warning for the unsigned case.

If we go with the above, maybe one should also stick in something which
ensures x is not a pointer; (void)(_x+_x); should do it. 

Rasmus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web