| Message ID | 1456411743-17741-5-git-send-email-george.dunlap@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Ian. > > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > CC: Ian Jackson <ian.jackson@citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > --- > tools/xentrace/xenalyze.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c > index 9f8c065..123030a 100644 > --- a/tools/xentrace/xenalyze.c > +++ b/tools/xentrace/xenalyze.c > @@ -7306,6 +7306,7 @@ void sched_runstate_process(struct pcpu_info *p) > } > goto update; > } > + __builtin_unreachable(); > fprintf(stderr, "FATAL: Logic hole in %s\n", __func__); > error(ERR_ASSERT, NULL); > }
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... -George
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(). 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. BTW in my simple test case actually trying to execute __builtin_unreachable() results in a SEGV, so that logging really isn't doing anything useful with your patch. Ian.
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). > 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). -George
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.
Ian Campbell writes ("Re: [Xen-devel] [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable"):
> On Thu, 2016-02-25 at 15:43 +0000, George Dunlap wrote:
> > 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.
Indeed. I would replace the fprintf/error with
assert(!"logic hole in this function");
This is a standard idiom for `abort()' when you don't like to just
call abort because it doesn't produce a message.
Ian.
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c index 9f8c065..123030a 100644 --- a/tools/xentrace/xenalyze.c +++ b/tools/xentrace/xenalyze.c @@ -7306,6 +7306,7 @@ void sched_runstate_process(struct pcpu_info *p) } goto update; } + __builtin_unreachable(); fprintf(stderr, "FATAL: Logic hole in %s\n", __func__); error(ERR_ASSERT, NULL); }
...so that coverity knows it's unreachable. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/xentrace/xenalyze.c | 1 + 1 file changed, 1 insertion(+)