diff mbox series

[v2] common/sfc: replace out of bounds condition with static_assert

Message ID 20240119221401.68182-1-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] common/sfc: replace out of bounds condition with static_assert |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation fail Compilation issues
ci/github-robot: build fail github build: failed
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing fail Testing issues
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing fail Testing issues
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Stephen Hemminger Jan. 19, 2024, 10:13 p.m. UTC 
  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

Morten Brørup Jan. 20, 2024, 7:53 a.m. UTC | #1 
> 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>
Ferruh Yigit Feb. 7, 2024, 7:10 p.m. UTC | #2 
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.
Stephen Hemminger Feb. 7, 2024, 10:34 p.m. UTC | #3 
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
Stephen Hemminger Feb. 7, 2024, 10:36 p.m. UTC | #4 
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.
Ferruh Yigit Feb. 7, 2024, 11:30 p.m. UTC | #5 
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
Stephen Hemminger Feb. 11, 2024, 5:41 p.m. UTC | #6 
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.
diff mbox series

Patch

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]))