Re: [Xen-devel] [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-03-26 at 09:41 +0000, Julien Grall wrote:
> 
> On 26/03/14 09:03, Ian Campbell wrote:
> 
> > enums and ints are often, for better or worse, interchangeable. That's
> > why the existing assert is there, to catch people who are inadvertently
> > doing something like this. (I don't think the cast in Stefano's example
> > is strictly needed, so a real case is less likely to leap out into your
> > face during review).
> 
> It's a shame that the compiler is not able to warn when an int is 
> implicitly cast into an enum.
> 
> >> I think instead of an ASSERT, sgi & 0xff might better. It won't be
> >> harmless for the GIC, even debug is turned off. Right now, the
> >> developper can put the GIC in wrong state if the value is not in this 
> >> range.
> >
> > The whole point of this assert is to help catch programmer mistakes. If
> > the programmers and review process was perfect then the assert would be
> > unnecessary.
> 
> It's valid for the compiler to do some optimization and think this 
> ASSERT is not neccessary. So we don't catch programmer mistakes.

If the compiler is able to prove that it can optimise the ASSERT away
then the programmer has not made a mistake, I think.

> If we want to keep the assert for this reason, we will have also to add 
> sgi & 0xff to protect non-debug build and compiler which remove this assert.

The purpose of the assert in a debug build is so that we can assume it
is ok in a non-debug build.
> 
> > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?
> 
> I forgot to try this solution. Surprisingly, the compiler is able to 
> compile correctly this code. So I can replace the ASSERT(sgi < 16) with 
> ASSERT(sgi < GIC_SGI_MAX) + BUILD_BUG_ON.

Good, that sounds like the preferable approach then.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel