diff mbox series

[RFC,4/7] arm/gic: Drop pointless assertions

Message ID 1573031953-12894-5-git-send-email-andrii.anisov@gmail.com (mailing list archive)
State New, archived
Headers show
Series Build XEN with ARM Compiler 6.6.3 | expand

Commit Message

Andrii Anisov Nov. 6, 2019, 9:19 a.m. UTC 
From: Andrii Anisov <andrii_anisov@epam.com>

Also armclang complains about the condition always true,
because `sgi` is of type enum with all its values under 16.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---
 xen/arch/arm/gic.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Stefano Stabellini Nov. 11, 2019, 8:52 p.m. UTC | #1 
On Wed, 6 Nov 2019, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> Also armclang complains about the condition always true,
> because `sgi` is of type enum with all its values under 16.
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>

Although I am not completely opposed to this, given the choice I would
prefer to keep the ASSERTs.

Given that I would imagine that the ARM C Compiler will also complain
about many other ASSERTs, I wonder if it wouldn't be better to just
disable *all* ASSERTs when building with armcc by changing the
implementation of the ASSERT MACRO.


> ---
>  xen/arch/arm/gic.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 113655a..58c6141 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -294,8 +294,6 @@ void __init gic_init(void)
>  
>  void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>  {
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> -
>      gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask);
>  }
>  
> @@ -306,15 +304,11 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
>  
>  void send_SGI_self(enum gic_sgi sgi)
>  {
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> -
>      gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL);
>  }
>  
>  void send_SGI_allbutself(enum gic_sgi sgi)
>  {
> -   ASSERT(sgi < 16); /* There are only 16 SGIs */
> -
>     gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL);
>  }
>  
> -- 
> 2.7.4
>
Julien Grall Nov. 13, 2019, 1:14 a.m. UTC | #2 
On Tue, 12 Nov 2019, 05:52 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Wed, 6 Nov 2019, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@epam.com>
> >
> > Also armclang complains about the condition always true,
> > because `sgi` is of type enum with all its values under 16.
> >
> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
>
> Although I am not completely opposed to this, given the choice I would
> prefer to keep the ASSERTs.
>

Why? What would that prevent? It is an enum, so unless you do an horrible
hack on the other side, this should always be valid.

But then, why would this be an issue here and not in the tens other place
where enum is used?



> Given that I would imagine that the ARM C Compiler will also complain
> about many other ASSERTs, I wonder if it wouldn't be better to just
> disable *all* ASSERTs when building with armcc by changing the
> implementation of the ASSERT MACRO.


ARM C compiler is valid here and I would not be surprised this will come up
in Clang and GCC in the future.

If you are worry that the enum is going to grow more than 16 items, then
you should use a BUILD_BUG_ON.




>
> > ---
> >  xen/arch/arm/gic.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 113655a..58c6141 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -294,8 +294,6 @@ void __init gic_init(void)
> >
> >  void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
> >  {
> > -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> > -
> >      gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask);
> >  }
> >
> > @@ -306,15 +304,11 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi
> sgi)
> >
> >  void send_SGI_self(enum gic_sgi sgi)
> >  {
> > -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> > -
> >      gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL);
> >  }
> >
> >  void send_SGI_allbutself(enum gic_sgi sgi)
> >  {
> > -   ASSERT(sgi < 16); /* There are only 16 SGIs */
> > -
> >     gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL);
> >  }
> >
> > --
> > 2.7.4
> >
>
Stefano Stabellini Nov. 20, 2019, 10:15 p.m. UTC | #3 
On Wed, 13 Nov 2019, Julien Grall wrote:
> On Tue, 12 Nov 2019, 05:52 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Wed, 6 Nov 2019, Andrii Anisov wrote:
>       > From: Andrii Anisov <andrii_anisov@epam.com>
>       >
>       > Also armclang complains about the condition always true,
>       > because `sgi` is of type enum with all its values under 16.
>       >
>       > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> 
>       Although I am not completely opposed to this, given the choice I would
>       prefer to keep the ASSERTs.
> 
> 
> Why? What would that prevent? It is an enum, so unless you do an horrible hack on the other side, this should always be valid.
> 
> But then, why would this be an issue here and not in the tens other place where enum is used?
> 
> 
> 
>       Given that I would imagine that the ARM C Compiler will also complain
>       about many other ASSERTs, I wonder if it wouldn't be better to just
>       disable *all* ASSERTs when building with armcc by changing the
>       implementation of the ASSERT MACRO.
> 
> 
> ARM C compiler is valid here and I would not be surprised this will come up in Clang and GCC in the future.
> 
> If you are worry that the enum is going to grow more than 16 items, then you should use a BUILD_BUG_ON.

That would be better actually

 
>       > ---
>       >  xen/arch/arm/gic.c | 6 ------
>       >  1 file changed, 6 deletions(-)
>       >
>       > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>       > index 113655a..58c6141 100644
>       > --- a/xen/arch/arm/gic.c
>       > +++ b/xen/arch/arm/gic.c
>       > @@ -294,8 +294,6 @@ void __init gic_init(void)
>       > 
>       >  void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>       >  {
>       > -    ASSERT(sgi < 16); /* There are only 16 SGIs */
>       > -
>       >      gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask);
>       >  }
>       > 
>       > @@ -306,15 +304,11 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
>       > 
>       >  void send_SGI_self(enum gic_sgi sgi)
>       >  {
>       > -    ASSERT(sgi < 16); /* There are only 16 SGIs */
>       > -
>       >      gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL);
>       >  }
>       > 
>       >  void send_SGI_allbutself(enum gic_sgi sgi)
>       >  {
>       > -   ASSERT(sgi < 16); /* There are only 16 SGIs */
>       > -
>       >     gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL);
>       >  }
>       > 
>       > --
>       > 2.7.4
>       >
> 
> 
>
diff mbox series

Patch

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 113655a..58c6141 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -294,8 +294,6 @@  void __init gic_init(void)
 
 void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
 {
-    ASSERT(sgi < 16); /* There are only 16 SGIs */
-
     gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask);
 }
 
@@ -306,15 +304,11 @@  void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
 
 void send_SGI_self(enum gic_sgi sgi)
 {
-    ASSERT(sgi < 16); /* There are only 16 SGIs */
-
     gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL);
 }
 
 void send_SGI_allbutself(enum gic_sgi sgi)
 {
-   ASSERT(sgi < 16); /* There are only 16 SGIs */
-
    gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL);
 }