[ovs-dev] ovsdb-idlc: Use ALIGNED_CAST to avoid spurious warnings for index rows.

Message ID 20180910200059.31636-1-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] ovsdb-idlc: Use ALIGNED_CAST to avoid spurious warnings for index rows.
Related show

Commit Message

Ben Pfaff Sept. 10, 2018, 8 p.m.
The *_index_init_row() function casts a generic ovsdb_idl_row pointer to
a specific type of row pointer.  This can cause an increase in required
alignment with some kinds of data on some architectures.  GCC complains,
e.g.:

    lib/vswitch-idl.c: In function 'ovsrec_flow_sample_collector_set_index_init_row'
    lib/vswitch-idl.c:9277:12: warning: cast increases required alignment of target

However, rows are always allocated with malloc(), which returns member
suitable for any type, so this is a false positive warning and this commit
suppresses it.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/ovsdb-idlc.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Han Zhou Sept. 12, 2018, 6:32 a.m. | #1
On Mon, Sep 10, 2018 at 1:01 PM Ben Pfaff <blp@ovn.org> wrote:
>
> The *_index_init_row() function casts a generic ovsdb_idl_row pointer to
> a specific type of row pointer.  This can cause an increase in required
> alignment with some kinds of data on some architectures.  GCC complains,
> e.g.:
>
>     lib/vswitch-idl.c: In function
'ovsrec_flow_sample_collector_set_index_init_row'
>     lib/vswitch-idl.c:9277:12: warning: cast increases required alignment
of target

Hi Ben, could you share on which compiler/version you see this warning?
>
> However, rows are always allocated with malloc(), which returns member
> suitable for any type, so this is a false positive warning and this commit
> suppresses it.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  ovsdb/ovsdb-idlc.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
> index 1c9483cbe393..40fef39edff7 100755
> --- a/ovsdb/ovsdb-idlc.in
> +++ b/ovsdb/ovsdb-idlc.in
> @@ -1217,7 +1217,7 @@ struct %(s)s *
>  %(s)s_index_init_row(struct ovsdb_idl_index *index)
>  {
>      ovs_assert(index->table->class_ == &%(p)stable_%(tl)s);
> -    return (struct %(s)s *) ovsdb_idl_index_init_row(index);
> +    return ALIGNED_CAST(struct %(s)s *, ovsdb_idl_index_init_row(index));
>  }
>
>  struct %(s)s *
> --
> 2.16.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff Sept. 12, 2018, 9:18 p.m. | #2
On Tue, Sep 11, 2018 at 11:32:25PM -0700, Han Zhou wrote:
> On Mon, Sep 10, 2018 at 1:01 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > The *_index_init_row() function casts a generic ovsdb_idl_row pointer to
> > a specific type of row pointer.  This can cause an increase in required
> > alignment with some kinds of data on some architectures.  GCC complains,
> > e.g.:
> >
> >     lib/vswitch-idl.c: In function
> 'ovsrec_flow_sample_collector_set_index_init_row'
> >     lib/vswitch-idl.c:9277:12: warning: cast increases required alignment
> of target
> 
> Hi Ben, could you share on which compiler/version you see this warning?

It's on non-x86 architectures, e.g. mipsel.  I don't think it's
particularly sensitive to GCC version but this is with version 8.2.0:
https://buildd.debian.org/status/fetch.php?pkg=openvswitch&arch=mipsel&ver=2.10.0%2B2018.08.28%2Bgit.8ca7c82b7d%2Bds1-3&stamp=1536673373&raw=0
Han Zhou Sept. 12, 2018, 10:47 p.m. | #3
On Wed, Sep 12, 2018 at 2:18 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Tue, Sep 11, 2018 at 11:32:25PM -0700, Han Zhou wrote:
> > On Mon, Sep 10, 2018 at 1:01 PM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > The *_index_init_row() function casts a generic ovsdb_idl_row pointer
to
> > > a specific type of row pointer.  This can cause an increase in
required
> > > alignment with some kinds of data on some architectures.  GCC
complains,
> > > e.g.:
> > >
> > >     lib/vswitch-idl.c: In function
> > 'ovsrec_flow_sample_collector_set_index_init_row'
> > >     lib/vswitch-idl.c:9277:12: warning: cast increases required
alignment
> > of target
> >
> > Hi Ben, could you share on which compiler/version you see this warning?
>
> It's on non-x86 architectures, e.g. mipsel.  I don't think it's
> particularly sensitive to GCC version but this is with version 8.2.0:
>
https://buildd.debian.org/status/fetch.php?pkg=openvswitch&arch=mipsel&ver=2.10.0%2B2018.08.28%2Bgit.8ca7c82b7d%2Bds1-3&stamp=1536673373&raw=0

