mlx4: Fix 1<<31 expressions

providers/mlx4/mlx4dv.h enum { · line 293

Quoted 5 lines · Jason Gunthorpe

enum {

providers/mlx4/mlx4dv.h +1 -1

-	MLX4_WQE_BIND_TYPE_2		= (1<<31),
+	MLX4_WQE_BIND_TYPE_2		= (1UL<<31),
 	MLX4_WQE_BIND_ZERO_BASED	= (1<<30),
 };

Hello Jason,

A quote from the C11 standard:

"The expression that defines the value of an enumeration constant shall be an integer constant expression that has a value representable as an int. [ ... ] Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined but shall be capable of representing the values of all the members of the enumeration."

Does that mean that this change relies on extensions to the C standard?

-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Jason Gunthorpe
Sent: Friday, February 09, 2018 10:11 AM
To: Bart Van Assche <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

Quoted 11 lines · Bart Van Assche

providers/mlx4/mlx4dv.h enum { · line 293

Quoted 5 lines · Jason Gunthorpe

enum {

providers/mlx4/mlx4dv.h +1 -1

-	MLX4_WQE_BIND_TYPE_2		= (1<<31),
+	MLX4_WQE_BIND_TYPE_2		= (1UL<<31),
 	MLX4_WQE_BIND_ZERO_BASED	= (1<<30),
 };

Hello Jason,

A quote from the C11 standard:

Quoted 8 lines

"The expression that defines the value of an enumeration constant shall be an integer constant expression that has a value representable as an int. [ ... ] Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined but shall be capable of representing the values of all the members of the enumeration."

Does that mean that this change relies on extensions to the C standard?

Yes, this relies on a common compiler extension to promote the type of enum constants beyond int. AFAIK all compilers have done this forever, kinda bonkers it isn't in C11, IMHO.

As I understand it, the current code is also non-conforming as (1<<31) is apparently undefined behavior, and at least gcc throws a warning for it in - Wpedantic mode.

I did a patch that will make our public headers mostly strictly conforming to C11 with -Wpedantic set, which I've been wondering if it is worth sending..

Quoted 2 lines

I did a patch that will make our public headers mostly strictly conforming to C11 with -Wpedantic set, which I've been wondering if it is worth sending..

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Friday, February 09, 2018 10:13 AM
To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc: Bart Van Assche <[email protected]>; [email protected]
Subject: Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote:

Quoted 2 lines

Quoted 2 lines

I did a patch that will make our public headers mostly strictly conforming to C11 with -Wpedantic set, which I've been wondering if it is

worth sending..

Quoted 2 lines

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

BIT only works in the kernel. This is a publich header from rdma-core userspace.

Quoted 5 lines · Parav Pandit

Quoted 2 lines

I did a patch that will make our public headers mostly strictly conforming to C11 with -Wpedantic set, which I've been wondering if it is worth sending..

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

BIT only works in the kernel. This is a publich header from rdma-core userspace.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Jason Gunthorpe

Quoted 2 lines · Parav Pandit

Quoted 2 lines

I did a patch that will make our public headers mostly strictly conforming to C11 with -Wpedantic set, which I've been wondering if it is worth sending..

Quoted 5 lines

Quoted 2 lines

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

BIT only works in the kernel. This is a publich header from rdma-core userspace.

➜ rdma-core git:(master) git grep "define BIT" providers/mlx5/cq.c providers/mlx5/cq.c:#define BIT(i) (1UL << (i))

Quoted 9 lines · Leon Romanovsky

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Parav Pandit

Quoted 2 lines

I did a patch that will make our public headers mostly strictly conforming to C11 with -Wpedantic set, which I've been wondering if it is worth sending..

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

Quoted 5 lines

Quoted 2 lines

BIT only works in the kernel. This is a publich header from rdma-core userspace.

➜ rdma-core git:(master) git grep "define BIT" providers/mlx5/cq.c providers/mlx5/cq.c:#define BIT(i) (1UL << (i))

Still can't use it in a public header.

Quoted 2 lines · Parav Pandit

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

Personally I'm in favor of removing the BIT() macro from the kernel. That macro is not useful and only adds confusion. Code that does not use that macro is easier to read because one does not have to look up whether the BIT() macro is defined as 1U << x, 1UL << x or 1ULL << x.

Quoted 8 lines · Bart Van Assche

Quoted 2 lines · Parav Pandit

Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

Personally I'm in favor of removing the BIT() macro from the kernel. That macro is not useful and only adds confusion. Code that does not use that macro is easier to read because one does not have to look up whether the BIT() macro is defined as 1U << x, 1UL << x or 1ULL << x.

Really? The rule is pretty simple:

  BIT(x) for x < 32
  BIT_ULL(x) for x >= 32 && x < 64

Why do you care about what specific type the macro produces?