[PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nathan Chancellor]

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;
 
 	/*
 	 * Only needed for special QP types, or when using the RW API.
-- 
2.19.0

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nathan Chancellor]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[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



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nathan Chancellor]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nathan Chancellor]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nathan Chancellor]

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

[PATCH v2] IB/mlx4: Avoid implicit enumerated type conversion

[Nathan Chancellor]

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 int.

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Use int instead of u32 since enums are restricted to this range, as
  suggested by Nick.

 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 0d822a9db300..e5ec04e844cd 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1151,7 +1151,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;
+	int			create_flags;
 
 	/*
 	 * Only needed for special QP types, or when using the RW API.
-- 
2.19.0

Re: [PATCH v2] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

On Thu, Sep 27, 2018 at 1:44 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 int.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Use int instead of u32 since enums are restricted to this range, as
>   suggested by Nick.

Thank you for making this change.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.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 0d822a9db300..e5ec04e844cd 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1151,7 +1151,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;
> +       int                     create_flags;
>
>         /*
>          * Only needed for special QP types, or when using the RW API.
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Bart Van Assche]

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.

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Bart Van Assche]

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.

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx4: Avoid implicit enumerated type conversion

[Nick Desaulniers]

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



-- 
Thanks,
~Nick Desaulniers