| 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. | expand |
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
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
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.
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.
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
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.
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 *
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(-)