| Message ID | 20180208233829.GA16128@ziepe.ca (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Leon Romanovsky |
| Headers | show |
On Thu, 2018-02-08 at 16:38 -0700, Jason Gunthorpe wrote: > enum { > - 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? Thanks, Bart.
On Fri, Feb 09, 2018 at 04:01:51PM +0000, Bart Van Assche wrote: > On Thu, 2018-02-08 at 16:38 -0700, Jason Gunthorpe wrote: > > enum { > > - 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? 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.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe > Sent: Friday, February 09, 2018 10:11 AM > To: Bart Van Assche <Bart.VanAssche@wdc.com> > Cc: linux-rdma@vger.kernel.org > Subject: Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions > > On Fri, Feb 09, 2018 at 04:01:51PM +0000, Bart Van Assche wrote: > > On Thu, 2018-02-08 at 16:38 -0700, Jason Gunthorpe wrote: > > > enum { > > > - 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? > > 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.. > Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote: > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gRnJpLCAyMDE4LTAyLTA5IGF0IDE2OjEyICswMDAwLCBQYXJhdiBQYW5kaXQgd3JvdGU6DQo+ IEFueSByZWFzb24gdG8gdXNlIG9wZW4gY29kZSBpbnN0ZWFkIG9mIHVzaW5nIEJJVCgpIG1hY3Jv IGZyb20NCj4gaW5jbHVkZS9saW51eC9iaXRvcHMuaCB3aGljaCBkb2VzIHRoZSBzYW1lIHRoaW5n Pw0KDQpQZXJzb25hbGx5IEknbSBpbiBmYXZvciBvZiByZW1vdmluZyB0aGUgQklUKCkgbWFjcm8g ZnJvbSB0aGUga2VybmVsLiBUaGF0IG1hY3JvDQppcyBub3QgdXNlZnVsIGFuZCBvbmx5IGFkZHMg Y29uZnVzaW9uLiBDb2RlIHRoYXQgZG9lcyBub3QgdXNlIHRoYXQgbWFjcm8gaXMNCmVhc2llciB0 byByZWFkIGJlY2F1c2Ugb25lIGRvZXMgbm90IGhhdmUgdG8gbG9vayB1cCB3aGV0aGVyIHRoZSBC SVQoKSBtYWNybw0KaXMgZGVmaW5lZCBhcyAxVSA8PCB4LCAxVUwgPDwgeCBvciAxVUxMIDw8IHgu DQoNCkJhcnQuDQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > Sent: Friday, February 09, 2018 10:13 AM > To: Parav Pandit <parav@mellanox.com> > Cc: Bart Van Assche <Bart.VanAssche@wdc.com>; linux-rdma@vger.kernel.org > Subject: Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions > > On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote: > > > > 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. > o.k. Got it. Same duplicated definition exist in kernel too which is not yet fixed because its unused, I guess. Likely user space headers don't have any BIT() or support macros. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 09, 2018 at 04:19:23PM +0000, Bart Van Assche wrote: > On Fri, 2018-02-09 at 16:12 +0000, Parav Pandit wrote: > > 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? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 09, 2018 at 09:13:25AM -0700, Jason Gunthorpe wrote: > On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote: > > > > 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. ➜ rdma-core git:(master) git grep "define BIT" providers/mlx5/cq.c providers/mlx5/cq.c:#define BIT(i) (1UL << (i)) Thanks > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 09, 2018 at 06:42:00PM +0200, Leon Romanovsky wrote: > On Fri, Feb 09, 2018 at 09:13:25AM -0700, Jason Gunthorpe wrote: > > On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote: > > > > > > 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. > > ➜ 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. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 09, 2018 at 09:51:38AM -0700, Jason Gunthorpe wrote: > On Fri, Feb 09, 2018 at 06:42:00PM +0200, Leon Romanovsky wrote: > > On Fri, Feb 09, 2018 at 09:13:25AM -0700, Jason Gunthorpe wrote: > > > On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote: > > > > > > > > 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. > > > > ➜ 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. Thanks, I missed that we are talking about mlx4dv.h Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On 02/09/18 08:24, Jason Gunthorpe wrote: > On Fri, Feb 09, 2018 at 04:19:23PM +0000, Bart Van Assche wrote: >> On Fri, 2018-02-09 at 16:12 +0000, Parav Pandit wrote: >>> 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? That's something additional kernel developers have to memorize. If the BIT() macro is not used but the shift operation is explicit then that saves the lookup of the BIT() definition in case one would have forgotten what the BIT() definition looks like. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/providers/mlx4/mlx4dv.h b/providers/mlx4/mlx4dv.h index d47d3cd2e1f135..5312a866b6e281 100644 --- a/providers/mlx4/mlx4dv.h +++ b/providers/mlx4/mlx4dv.h @@ -288,12 +288,12 @@ enum { }; enum { - MLX4_WQE_BIND_TYPE_2 = (1<<31), + MLX4_WQE_BIND_TYPE_2 = (1UL<<31), MLX4_WQE_BIND_ZERO_BASED = (1<<30), }; enum { - MLX4_INLINE_SEG = 1 << 31, + MLX4_INLINE_SEG = 1UL << 31, MLX4_INLINE_ALIGN = 64, }; @@ -304,7 +304,7 @@ enum { enum { MLX4_WQE_MW_REMOTE_READ = 1 << 29, MLX4_WQE_MW_REMOTE_WRITE = 1 << 30, - MLX4_WQE_MW_ATOMIC = 1 << 31 + MLX4_WQE_MW_ATOMIC = 1UL << 31 }; struct mlx4_wqe_local_inval_seg {
They produce the value INT32_MIN not 0x80000000. This is sometimes OK if it is casted appropriately but leaves a subtle trap to the user. Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> --- providers/mlx4/mlx4dv.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)