| Message ID | 20180924195716.30848-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/mlx4: Avoid implicit enumerated type conversion | expand |
On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns when one enumerated type is implicitly converted to another. > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > type 'enum ib_qp_create_flags' [-Wenum-conversion] > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > type 'enum ib_qp_create_flags' [-Wenum-conversion] > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > ~ ^~~~~~~~~~~~~~~~~ > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > type ib_qp_create_flags. Make that clear to Clang by changing the > create_flags type to u32. > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > include/rdma/ib_verbs.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index e463d3007a35..f6f4d9e3c8ed 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > struct ib_qp_cap cap; > enum ib_sig_type sq_sig_type; > enum ib_qp_type qp_type; > - enum ib_qp_create_flags create_flags; > + u32 create_flags; I think it might be better to just have explicit casts at the assignment. What do the maintainers think? > > /* > * Only needed for special QP types, or when using the RW API. > -- > 2.19.0 >
On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > ~ ^~~~~~~~~~~~~~~~~ > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > type ib_qp_create_flags. Make that clear to Clang by changing the > > create_flags type to u32. > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > include/rdma/ib_verbs.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > index e463d3007a35..f6f4d9e3c8ed 100644 > > --- a/include/rdma/ib_verbs.h > > +++ b/include/rdma/ib_verbs.h > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > struct ib_qp_cap cap; > > enum ib_sig_type sq_sig_type; > > enum ib_qp_type qp_type; > > - enum ib_qp_create_flags create_flags; > > + u32 create_flags; > > I think it might be better to just have explicit casts at the > assignment. What do the maintainers think? > That's fine with me, I tend to explicitly cast if it is only one location but it certainly makes sense in this case as well. I'll wait for the maintainers to weigh in before sending a v2. Thanks for the review, Nathan > > > > /* > > * Only needed for special QP types, or when using the RW API. > > -- > > 2.19.0 > > > > > -- > Thanks, > ~Nick Desaulniers
On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > create_flags type to u32. > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > --- > > > include/rdma/ib_verbs.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > --- a/include/rdma/ib_verbs.h > > > +++ b/include/rdma/ib_verbs.h > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > struct ib_qp_cap cap; > > > enum ib_sig_type sq_sig_type; > > > enum ib_qp_type qp_type; > > > - enum ib_qp_create_flags create_flags; > > > + u32 create_flags; > > > > I think it might be better to just have explicit casts at the > > assignment. What do the maintainers think? > > > > That's fine with me, I tend to explicitly cast if it is only one > location but it certainly makes sense in this case as well. I'll > wait for the maintainers to weigh in before sending a v2. Yeah, I mean my opinion on this might seem arbitrary, but based on the pattern and the comment in ib_qp_create_flags, those enum values are reserved to be "subclassed" in a sense, so they should always be in sync or this code will have bigger problems. > > Thanks for the review, > Nathan > > > > > > > /* > > > * Only needed for special QP types, or when using the RW API. > > > -- > > > 2.19.0 > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers
On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > create_flags type to u32. > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > include/rdma/ib_verbs.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > +++ b/include/rdma/ib_verbs.h > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > struct ib_qp_cap cap; > > > > enum ib_sig_type sq_sig_type; > > > > enum ib_qp_type qp_type; > > > > - enum ib_qp_create_flags create_flags; > > > > + u32 create_flags; > > > > > > I think it might be better to just have explicit casts at the > > > assignment. What do the maintainers think? > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > location but it certainly makes sense in this case as well. I'll > > wait for the maintainers to weigh in before sending a v2. > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > pattern and the comment in ib_qp_create_flags, those enum values are > reserved to be "subclassed" in a sense, so they should always be in > sync or this code will have bigger problems. One should not use an 'enum' type name for bitfield storage, as once you start or'ing things together the values no longer fall on the enum. Some compilers and tools even give warnings in this case, ie enum x foo = X_A | X_B; Is an assignment from 'int' to an 'enum x' with an implicit cast. For this reason, usually bitfield enum declarations should be anonymous. Jason
On Mon, Sep 24, 2018 at 08:37:22PM -0600, Jason Gunthorpe wrote: > On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > > create_flags type to u32. > > > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > include/rdma/ib_verbs.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > > +++ b/include/rdma/ib_verbs.h > > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > > struct ib_qp_cap cap; > > > > > enum ib_sig_type sq_sig_type; > > > > > enum ib_qp_type qp_type; > > > > > - enum ib_qp_create_flags create_flags; > > > > > + u32 create_flags; > > > > > > > > I think it might be better to just have explicit casts at the > > > > assignment. What do the maintainers think? > > > > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > > location but it certainly makes sense in this case as well. I'll > > > wait for the maintainers to weigh in before sending a v2. > > > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > > pattern and the comment in ib_qp_create_flags, those enum values are > > reserved to be "subclassed" in a sense, so they should always be in > > sync or this code will have bigger problems. > > One should not use an 'enum' type name for bitfield storage, as once > you start or'ing things together the values no longer fall on the > enum. Some compilers and tools even give warnings in this case, ie > > enum x foo = X_A | X_B; > > Is an assignment from 'int' to an 'enum x' with an implicit cast. > > For this reason, usually bitfield enum declarations should be > anonymous. > > Jason Hi Jason, I apologize for not understanding but how should I adjust my patch so that it is acceptable? Do you want the explicit casts like Nick suggested? Thanks for the comments, Nathan
On Wed, Sep 26, 2018 at 06:08:03PM -0700, Nathan Chancellor wrote: > On Mon, Sep 24, 2018 at 08:37:22PM -0600, Jason Gunthorpe wrote: > > On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > > > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > > > create_flags type to u32. > > > > > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > include/rdma/ib_verbs.h | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > > > +++ b/include/rdma/ib_verbs.h > > > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > > > struct ib_qp_cap cap; > > > > > > enum ib_sig_type sq_sig_type; > > > > > > enum ib_qp_type qp_type; > > > > > > - enum ib_qp_create_flags create_flags; > > > > > > + u32 create_flags; > > > > > > > > > > I think it might be better to just have explicit casts at the > > > > > assignment. What do the maintainers think? > > > > > > > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > > > location but it certainly makes sense in this case as well. I'll > > > > wait for the maintainers to weigh in before sending a v2. > > > > > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > > > pattern and the comment in ib_qp_create_flags, those enum values are > > > reserved to be "subclassed" in a sense, so they should always be in > > > sync or this code will have bigger problems. > > > > One should not use an 'enum' type name for bitfield storage, as once > > you start or'ing things together the values no longer fall on the > > enum. Some compilers and tools even give warnings in this case, ie > > > > enum x foo = X_A | X_B; > > > > Is an assignment from 'int' to an 'enum x' with an implicit cast. > > > > For this reason, usually bitfield enum declarations should be > > anonymous. > > > > Jason > > Hi Jason, > > I apologize for not understanding but how should I adjust my patch so > that it is acceptable? Do you want the explicit casts like Nick > suggested? No, I think your original patch is fine, I was waiting to see if you or Nick disagreed with my assessment on bitfields.. Jason
On Wed, Sep 26, 2018 at 9:48 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Wed, Sep 26, 2018 at 06:08:03PM -0700, Nathan Chancellor wrote: > > On Mon, Sep 24, 2018 at 08:37:22PM -0600, Jason Gunthorpe wrote: > > > On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > > > > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > > > > create_flags type to u32. > > > > > > > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > include/rdma/ib_verbs.h | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > > > > +++ b/include/rdma/ib_verbs.h > > > > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > > > > struct ib_qp_cap cap; > > > > > > > enum ib_sig_type sq_sig_type; > > > > > > > enum ib_qp_type qp_type; > > > > > > > - enum ib_qp_create_flags create_flags; > > > > > > > + u32 create_flags; > > > > > > > > > > > > I think it might be better to just have explicit casts at the > > > > > > assignment. What do the maintainers think? > > > > > > > > > > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > > > > location but it certainly makes sense in this case as well. I'll > > > > > wait for the maintainers to weigh in before sending a v2. > > > > > > > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > > > > pattern and the comment in ib_qp_create_flags, those enum values are > > > > reserved to be "subclassed" in a sense, so they should always be in > > > > sync or this code will have bigger problems. > > > > > > One should not use an 'enum' type name for bitfield storage, as once > > > you start or'ing things together the values no longer fall on the > > > enum. Some compilers and tools even give warnings in this case, ie > > > > > > enum x foo = X_A | X_B; > > > > > > Is an assignment from 'int' to an 'enum x' with an implicit cast. > > > > > > For this reason, usually bitfield enum declarations should be > > > anonymous. > > > > > > Jason > > > > Hi Jason, > > > > I apologize for not understanding but how should I adjust my patch so > > that it is acceptable? Do you want the explicit casts like Nick > > suggested? > > No, I think your original patch is fine, I was waiting to see if you > or Nick disagreed with my assessment on bitfields.. It wasn't clear to me whether you were ack'ing or nack'ing the patch. If we don't change the patch to explicit casts, shouldn't the change be to make create_flags an int? (note: signedness)
On Thu, Sep 27, 2018 at 01:13:31PM -0700, Nick Desaulniers wrote: > On Wed, Sep 26, 2018 at 9:48 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Wed, Sep 26, 2018 at 06:08:03PM -0700, Nathan Chancellor wrote: > > > On Mon, Sep 24, 2018 at 08:37:22PM -0600, Jason Gunthorpe wrote: > > > > On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > > > > > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > > > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > > > > > create_flags type to u32. > > > > > > > > > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > include/rdma/ib_verbs.h | 2 +- > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > > > > > +++ b/include/rdma/ib_verbs.h > > > > > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > > > > > struct ib_qp_cap cap; > > > > > > > > enum ib_sig_type sq_sig_type; > > > > > > > > enum ib_qp_type qp_type; > > > > > > > > - enum ib_qp_create_flags create_flags; > > > > > > > > + u32 create_flags; > > > > > > > > > > > > > > I think it might be better to just have explicit casts at the > > > > > > > assignment. What do the maintainers think? > > > > > > > > > > > > > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > > > > > location but it certainly makes sense in this case as well. I'll > > > > > > wait for the maintainers to weigh in before sending a v2. > > > > > > > > > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > > > > > pattern and the comment in ib_qp_create_flags, those enum values are > > > > > reserved to be "subclassed" in a sense, so they should always be in > > > > > sync or this code will have bigger problems. > > > > > > > > One should not use an 'enum' type name for bitfield storage, as once > > > > you start or'ing things together the values no longer fall on the > > > > enum. Some compilers and tools even give warnings in this case, ie > > > > > > > > enum x foo = X_A | X_B; > > > > > > > > Is an assignment from 'int' to an 'enum x' with an implicit cast. > > > > > > > > For this reason, usually bitfield enum declarations should be > > > > anonymous. > > > > > > > > Jason > > > > > > Hi Jason, > > > > > > I apologize for not understanding but how should I adjust my patch so > > > that it is acceptable? Do you want the explicit casts like Nick > > > suggested? > > > > No, I think your original patch is fine, I was waiting to see if you > > or Nick disagreed with my assessment on bitfields.. > > It wasn't clear to me whether you were ack'ing or nack'ing the patch. > If we don't change the patch to explicit casts, shouldn't the change > be to make create_flags an int? (note: signedness) > > -- > Thanks, > ~Nick Desaulniers Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is signedness necessary? Nathan
On Thu, Sep 27, 2018 at 1:28 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Thu, Sep 27, 2018 at 01:13:31PM -0700, Nick Desaulniers wrote: > > On Wed, Sep 26, 2018 at 9:48 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Wed, Sep 26, 2018 at 06:08:03PM -0700, Nathan Chancellor wrote: > > > > On Mon, Sep 24, 2018 at 08:37:22PM -0600, Jason Gunthorpe wrote: > > > > > On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > > > > > > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > > > > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > > > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > > > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > > > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > > > > > > create_flags type to u32. > > > > > > > > > > > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > > include/rdma/ib_verbs.h | 2 +- > > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > > > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > > > > > > +++ b/include/rdma/ib_verbs.h > > > > > > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > > > > > > struct ib_qp_cap cap; > > > > > > > > > enum ib_sig_type sq_sig_type; > > > > > > > > > enum ib_qp_type qp_type; > > > > > > > > > - enum ib_qp_create_flags create_flags; > > > > > > > > > + u32 create_flags; > > > > > > > > > > > > > > > > I think it might be better to just have explicit casts at the > > > > > > > > assignment. What do the maintainers think? > > > > > > > > > > > > > > > > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > > > > > > location but it certainly makes sense in this case as well. I'll > > > > > > > wait for the maintainers to weigh in before sending a v2. > > > > > > > > > > > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > > > > > > pattern and the comment in ib_qp_create_flags, those enum values are > > > > > > reserved to be "subclassed" in a sense, so they should always be in > > > > > > sync or this code will have bigger problems. > > > > > > > > > > One should not use an 'enum' type name for bitfield storage, as once > > > > > you start or'ing things together the values no longer fall on the > > > > > enum. Some compilers and tools even give warnings in this case, ie > > > > > > > > > > enum x foo = X_A | X_B; > > > > > > > > > > Is an assignment from 'int' to an 'enum x' with an implicit cast. > > > > > > > > > > For this reason, usually bitfield enum declarations should be > > > > > anonymous. > > > > > > > > > > Jason > > > > > > > > Hi Jason, > > > > > > > > I apologize for not understanding but how should I adjust my patch so > > > > that it is acceptable? Do you want the explicit casts like Nick > > > > suggested? > > > > > > No, I think your original patch is fine, I was waiting to see if you > > > or Nick disagreed with my assessment on bitfields.. > > > > It wasn't clear to me whether you were ack'ing or nack'ing the patch. > > If we don't change the patch to explicit casts, shouldn't the change > > be to make create_flags an int? (note: signedness) > > > > -- > > Thanks, > > ~Nick Desaulniers > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > signedness necessary? enums are by default restricted to the range of ints. It's not right to tie these "anonymous enums" to a specific width and signedness other than int.
On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > On Thu, Sep 27, 2018 at 1:28 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Thu, Sep 27, 2018 at 01:13:31PM -0700, Nick Desaulniers wrote: > > > On Wed, Sep 26, 2018 at 9:48 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > On Wed, Sep 26, 2018 at 06:08:03PM -0700, Nathan Chancellor wrote: > > > > > On Mon, Sep 24, 2018 at 08:37:22PM -0600, Jason Gunthorpe wrote: > > > > > > On Mon, Sep 24, 2018 at 03:29:38PM -0700, Nick Desaulniers wrote: > > > > > > > On Mon, Sep 24, 2018 at 3:27 PM Nathan Chancellor > > > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > > > > > On Mon, Sep 24, 2018 at 03:24:36PM -0700, Nick Desaulniers wrote: > > > > > > > > > On Mon, Sep 24, 2018 at 12:57 PM Nathan Chancellor > > > > > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > > > > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > > > > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > > > > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > > > > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > > > > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > > > > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > > > > > > > create_flags type to u32. > > > > > > > > > > > > > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > > > include/rdma/ib_verbs.h | 2 +- > > > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > > > > > > > > index e463d3007a35..f6f4d9e3c8ed 100644 > > > > > > > > > > +++ b/include/rdma/ib_verbs.h > > > > > > > > > > @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { > > > > > > > > > > struct ib_qp_cap cap; > > > > > > > > > > enum ib_sig_type sq_sig_type; > > > > > > > > > > enum ib_qp_type qp_type; > > > > > > > > > > - enum ib_qp_create_flags create_flags; > > > > > > > > > > + u32 create_flags; > > > > > > > > > > > > > > > > > > I think it might be better to just have explicit casts at the > > > > > > > > > assignment. What do the maintainers think? > > > > > > > > > > > > > > > > > > > > > > > > > That's fine with me, I tend to explicitly cast if it is only one > > > > > > > > location but it certainly makes sense in this case as well. I'll > > > > > > > > wait for the maintainers to weigh in before sending a v2. > > > > > > > > > > > > > > Yeah, I mean my opinion on this might seem arbitrary, but based on the > > > > > > > pattern and the comment in ib_qp_create_flags, those enum values are > > > > > > > reserved to be "subclassed" in a sense, so they should always be in > > > > > > > sync or this code will have bigger problems. > > > > > > > > > > > > One should not use an 'enum' type name for bitfield storage, as once > > > > > > you start or'ing things together the values no longer fall on the > > > > > > enum. Some compilers and tools even give warnings in this case, ie > > > > > > > > > > > > enum x foo = X_A | X_B; > > > > > > > > > > > > Is an assignment from 'int' to an 'enum x' with an implicit cast. > > > > > > > > > > > > For this reason, usually bitfield enum declarations should be > > > > > > anonymous. > > > > > > > > > > > > Jason > > > > > > > > > > Hi Jason, > > > > > > > > > > I apologize for not understanding but how should I adjust my patch so > > > > > that it is acceptable? Do you want the explicit casts like Nick > > > > > suggested? > > > > > > > > No, I think your original patch is fine, I was waiting to see if you > > > > or Nick disagreed with my assessment on bitfields.. > > > > > > It wasn't clear to me whether you were ack'ing or nack'ing the patch. > > > If we don't change the patch to explicit casts, shouldn't the change > > > be to make create_flags an int? (note: signedness) > > > > > > -- > > > Thanks, > > > ~Nick Desaulniers > > > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > signedness necessary? > > enums are by default restricted to the range of ints. It's not right > to tie these "anonymous enums" to a specific width and signedness > other than int. > > -- > Thanks, > ~Nick Desaulniers Thanks for the clarification, I will go ahead and send a v2 now. Nathan
On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > signedness necessary? > > enums are by default restricted to the range of ints. That's not quite right, the compiler sizes the enum to be able to fit the largest value contained within, today that is int, but if we added 1<<31, then it would become larger. Values intended to hold bitfields should always be unsigned, so u32 is an appropriate for flags where the flags are defined in an enum. Jason
On Thu, 2018-09-27 at 16:28 -0600, Jason Gunthorpe wrote: > On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > > > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > > signedness necessary? > > > > enums are by default restricted to the range of ints. > > That's not quite right, the compiler sizes the enum to be able to fit > the largest value contained within, today that is int, but if we added > 1<<31, then it would become larger. Hi Jason, Are you perhaps confusing C and C++? For C++, an enumeration whose underlying type is not fixed, the underlying type is an integral type that can represent all the enumerator values defined in the enumeration. For C however I think that enumeration values are restricted to what fits in an int. Bart.
On Thu, Sep 27, 2018 at 3:33 PM Bart Van Assche <bvanassche@acm.org> wrote: > > On Thu, 2018-09-27 at 16:28 -0600, Jason Gunthorpe wrote: > > On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > > > > > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > > > signedness necessary? > > > > > > enums are by default restricted to the range of ints. > > > > That's not quite right, the compiler sizes the enum to be able to fit > > the largest value contained within, today that is int, but if we added > > 1<<31, then it would become larger. > > Hi Jason, > > Are you perhaps confusing C and C++? For C++, an enumeration whose underlying > type is not fixed, the underlying type is an integral type that can represent > all the enumerator values defined in the enumeration. For C however I think > that enumeration values are restricted to what fits in an int. > > Bart. > To quote the sacred texts (ANSIIISO9899-1990): 6.5.2.2 Enumeration specifiers The expression that defines the value of an enumeration constant shall be an integral constant expression that has a value representable as an int.
On Thu, Sep 27, 2018 at 03:42:24PM -0700, Nick Desaulniers wrote: > On Thu, Sep 27, 2018 at 3:33 PM Bart Van Assche <bvanassche@acm.org> wrote: > > > > On Thu, 2018-09-27 at 16:28 -0600, Jason Gunthorpe wrote: > > > On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > > > > > > > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > > > > signedness necessary? > > > > > > > > enums are by default restricted to the range of ints. > > > > > > That's not quite right, the compiler sizes the enum to be able to fit > > > the largest value contained within, today that is int, but if we added > > > 1<<31, then it would become larger. > > > > Hi Jason, > > > > Are you perhaps confusing C and C++? For C++, an enumeration whose underlying > > type is not fixed, the underlying type is an integral type that can represent > > all the enumerator values defined in the enumeration. For C however I think > > that enumeration values are restricted to what fits in an int. > > > > Bart. > > > > To quote the sacred texts (ANSIIISO9899-1990): > 6.5.2.2 Enumeration specifiers > The expression that defines the value of an enumeration constant shall > be an integral constant > expression that has a value representable as an int. This is the wrong part of the standard to quote it is talking about *enumeration constants* not the 'enum X' itself. Anyhow, the standard is hard to read in this area, but reality is this: #include <stdio.h> enum a { A1 = 1, A2 = 1ULL<<40, }; int main(int argc, const char *argv[]) { printf("%zu\n", sizeof(enum a)); return 0; } $ gcc -Wall -std=c11 test.c && ./a.out 8 I forget if this a common compiler extension, unclear standard, or was formally revised in C11 or what, but it is the real world the Linux kernel lives in. It is even more confusing if you wonder what types A1 and A2 are! Jason
On Thu, 2018-09-27 at 16:58 -0600, Jason Gunthorpe wrote: > Anyhow, the standard is hard to read in this area, but reality is > this: > > #include <stdio.h> > > enum a > { > A1 = 1, > A2 = 1ULL<<40, > }; > > int main(int argc, const char *argv[]) > { > printf("%zu\n", sizeof(enum a)); > return 0; > } > > $ gcc -Wall -std=c11 test.c && ./a.out > 8 > > I forget if this a common compiler extension, unclear standard, or was > formally revised in C11 or what, but it is the real world the Linux > kernel lives in. > > It is even more confusing if you wonder what types A1 and A2 are! What's unfortunate is that gcc and sparse have different opinions about how to compile the above code. Even if gcc supports enumeration constants that exceed what fits in an int, sparse does not AFAIK. Bart.
On Thu, Sep 27, 2018 at 3:58 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Thu, Sep 27, 2018 at 03:42:24PM -0700, Nick Desaulniers wrote: > > On Thu, Sep 27, 2018 at 3:33 PM Bart Van Assche <bvanassche@acm.org> wrote: > > > > > > On Thu, 2018-09-27 at 16:28 -0600, Jason Gunthorpe wrote: > > > > On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > > > > > > > > > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > > > > > signedness necessary? > > > > > > > > > > enums are by default restricted to the range of ints. > > > > > > > > That's not quite right, the compiler sizes the enum to be able to fit > > > > the largest value contained within, today that is int, but if we added > > > > 1<<31, then it would become larger. > > > > > > Hi Jason, > > > > > > Are you perhaps confusing C and C++? For C++, an enumeration whose underlying > > > type is not fixed, the underlying type is an integral type that can represent > > > all the enumerator values defined in the enumeration. For C however I think > > > that enumeration values are restricted to what fits in an int. > > > > > > Bart. > > > > > > > To quote the sacred texts (ANSIIISO9899-1990): > > > 6.5.2.2 Enumeration specifiers > > The expression that defines the value of an enumeration constant shall > > be an integral constant > > expression that has a value representable as an int. > > This is the wrong part of the standard to quote it is talking about > *enumeration constants* not the 'enum X' itself. > > Anyhow, the standard is hard to read in this area, but reality is > this: You mean undefined behavior? > > #include <stdio.h> > > enum a > { > A1 = 1, > A2 = 1ULL<<40, > }; > > int main(int argc, const char *argv[]) > { > printf("%zu\n", sizeof(enum a)); > return 0; > } > > $ gcc -Wall -std=c11 test.c && ./a.out > 8 > > I forget if this a common compiler extension, unclear standard, or was > formally revised in C11 or what, but it is the real world the Linux > kernel lives in. > > It is even more confusing if you wonder what types A1 and A2 are! > > Jason This example is a strawman; we're talking about the minimum sizeof an enum when all initialized values are representable within an int, which is well defined behavior by the citation I cited earlier. The point is, unless you use __attribute__((packed)) on your enum, it will NEVER be *smaller* than an int for compilers and C standard that the Linux kernel cares about. And if you're going to throw type safety out the window by converting values from one enum to another, for storage you MUST use an int (anything larger as in your example is undefined behavior). I don't disagree with your point that values should be unsigned for bitwise operations, but it's not clean to reconcile that with converting values between different enums. I suggest explicit casts to unsigned types before bitwise operations. https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands
On Thu, Sep 27, 2018 at 05:55:43PM -0700, Nick Desaulniers wrote: > On Thu, Sep 27, 2018 at 3:58 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Thu, Sep 27, 2018 at 03:42:24PM -0700, Nick Desaulniers wrote: > > > On Thu, Sep 27, 2018 at 3:33 PM Bart Van Assche <bvanassche@acm.org> wrote: > > > > > > > > On Thu, 2018-09-27 at 16:28 -0600, Jason Gunthorpe wrote: > > > > > On Thu, Sep 27, 2018 at 01:34:16PM -0700, Nick Desaulniers wrote: > > > > > > > > > > > > Neither ib_qp_create_flags nor mlx4_ib_qp_flags have negative values, is > > > > > > > signedness necessary? > > > > > > > > > > > > enums are by default restricted to the range of ints. > > > > > > > > > > That's not quite right, the compiler sizes the enum to be able to fit > > > > > the largest value contained within, today that is int, but if we added > > > > > 1<<31, then it would become larger. > > > > > > > > Hi Jason, > > > > > > > > Are you perhaps confusing C and C++? For C++, an enumeration whose underlying > > > > type is not fixed, the underlying type is an integral type that can represent > > > > all the enumerator values defined in the enumeration. For C however I think > > > > that enumeration values are restricted to what fits in an int. > > > > > > > > Bart. > > > > > > > > > > To quote the sacred texts (ANSIIISO9899-1990): > > > > > 6.5.2.2 Enumeration specifiers > > > The expression that defines the value of an enumeration constant shall > > > be an integral constant > > > expression that has a value representable as an int. > > > > This is the wrong part of the standard to quote it is talking about > > *enumeration constants* not the 'enum X' itself. > > > > Anyhow, the standard is hard to read in this area, but reality is > > this: > > You mean undefined behavior? I think we call this an unstandardized compiler extension :) > > #include <stdio.h> > > > > enum a > > { > > A1 = 1, > > A2 = 1ULL<<40, > > }; > > > > int main(int argc, const char *argv[]) > > { > > printf("%zu\n", sizeof(enum a)); > > return 0; > > } > > > > $ gcc -Wall -std=c11 test.c && ./a.out > > 8 > > > > I forget if this a common compiler extension, unclear standard, or was > > formally revised in C11 or what, but it is the real world the Linux > > kernel lives in. > > > > It is even more confusing if you wonder what types A1 and A2 are! > > > > Jason > > This example is a strawman; we're talking about the minimum sizeof an > enum when all initialized values are representable within an int, Hmm? I said "the compiler sizes the enum to be able to fit the largest value contained within", which is correct for gnu89 mode. It is not ISO C, it looks like it is a popular compiler extension that Linux relies on. > And if you're going to throw type safety out the window by converting > values from one enum to another, for storage you MUST use an int > (anything larger as in your example is undefined behavior). No, that isn't right even without this extension, it is confusing, but the standard you quoted is talking about the type of the CONSTANT, not the enum. Ie this: enum a {A1=1}; enum a val = A1; int foo = val; Gives this warning: t.c:10:17: warning: implicit conversion changes signedness: 'enum a' to 'int' [-Wsign-conversion] The correct integral storage for that enum is 'unsigned int'. There is another peice of standard talking about the type of the enum itself, and confoundingly it is a different type than the types of the constants. C++ got this right, the type of the enum and the type of the constants are always the same and always sized to match the largest constant in the enum, and C++11 got this *really right* and allows the programmer to specify the underlying type of the enum and all of its constants. No more subtle bugs with ~FOO because enum constant values have negative types! > I don't disagree with your point that values should be unsigned for > bitwise operations, but it's not clean to reconcile that with > converting values between different enums. I suggest explicit casts > to unsigned types before bitwise operations. Sometimes the casts are needed, particularly when using ~, but for | it is OK to have no casts, promotion rules work out OK. But, again, this question was about the correct type to use when storing bitwise flags, and that type is u32/64 etc no matter if the constants are defined as enum constants or #defines values. So the first patch was the right one! :) Jason
On Mon, Sep 24, 2018 at 12:57:16PM -0700, Nathan Chancellor wrote: > Clang warns when one enumerated type is implicitly converted to another. > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > type 'enum ib_qp_create_flags' [-Wenum-conversion] > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > type 'enum ib_qp_create_flags' [-Wenum-conversion] > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > ~ ^~~~~~~~~~~~~~~~~ > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > type ib_qp_create_flags. Make that clear to Clang by changing the > create_flags type to u32. > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > include/rdma/ib_verbs.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied to for-next, thanks BTW, how are you compiling with clang? Jason
On Wed, Oct 3, 2018 at 3:35 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Sep 24, 2018 at 12:57:16PM -0700, Nathan Chancellor wrote: > > Clang warns when one enumerated type is implicitly converted to another. > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > ~ ^~~~~~~~~~~~~~~~~ > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > type ib_qp_create_flags. Make that clear to Clang by changing the > > create_flags type to u32. > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > include/rdma/ib_verbs.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Applied to for-next, thanks > > BTW, how are you compiling with clang? https://github.com/ClangBuiltLinux/linux/wiki/Steps-for-compiling-the-kernel-with-Clang try it out, let us know bugs you find here: https://github.com/ClangBuiltLinux/linux/issues Still looking into the case you pointed out earlier. I suspect the signedness of enums was undefined in c90, then defined as implementation specific in c99 (though I'm still researching that book report). Thanks for your insights! > > Jason
On Wed, Oct 03, 2018 at 03:53:58PM -0700, Nick Desaulniers wrote: > On Wed, Oct 3, 2018 at 3:35 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Mon, Sep 24, 2018 at 12:57:16PM -0700, Nathan Chancellor wrote: > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > create_flags type to u32. > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > include/rdma/ib_verbs.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Applied to for-next, thanks > > > > BTW, how are you compiling with clang? > > https://github.com/ClangBuiltLinux/linux/wiki/Steps-for-compiling-the-kernel-with-Clang > try it out, let us know bugs you find here: > https://github.com/ClangBuiltLinux/linux/issues Oh I see, you are doing ARM64! > Still looking into the case you pointed out earlier. I suspect the > signedness of enums was undefined in c90, then defined as > implementation specific in c99 (though I'm still researching that > book report). Thanks for your insights! C enums details are a topic that seems more confusing every time it gets brought up :( Jason
On Wed, Oct 3, 2018 at 4:01 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Wed, Oct 03, 2018 at 03:53:58PM -0700, Nick Desaulniers wrote: > > On Wed, Oct 3, 2018 at 3:35 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Mon, Sep 24, 2018 at 12:57:16PM -0700, Nathan Chancellor wrote: > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; > > > > ~ ^~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion > > > > from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration > > > > type 'enum ib_qp_create_flags' [-Wenum-conversion] > > > > qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; > > > > ~ ^~~~~~~~~~~~~~~~~ > > > > > > > > The type mlx4_ib_qp_flags explicitly provides supplemental values to the > > > > type ib_qp_create_flags. Make that clear to Clang by changing the > > > > create_flags type to u32. > > > > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > include/rdma/ib_verbs.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > Applied to for-next, thanks > > > > > > BTW, how are you compiling with clang? > > > > https://github.com/ClangBuiltLinux/linux/wiki/Steps-for-compiling-the-kernel-with-Clang > > try it out, let us know bugs you find here: > > https://github.com/ClangBuiltLinux/linux/issues > > Oh I see, you are doing ARM64! Well, it's the first arch I'm focusing on. X86_64 and ARM32 are also a priority. Open source contributors are filing bugs against powerpc, x86, and even risc-v. I'm helping them with code review or putting them in contact with the relevant parties. We're going to talk more about this effort at Linux Plumbers Conference in November if you're going. > > > Still looking into the case you pointed out earlier. I suspect the > > signedness of enums was undefined in c90, then defined as > > implementation specific in c99 (though I'm still researching that > > book report). Thanks for your insights! > > C enums details are a topic that seems more confusing every time it > gets brought up :( > > Jason
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index e463d3007a35..f6f4d9e3c8ed 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1149,7 +1149,7 @@ struct ib_qp_init_attr { struct ib_qp_cap cap; enum ib_sig_type sq_sig_type; enum ib_qp_type qp_type; - enum ib_qp_create_flags create_flags; + u32 create_flags; /* * Only needed for special QP types, or when using the RW API.
Clang warns when one enumerated type is implicitly converted to another. drivers/infiniband/hw/mlx4/mad.c:1811:41: warning: implicit conversion from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration type 'enum ib_qp_create_flags' [-Wenum-conversion] qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_TUNNEL_QP; ~ ^~~~~~~~~~~~~~~~~~~~~~~ drivers/infiniband/hw/mlx4/mad.c:1819:41: warning: implicit conversion from enumeration type 'enum mlx4_ib_qp_flags' to different enumeration type 'enum ib_qp_create_flags' [-Wenum-conversion] qp_init_attr.init_attr.create_flags = MLX4_IB_SRIOV_SQP; ~ ^~~~~~~~~~~~~~~~~ The type mlx4_ib_qp_flags explicitly provides supplemental values to the type ib_qp_create_flags. Make that clear to Clang by changing the create_flags type to u32. Reported-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- include/rdma/ib_verbs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)