Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2016-02-25 at 15:43 +0000, George Dunlap wrote:
> On 25/02/16 15:28, Ian Campbell wrote:
> > On Thu, 2016-02-25 at 15:09 +0000, George Dunlap wrote:
> > > On 25/02/16 15:03, Ian Campbell wrote:
> > > > On Thu, 2016-02-25 at 14:48 +0000, George Dunlap wrote:
> > > > > ...so that coverity knows it's unreachable.
> > > > 
> > > > I would not be surprised if Coverity starts complaining about the
> > > > dead
> > > > code
> > > > once this is in place. fprintf + abort is probably what would be
> > > > wanted
> > > > to
> > > > placate it in this case.
> > > 
> > > Hrm -- it would be nice to have a way to figure out what coverity
> > > likes
> > > without having to actually check something into the tree...
> > 
> > If this code is truly unreachable (i.e. it is after a while(1) with no
> > breaks etc) then you should just drop the logging since it will never
> > be
> > reached, then the __builtin_unreachable() is appropriate.
> > 
> > If, as the log message implies, this is code which _should_ be
> > unreachable
> > by design but would be reached in the case of a logic error in the
> > preceding code then what you want is either fprintf()+abort() or maybe
> > assert().
> 
> Right -- well basically error(ASSERT,...) is a custom abort().  But in
> the current case it isn't actually doing anything more than an abort()
> would, so perhaps I should use that instead (since coverity knows about
> abort() and assert() but not my custom function).

Personally this is what I would do in this case.

> > But Coverity seems to have disproven this possibility, correctly AFAICT
> > because all of the preceeding cases of the if chain end with a goto,
> > this
> > removing the logging and leaving the __builtin_unreachable() is the way
> > to
> > go.
> > 
> > I don't think this is really about what would keep Coverity happy, more
> > to
> > do with the intended semantics of execution reaching this point.
> 
> It's already doing what I want mostly; so maybe I should just close the
> bug as "intentional" (or "needs modelling" or something).

This is also a valid thing to do.

Remember, the goal of coverity is to provide a list of places where there
might be opportunities for improvements to be made to the code, not to
provide a list of places to change just to make coverity shut up. If the
code isn't actually improved by fixing whatever coverity is complaining
about then "intentional" or "needs modelling" is the right response.

Ian.

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