I see, thanks. But I wonder why it complains only for this particular type
cast - there are many type casts in other places. Do you have a hint on
when should the ALIGNED_CAST be used, or do we have to rely on the compiler
warnings - and it is not easy to get since most developers doesn't compile
for other archs.
Ben Pfaff Sept. 13, 2018, 9:54 p.m. | #4
On Wed, Sep 12, 2018 at 03:47:05PM -0700, Han Zhou wrote:
> On Wed, Sep 12, 2018 at 2:18 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Tue, Sep 11, 2018 at 11:32:25PM -0700, Han Zhou wrote:
> > > On Mon, Sep 10, 2018 at 1:01 PM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > The *_index_init_row() function casts a generic ovsdb_idl_row pointer
> to
> > > > a specific type of row pointer.  This can cause an increase in
> required
> > > > alignment with some kinds of data on some architectures.  GCC
> complains,
> > > > e.g.:
> > > >
> > > >     lib/vswitch-idl.c: In function
> > > 'ovsrec_flow_sample_collector_set_index_init_row'
> > > >     lib/vswitch-idl.c:9277:12: warning: cast increases required
> alignment
> > > of target
> > >
> > > Hi Ben, could you share on which compiler/version you see this warning?
> >
> > It's on non-x86 architectures, e.g. mipsel.  I don't think it's
> > particularly sensitive to GCC version but this is with version 8.2.0:
> >
> https://buildd.debian.org/status/fetch.php?pkg=openvswitch&arch=mipsel&ver=2.10.0%2B2018.08.28%2Bgit.8ca7c82b7d%2Bds1-3&stamp=1536673373&raw=0
> 
> I see, thanks. But I wonder why it complains only for this particular type
> cast - there are many type casts in other places.

Without actually looking carefully, I think it's because struct
ovsdb_idl_row doesn't have any 64-bit members but the row type in
question does ("int64_t id;").  That makes it look to the compiler like
we're doing something questionable.

> Do you have a hint on when should the ALIGNED_CAST be used, or do we
> have to rely on the compiler warnings - and it is not easy to get
> since most developers doesn't compile for other archs.

The sole value of ALIGNED_CAST is suppressing compiler warnings, in the
case where the compiler's warning is uninformed.  Thus, there's no good
reason for developers to add these proactively anyway.
Han Zhou Sept. 13, 2018, 11:26 p.m. | #5
On Thu, Sep 13, 2018 at 2:54 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Wed, Sep 12, 2018 at 03:47:05PM -0700, Han Zhou wrote:
> > On Wed, Sep 12, 2018 at 2:18 PM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > On Tue, Sep 11, 2018 at 11:32:25PM -0700, Han Zhou wrote:
> > > > On Mon, Sep 10, 2018 at 1:01 PM Ben Pfaff <blp@ovn.org> wrote:
> > > > >
> > > > > The *_index_init_row() function casts a generic ovsdb_idl_row
pointer
> > to
> > > > > a specific type of row pointer.  This can cause an increase in
> > required
> > > > > alignment with some kinds of data on some architectures.  GCC
> > complains,
> > > > > e.g.:
> > > > >
> > > > >     lib/vswitch-idl.c: In function
> > > > 'ovsrec_flow_sample_collector_set_index_init_row'
> > > > >     lib/vswitch-idl.c:9277:12: warning: cast increases required
> > alignment
> > > > of target
> > > >
> > > > Hi Ben, could you share on which compiler/version you see this
warning?
> > >
> > > It's on non-x86 architectures, e.g. mipsel.  I don't think it's
> > > particularly sensitive to GCC version but this is with version 8.2.0:
> > >
> >
https://buildd.debian.org/status/fetch.php?pkg=openvswitch&arch=mipsel&ver=2.10.0%2B2018.08.28%2Bgit.8ca7c82b7d%2Bds1-3&stamp=1536673373&raw=0
> >
> > I see, thanks. But I wonder why it complains only for this particular
type
> > cast - there are many type casts in other places.
>
> Without actually looking carefully, I think it's because struct
> ovsdb_idl_row doesn't have any 64-bit members but the row type in
> question does ("int64_t id;").  That makes it look to the compiler like
> we're doing something questionable.
>
> > Do you have a hint on when should the ALIGNED_CAST be used, or do we
> > have to rely on the compiler warnings - and it is not easy to get
> > since most developers doesn't compile for other archs.
>
> The sole value of ALIGNED_CAST is suppressing compiler warnings, in the
> case where the compiler's warning is uninformed.  Thus, there's no good
> reason for developers to add these proactively anyway.

