[v2] common/sfc: replace out of bounds condition with static_assert
Checks
Commit Message
The sfc base code had its own definition of static assertions
using the out of bound array access hack. Replace it with a
static_assert like rte_common.h.
Fixes: f67e4719147d ("net/sfc/base: fix coding style")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
v2 - add assert.h to make sure it works in other environments
drivers/common/sfc_efx/base/efx.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Comments
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 19 January 2024 23.14
>
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
>
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
On 1/19/2024 10:13 PM, Stephen Hemminger wrote:
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
>
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v2 - add assert.h to make sure it works in other environments
>
> drivers/common/sfc_efx/base/efx.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> index 3312c2fa8f81..38f2aed3e336 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -7,6 +7,8 @@
> #ifndef _SYS_EFX_H
> #define _SYS_EFX_H
>
> +#include <assert.h>
> +
> #include "efx_annote.h"
> #include "efsys.h"
> #include "efx_types.h"
> @@ -17,8 +19,8 @@
> extern "C" {
> #endif
>
> -#define EFX_STATIC_ASSERT(_cond) \
> - ((void)sizeof (char[(_cond) ? 1 : -1]))
> +#define EFX_STATIC_ASSERT(_cond) \
> + do { static_assert((_cond), "assert failed" #_cond); } while (0)
>
> #define EFX_ARRAY_SIZE(_array) \
> (sizeof (_array) / sizeof ((_array)[0]))
Getting following build error with clang:
FAILED: drivers/common/sfc_efx/base/libsfc_base.a.p/ef10_filter.c.o
./drivers/common/sfc_efx/base/ef10_filter.c
../drivers/common/sfc_efx/base/ef10_filter.c:503:2: error: static_assert
expression is not an integral constant expression
EFX_STATIC_ASSERT((EFX_FIELD_OFFSET(efx_filter_spec_t,
efs_outer_vid) %
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx.h:23:21: note: expanded from macro
'EFX_STATIC_ASSERT'
do { static_assert((_cond), "assert failed" #_cond); } while (0)
^~~~~~~
../drivers/common/sfc_efx/base/ef10_filter.c:503:21: note: cast that
performs the conversions of a reinterpret_cast is not allowed in a
constant expression
EFX_STATIC_ASSERT((EFX_FIELD_OFFSET(efx_filter_spec_t,
efs_outer_vid) %
^
../drivers/common/sfc_efx/base/efx.h:29:3: note: expanded from macro
'EFX_FIELD_OFFSET'
((size_t)&(((_type *)0)->_field))
^
../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
>= width of type [-Werror,-Wshift-count-overflow]
matches_count = MCDI_OUT_DWORD(req,
^~~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
macro 'MCDI_OUT_DWORD'
EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst), \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
macro 'EFX_DWORD_FIELD'
EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
^~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
macro 'EFX_MASK32'
(((((uint32_t)1) << EFX_WIDTH(_field))) - 1))
^ ~~~~~~~~~~~~~~~~~
2 errors generated.
On Wed, 7 Feb 2024 19:10:37 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> On 1/19/2024 10:13 PM, Stephen Hemminger wrote:
> > The sfc base code had its own definition of static assertions
> > using the out of bound array access hack. Replace it with a
> > static_assert like rte_common.h.
> >
> > Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v2 - add assert.h to make sure it works in other environments
> >
> > drivers/common/sfc_efx/base/efx.h | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> > index 3312c2fa8f81..38f2aed3e336 100644
> > --- a/drivers/common/sfc_efx/base/efx.h
> > +++ b/drivers/common/sfc_efx/base/efx.h
> > @@ -7,6 +7,8 @@
> > #ifndef _SYS_EFX_H
> > #define _SYS_EFX_H
> >
> > +#include <assert.h>
> > +
> > #include "efx_annote.h"
> > #include "efsys.h"
> > #include "efx_types.h"
> > @@ -17,8 +19,8 @@
> > extern "C" {
> > #endif
> >
> > -#define EFX_STATIC_ASSERT(_cond) \
> > - ((void)sizeof (char[(_cond) ? 1 : -1]))
> > +#define EFX_STATIC_ASSERT(_cond) \
> > + do { static_assert((_cond), "assert failed" #_cond); } while (0)
> >
> > #define EFX_ARRAY_SIZE(_array) \
> > (sizeof (_array) / sizeof ((_array)[0]))
>
> Getting following build error with clang:
What version of clang?
It works for me with clang 16.0.6
On Wed, 7 Feb 2024 19:10:37 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> ../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
> >= width of type [-Werror,-Wshift-count-overflow]
> matches_count = MCDI_OUT_DWORD(req,
> ^~~~~~~~~~~~~~~~~~~
> ../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
> macro 'MCDI_OUT_DWORD'
> EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst), \
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
> macro 'EFX_DWORD_FIELD'
> EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
> ^~~~~~~~~~~~~~~~~~
> ../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
> macro 'EFX_MASK32'
> (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))
None of this got changed by the patch. Looks like it would not compile
even without the patch on your version of clang.
On 2/7/2024 10:36 PM, Stephen Hemminger wrote:
> On Wed, 7 Feb 2024 19:10:37 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
>> ../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
>>> = width of type [-Werror,-Wshift-count-overflow]
>> matches_count = MCDI_OUT_DWORD(req,
>> ^~~~~~~~~~~~~~~~~~~
>> ../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
>> macro 'MCDI_OUT_DWORD'
>> EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst), \
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
>> macro 'EFX_DWORD_FIELD'
>> EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
>> ^~~~~~~~~~~~~~~~~~
>> ../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
>> macro 'EFX_MASK32'
>> (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))
>
> None of this got changed by the patch. Looks like it would not compile
> even without the patch on your version of clang.
>
Nope, error only happens with the patch.
And CI seems reporting the errors:
https://mails.dpdk.org/archives/test-report/2024-January/558546.html
On Wed, 7 Feb 2024 23:30:07 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> On 2/7/2024 10:36 PM, Stephen Hemminger wrote:
> > On Wed, 7 Feb 2024 19:10:37 +0000
> > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >
> >> ../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
> >>> = width of type [-Werror,-Wshift-count-overflow]
> >> matches_count = MCDI_OUT_DWORD(req,
> >> ^~~~~~~~~~~~~~~~~~~
> >> ../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
> >> macro 'MCDI_OUT_DWORD'
> >> EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst), \
> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> ../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
> >> macro 'EFX_DWORD_FIELD'
> >> EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
> >> ^~~~~~~~~~~~~~~~~~
> >> ../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
> >> macro 'EFX_MASK32'
> >> (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))
> >
> > None of this got changed by the patch. Looks like it would not compile
> > even without the patch on your version of clang.
> >
>
> Nope, error only happens with the patch.
>
> And CI seems reporting the errors:
> https://mails.dpdk.org/archives/test-report/2024-January/558546.html
This patch is allowing compiler to see some things as constant which
were hidden before. The driver is buggy, for example, look at this:
/*
* Stop lint complaining about some shifts.
*/
#ifdef __lint
extern int fix_lint;
#define FIX_LINT(_x) (_x + fix_lint)
#else
#define FIX_LINT(_x) (_x)
#endif
Looks like it is wallpapering over some errors.
@@ -7,6 +7,8 @@
#ifndef _SYS_EFX_H
#define _SYS_EFX_H
+#include <assert.h>
+
#include "efx_annote.h"
#include "efsys.h"
#include "efx_types.h"
@@ -17,8 +19,8 @@
extern "C" {
#endif
-#define EFX_STATIC_ASSERT(_cond) \
- ((void)sizeof (char[(_cond) ? 1 : -1]))
+#define EFX_STATIC_ASSERT(_cond) \
+ do { static_assert((_cond), "assert failed" #_cond); } while (0)
#define EFX_ARRAY_SIZE(_array) \
(sizeof (_array) / sizeof ((_array)[0]))