So it seems only in rare situation will this happen, and it is not a too
bad idea to rely on upstream CI system to find out.

BTW, the questions are only about how to avoid this in the future when we
submitting code. For the patch itself:

Acked-by: hzhou8@ebay.com
Ben Pfaff Sept. 18, 2018, 5:01 a.m. | #6
On Thu, Sep 13, 2018 at 04:26:52PM -0700, Han Zhou wrote:
> On Thu, Sep 13, 2018 at 2:54 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Wed, Sep 12, 2018 at 03:47:05PM -0700, Han Zhou wrote:
> > > On Wed, Sep 12, 2018 at 2:18 PM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > On Tue, Sep 11, 2018 at 11:32:25PM -0700, Han Zhou wrote:
> > > > > On Mon, Sep 10, 2018 at 1:01 PM Ben Pfaff <blp@ovn.org> wrote:
> > > > > >
> > > > > > The *_index_init_row() function casts a generic ovsdb_idl_row
> pointer
> > > to
> > > > > > a specific type of row pointer.  This can cause an increase in
> > > required
> > > > > > alignment with some kinds of data on some architectures.  GCC
> > > complains,
> > > > > > e.g.:
> > > > > >
> > > > > >     lib/vswitch-idl.c: In function
> > > > > 'ovsrec_flow_sample_collector_set_index_init_row'
> > > > > >     lib/vswitch-idl.c:9277:12: warning: cast increases required
> > > alignment
> > > > > of target
> > > > >
> > > > > Hi Ben, could you share on which compiler/version you see this
> warning?
> > > >
> > > > It's on non-x86 architectures, e.g. mipsel.  I don't think it's
> > > > particularly sensitive to GCC version but this is with version 8.2.0:
> > > >
> > >
> https://buildd.debian.org/status/fetch.php?pkg=openvswitch&arch=mipsel&ver=2.10.0%2B2018.08.28%2Bgit.8ca7c82b7d%2Bds1-3&stamp=1536673373&raw=0
> > >
> > > I see, thanks. But I wonder why it complains only for this particular
> type
> > > cast - there are many type casts in other places.
> >
> > Without actually looking carefully, I think it's because struct
> > ovsdb_idl_row doesn't have any 64-bit members but the row type in
> > question does ("int64_t id;").  That makes it look to the compiler like
> > we're doing something questionable.
> >
> > > Do you have a hint on when should the ALIGNED_CAST be used, or do we
> > > have to rely on the compiler warnings - and it is not easy to get
> > > since most developers doesn't compile for other archs.
> >
> > The sole value of ALIGNED_CAST is suppressing compiler warnings, in the
> > case where the compiler's warning is uninformed.  Thus, there's no good
> > reason for developers to add these proactively anyway.
> 
> So it seems only in rare situation will this happen, and it is not a too
> bad idea to rely on upstream CI system to find out.
> 
> BTW, the questions are only about how to avoid this in the future when we
> submitting code. For the patch itself:
> 
> Acked-by: hzhou8@ebay.com

Thanks, I applied this to master and branch-2.10, and sent out a patch
to better document ALIGNED_CAST.

Patch

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 1c9483cbe393..40fef39edff7 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -1217,7 +1217,7 @@  struct %(s)s *
 %(s)s_index_init_row(struct ovsdb_idl_index *index)
 {
     ovs_assert(index->table->class_ == &%(p)stable_%(tl)s);
-    return (struct %(s)s *) ovsdb_idl_index_init_row(index);
+    return ALIGNED_CAST(struct %(s)s *, ovsdb_idl_index_init_row(index));
 }
 
 struct %(s)s *