Coverity complaints about Remus in xen-unstable

Coverity complaints about Remus in xen-unstable

[Ian Jackson]

scan-admin@coverity.com writes ("New Defects reported by Coverity Scan for XenProject"):
> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
...
> ** CID 1242320:  Uninitialized scalar variable  (UNINIT)
> /tools/libxl/libxl.c: 859 in libxl_domain_remus_start()

This is a failure to set rc on some of the error paths.  It is not a
big problem, but it is a bug which should be fixed, in
libxl_domain_remus_start.

Yang Hongyang, could you prepare a patch to fix all the error exit
paths from this function to correctly set rc ?  Thanks.


Then there are also a lot like this:

> ** CID 1242321:  Unused value  (UNUSED_VALUE)
> /tools/libxl/libxl_remus_device.c: 216 in all_devices_setup_cb()

These are all:

> >>>     Pointer "gc" returned by "libxl__ao_inprogress_gc(ao)" is never used.

This is because a lot of functions were introduced which say
    STATE_AO_GC(something)
but do not happen to use the gc.  This is actually perfectly normal
in libxl.  And the STATE_AO_GC macro says:
    libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
So I think this is some kind of failure by Coverity.

Weirdly, although many of these uses (eg, all_devices_setup_cb at
libxl_remus_device.c:212) are in functions which do not use the
defined `ao' variable either, and ao is _not_ marked ok-to-be-unused,
Coverity hasn't complained about that.

Andrew Cooper, as our Coverity modelling expert, can you comment ?


I don't think there is actually anything wrong with having STATE_AO_GC
used when not needed.  It will reduce noise in future if code is added
which needs it, and in the meantime it's harmless.  So I think it
would probably be better if STATE_AO_GC declared ao
__attribute__((unused)) as well.

Yang Hongyang, supposing my comaintainers agree, would you care to
write a patch to do this ?


Thanks,
Ian.

Re: Coverity complaints about Remus in xen-unstable

[Andrew Cooper]

On 01/10/14 16:32, Ian Jackson wrote:
> scan-admin@coverity.com writes ("New Defects reported by Coverity Scan for XenProject"):
>> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
> ...
>> ** CID 1242320:  Uninitialized scalar variable  (UNINIT)
>> /tools/libxl/libxl.c: 859 in libxl_domain_remus_start()
> This is a failure to set rc on some of the error paths.  It is not a
> big problem, but it is a bug which should be fixed, in
> libxl_domain_remus_start.
>
> Yang Hongyang, could you prepare a patch to fix all the error exit
> paths from this function to correctly set rc ?  Thanks.

No need - I already did a patch earlier today, which has even been
committed.

http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bc8e20414aeacdb23d183793c1d947e28c26685b

>
>
> Then there are also a lot like this:
>
>> ** CID 1242321:  Unused value  (UNUSED_VALUE)
>> /tools/libxl/libxl_remus_device.c: 216 in all_devices_setup_cb()
> These are all:
>
>>>>>     Pointer "gc" returned by "libxl__ao_inprogress_gc(ao)" is never used.
> This is because a lot of functions were introduced which say
>     STATE_AO_GC(something)
> but do not happen to use the gc.  This is actually perfectly normal
> in libxl.  And the STATE_AO_GC macro says:
>     libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
> So I think this is some kind of failure by Coverity.

This is not a failure in the slightest.  The statement is "You have an
unused variable, and this constitutes a potential code maintenance
problem which you might want to see about fixing".

Coverity covers coding best practice as well as bugs.  The fact that the
programmer has indicated that the value is unused does not invalidate
Coverities statement about it being unused.

Also bear in mind that Coverities entire purpose is to second-guess what
the programmer has actually written, and raise queries based on what
they plausibly may have overlooked.  Blindly trusting an
"__attribute__((unused))" to 'fix' a compiler warning would entirely
defeat the purpose flagging code maintenance issues.


>
> Weirdly, although many of these uses (eg, all_devices_setup_cb at
> libxl_remus_device.c:212) are in functions which do not use the
> defined `ao' variable either, and ao is _not_ marked ok-to-be-unused,
> Coverity hasn't complained about that.
>
> Andrew Cooper, as our Coverity modelling expert, can you comment ?

'ao' is unconditionally used in all places, as a parameter to
libxl__ao_inprogress_gc(), used to get the gc.

>
>
> I don't think there is actually anything wrong with having STATE_AO_GC
> used when not needed.  It will reduce noise in future if code is added
> which needs it, and in the meantime it's harmless.  So I think it
> would probably be better if STATE_AO_GC declared ao
> __attribute__((unused)) as well.

I disagree.  Removing the gc could also aleivate redundant calls to
libxl__ao_inprogress_gc() which is not inlinable as it resides in a
different translation unit.

~Andrew

Re: Coverity complaints about Remus in xen-unstable

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] Coverity complaints about Remus in xen-unstable"):
> On 01/10/14 16:32, Ian Jackson wrote:
> > Yang Hongyang, could you prepare a patch to fix all the error exit
> > paths from this function to correctly set rc ?  Thanks.
> 
> No need - I already did a patch earlier today, which has even been
> committed.

Oh, good.  I didn't spot that.  Thanks.

> > This is because a lot of functions were introduced which say
> >     STATE_AO_GC(something)
> > but do not happen to use the gc.  This is actually perfectly normal
> > in libxl.  And the STATE_AO_GC macro says:
> >     libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
> > So I think this is some kind of failure by Coverity.
> 
> This is not a failure in the slightest.  [...]
> 
> Coverity covers coding best practice as well as bugs.  The fact that the
> programmer has indicated that the value is unused does not invalidate
> Coverities statement about it being unused.

It does however mean that it is not useful to tell the programmeer
about it.  In other words, I entirely disagree with your analysis
which I think is bordering on the absurd.

Is there a way to fix this in Coverity's modelling or should we report
it as a false positive ?

> Also bear in mind that Coverities entire purpose is to second-guess what
> the programmer has actually written, and raise queries based on what
> they plausibly may have overlooked.  Blindly trusting an
> "__attribute__((unused))" to 'fix' a compiler warning would entirely
> defeat the purpose flagging code maintenance issues.

The whole point of __attribute__((unused)) is for the programmer to be
able to say that it is _expected_ that the variable might not be used
and that it is unlikely to indicate any kind of `code maintenance
issue'.

As indeed in this case.

> > I don't think there is actually anything wrong with having STATE_AO_GC
> > used when not needed.  It will reduce noise in future if code is added
> > which needs it, and in the meantime it's harmless.  So I think it
> > would probably be better if STATE_AO_GC declared ao
> > __attribute__((unused)) as well.
> 
> I disagree.  Removing the gc could also aleivate redundant calls to
> libxl__ao_inprogress_gc() which is not inlinable as it resides in a
> different translation unit.

We do not care about that at all.  Nothing in these functions is even
slightly near a hot path.  We care much more about maintainability.

I would NAK a patch to remove all of these at-present-not-needed uses
of STATE_AO_GC.

> > Weirdly, although many of these uses (eg, all_devices_setup_cb at
> > libxl_remus_device.c:212) are in functions which do not use the
> > defined `ao' variable either, and ao is _not_ marked ok-to-be-unused,
> > Coverity hasn't complained about that.
> >
> > Andrew Cooper, as our Coverity modelling expert, can you comment ?
> 
> 'ao' is unconditionally used in all places, as a parameter to
> libxl__ao_inprogress_gc(), used to get the gc.

Oh, yes, of course.

Thanks,
Ian.

Re: Coverity complaints about Remus in xen-unstable

[David Vrabel]

On 02/10/14 10:34, Ian Jackson wrote:
> 
> Is there a way to fix this in Coverity's modelling or should we report
> it as a false positive ?

I don't think this is a false positive -- coverity has correctly
identified the variable as unused.  Perhaps you should tag the issue as
"Intentional"?

Longer term, I think libxl should move away from its profusion of macros
that declare local variables -- I think you nicely demonstrated that
they are confusing and that it's easy to forgot what they actually do.

David

Re: Coverity complaints about Remus in xen-unstable [and 1 more messages]

[Ian Jackson]

David Vrabel writes ("Re: [Xen-devel] Coverity complaints about Remus in xen-unstable"):
> On 02/10/14 10:34, Ian Jackson wrote:
> > Is there a way to fix this in Coverity's modelling or should we report
> > it as a false positive ?
> 
> I don't think this is a false positive -- coverity has correctly
> identified the variable as unused.  Perhaps you should tag the issue as
> "Intentional"?

Tagging dozens of issues individually is a waste of time when we have
specifically marked this issue as intentional in the code.

> Longer term, I think libxl should move away from its profusion of macros
> that declare local variables -- I think you nicely demonstrated that
> they are confusing and that it's easy to forgot what they actually do.

Replacing these local variables with open-coded versions invites
errors, variations in variable names and semantics, uses of the helper
functions in inappropriate ways, etc.  The macros (and supporting
functions) have been carefully designed to make it easy to use them
correctly and hard to use them incorrectly.

They also make updates to the code much easier to review.


Andrew Cooper writes ("Re: [Xen-devel] Coverity complaints about Remus in xen-unstable"):
> On 02/10/14 10:34, Ian Jackson wrote:
> > It does however mean that it is not useful to tell the programmeer
> > about it.
> 
> The author of the code is only one intended consumer of this information.
> 
> Other consumers, certainly in a corporate setting, might include 3rd
> party auditors, or managers wanting to monitor inflow and changes of
> defect rates as the code they are responsible for gets developed.

Someone who wants to know find of the perhaps-unused variables, despite
the messages having been suppressed, can:
  git-grep 'attribute.*unused'

> > Is there a way to fix this in Coverity's modelling or should we report
> > it as a false positive ?
> 
> It is not a false positive.  It is an entirely accurate statement that
> the variable is unused, and furthermore provides a justification of why
> Coverity considers this to be a problem. 
> http://cwe.mitre.org/data/definitions/563.html

The explanation there is a good explanation of why unused variables
are normally worth warning about.

However, these reasons do not apply for a use of STATE_AO_GC.  It is
very difficult to use STATE_AO_GC inappropriately.  The contextual
scope variables it generates are those expected by the libxl coding
style and have the expected semantics.

There is certainly no point asking programmers and reviewers to
manually add and remove STATE_AO_GC (and the two similar macros
AO_GC and EGC_GC) whenever they happen to add and remove the first and
last code in the body of a function which uses `gc'.  That is
pointless noise.

Furthermore, there is actually a plausible reason to call STATE_AO_GC
when not technically needed.  It calls libxl__ao_inprogress_gc which
checks that the ao is still in progress, helping early detection of
duplicated-flow-of-control bugs.


> There is an "Intentional" option for this purpose.  i.e. "I have taken
> on board what Coverity thinks, and still believe that the code is correct".

I am looking for a way to stop these complaints from surfacing in the
future.

The alternative is that we will simply (whether in software or
wetware) ignore ALL `unused variable' warnings from Coverity, thus
making them all useless.


> > The whole point of __attribute__((unused)) is for the programmer to be
> > able to say that it is _expected_ that the variable might not be used
> > and that it is unlikely to indicate any kind of `code maintenance
> > issue'.
> 
> The purpose of any __attribute__ is to inform the compiler of something
> it doesn't know, or can't work out.
> 
> Its purpose is not to silence warnings specifically requested by the use
> of -Wall.  I feel that using -Wno-unused-variable is a more appropriate
> way of achieving the same result, and has the advantage of not needing
> to litter the code with GCC-isms.

Disabling unused variable warnings globally is an absurd suggestion,
for the reasons explained in the Coverity link you gave!

You are wrong about the purpose of __attribute__((unused)).  Here is
what the GCC 4.4 manual says about it:

  `unused'
       This attribute, attached to a variable, means that the variable
       is meant to be possibly unused.  GCC will not produce a warning
       for this variable.

> First [legimate purpose for __attribute__((unused))] is for static
> functions/data which are referenced from assembly code.  In these
> cases, there is a valid reference which is invisible to the
> compiler, but eventually visible to the linker when the relocation
> references are resolved.

I think you are thinking of __attribute__((used)).

__attribute__((unused)) does not require the compiler to emit code for
the apparently-unused variable or function.
         
Ian.

Re: Coverity complaints about Remus in xen-unstable

[Andrew Cooper]

On 02/10/14 10:34, Ian Jackson wrote:
>
>>> This is because a lot of functions were introduced which say
>>>     STATE_AO_GC(something)
>>> but do not happen to use the gc.  This is actually perfectly normal
>>> in libxl.  And the STATE_AO_GC macro says:
>>>     libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
>>> So I think this is some kind of failure by Coverity.
>> This is not a failure in the slightest.  [...]
>>
>> Coverity covers coding best practice as well as bugs.  The fact that the
>> programmer has indicated that the value is unused does not invalidate
>> Coverities statement about it being unused.
> It does however mean that it is not useful to tell the programmeer
> about it.

The author of the code is only one intended consumer of this information.

Other consumers, certainly in a corporate setting, might include 3rd
party auditors, or managers wanting to monitor inflow and changes of
defect rates as the code they are responsible for gets developed.

The paid-for version has far more knobs to tweak than are exposed to the
users of Scan, including a choice of which checkers should be run during
analysis, and which results are worth reporting.

> In other words, I entirely disagree with your analysis
> which I think is bordering on the absurd.
>
> Is there a way to fix this in Coverity's modelling or should we report
> it as a false positive ?

It is not a false positive.  It is an entirely accurate statement that
the variable is unused, and furthermore provides a justification of why
Coverity considers this to be a problem. 
http://cwe.mitre.org/data/definitions/563.html

There is an "Intentional" option for this purpose.  i.e. "I have taken
on board what Coverity thinks, and still believe that the code is correct".

>> Also bear in mind that Coverities entire purpose is to second-guess what
>> the programmer has actually written, and raise queries based on what
>> they plausibly may have overlooked.  Blindly trusting an
>> "__attribute__((unused))" to 'fix' a compiler warning would entirely
>> defeat the purpose flagging code maintenance issues.
> The whole point of __attribute__((unused)) is for the programmer to be
> able to say that it is _expected_ that the variable might not be used
> and that it is unlikely to indicate any kind of `code maintenance
> issue'.

The purpose of any __attribute__ is to inform the compiler of something
it doesn't know, or can't work out.

Its purpose is not to silence warnings specifically requested by the use
of -Wall.  I feel that using -Wno-unused-variable is a more appropriate
way of achieving the same result, and has the advantage of not needing
to litter the code with GCC-isms.

> As indeed in this case.

This is a matter of opinion, not a statement of fact.

Allow me to venture my opinion.  I am aware of two cases which I
consider legitimate uses of __attribute__((unused)).

First is for static functions/data which are referenced from assembly
code.  In these cases, there is a valid reference which is invisible to
the compiler, but eventually visible to the linker when the relocation
references are resolved.

An example of the second may be found here:
http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=blob;f=tools/libxc/saverestore/common.c;h=d9e47ef837a85959e4082f95198ca7b772a9c120;hb=4bc342cccff3b1fac6c41cc6c4cc4b9eb13ff3d4#l49

This allows the BUILD_BUG_ON()s to work, to be located in the most
appropriate location I could find for them, but also allows any level of
optimisation to discard the function (and its zero contents) when the
linker eventually finds no reference to it.

>
>>> I don't think there is actually anything wrong with having STATE_AO_GC
>>> used when not needed.  It will reduce noise in future if code is added
>>> which needs it, and in the meantime it's harmless.  So I think it
>>> would probably be better if STATE_AO_GC declared ao
>>> __attribute__((unused)) as well.
>> I disagree.  Removing the gc could also aleivate redundant calls to
>> libxl__ao_inprogress_gc() which is not inlinable as it resides in a
>> different translation unit.
> We do not care about that at all.  Nothing in these functions is even
> slightly near a hot path.  We care much more about maintainability.

I clearly have a different idea of what "maintainable code" means.

>
> I would NAK a patch to remove all of these at-present-not-needed uses
> of STATE_AO_GC.

As maintainer of libxl, this is certainly your prerogative.

It is also the reason why my git reflog somewhere has a patch I
developed before realising that it almost certainly would be NAKed if I
posted it to xen-devel.

~Andrew

Re: Coverity complaints about Remus in xen-unstable

[Hongyang Yang]

Hi IanJ, Andrew,

   Sorry for the late replay, just back from a vacation.

在 10/02/2014 12:34 AM, Andrew Cooper 写道:
> On 01/10/14 16:32, Ian Jackson wrote:
>> scan-admin@coverity.com writes ("New Defects reported by Coverity Scan for XenProject"):
>>> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
>> ...
>>> ** CID 1242320:  Uninitialized scalar variable  (UNINIT)
>>> /tools/libxl/libxl.c: 859 in libxl_domain_remus_start()
>> This is a failure to set rc on some of the error paths.  It is not a
>> big problem, but it is a bug which should be fixed, in
>> libxl_domain_remus_start.
>>
>> Yang Hongyang, could you prepare a patch to fix all the error exit
>> paths from this function to correctly set rc ?  Thanks.
>
> No need - I already did a patch earlier today, which has even been
> committed.
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bc8e20414aeacdb23d183793c1d947e28c26685b

Thank you for this.

>
>>
>>
>> Then there are also a lot like this:
>>
>>> ** CID 1242321:  Unused value  (UNUSED_VALUE)
>>> /tools/libxl/libxl_remus_device.c: 216 in all_devices_setup_cb()
>> These are all:
>>
>>>>>>      Pointer "gc" returned by "libxl__ao_inprogress_gc(ao)" is never used.
>> This is because a lot of functions were introduced which say
>>      STATE_AO_GC(something)
>> but do not happen to use the gc.  This is actually perfectly normal
>> in libxl.  And the STATE_AO_GC macro says:
>>      libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
>> So I think this is some kind of failure by Coverity.
>
> This is not a failure in the slightest.  The statement is "You have an
> unused variable, and this constitutes a potential code maintenance
> problem which you might want to see about fixing".
>
> Coverity covers coding best practice as well as bugs.  The fact that the
> programmer has indicated that the value is unused does not invalidate
> Coverities statement about it being unused.
>
> Also bear in mind that Coverities entire purpose is to second-guess what
> the programmer has actually written, and raise queries based on what
> they plausibly may have overlooked.  Blindly trusting an
> "__attribute__((unused))" to 'fix' a compiler warning would entirely
> defeat the purpose flagging code maintenance issues.
>
>
>>
>> Weirdly, although many of these uses (eg, all_devices_setup_cb at
>> libxl_remus_device.c:212) are in functions which do not use the
>> defined `ao' variable either, and ao is _not_ marked ok-to-be-unused,
>> Coverity hasn't complained about that.
>>
>> Andrew Cooper, as our Coverity modelling expert, can you comment ?
>
> 'ao' is unconditionally used in all places, as a parameter to
> libxl__ao_inprogress_gc(), used to get the gc.
>
>>
>>
>> I don't think there is actually anything wrong with having STATE_AO_GC
>> used when not needed.  It will reduce noise in future if code is added
>> which needs it, and in the meantime it's harmless.  So I think it
>> would probably be better if STATE_AO_GC declared ao
>> __attribute__((unused)) as well.
>
> I disagree.  Removing the gc could also aleivate redundant calls to
> libxl__ao_inprogress_gc() which is not inlinable as it resides in a
> different translation unit.
>
> ~Andrew
>
> .
>

-- 
Thanks,
Yang.

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

Re: Coverity complaints about Remus in xen-unstable

[Hongyang Yang]

在 10/01/2014 11:32 PM, Ian Jackson 写道:
> scan-admin@coverity.com writes ("New Defects reported by Coverity Scan for XenProject"):
>> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
> ...
>> ** CID 1242320:  Uninitialized scalar variable  (UNINIT)
>> /tools/libxl/libxl.c: 859 in libxl_domain_remus_start()
>
> This is a failure to set rc on some of the error paths.  It is not a
> big problem, but it is a bug which should be fixed, in
> libxl_domain_remus_start.
>
> Yang Hongyang, could you prepare a patch to fix all the error exit
> paths from this function to correctly set rc ?  Thanks.
>
>
> Then there are also a lot like this:
>
>> ** CID 1242321:  Unused value  (UNUSED_VALUE)
>> /tools/libxl/libxl_remus_device.c: 216 in all_devices_setup_cb()
>
> These are all:
>
>>>>>      Pointer "gc" returned by "libxl__ao_inprogress_gc(ao)" is never used.
>
> This is because a lot of functions were introduced which say
>      STATE_AO_GC(something)
> but do not happen to use the gc.  This is actually perfectly normal
> in libxl.  And the STATE_AO_GC macro says:
>      libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
> So I think this is some kind of failure by Coverity.
>
> Weirdly, although many of these uses (eg, all_devices_setup_cb at
> libxl_remus_device.c:212) are in functions which do not use the
> defined `ao' variable either, and ao is _not_ marked ok-to-be-unused,
> Coverity hasn't complained about that.
>
> Andrew Cooper, as our Coverity modelling expert, can you comment ?
>
>
> I don't think there is actually anything wrong with having STATE_AO_GC
> used when not needed.  It will reduce noise in future if code is added
> which needs it, and in the meantime it's harmless.  So I think it
> would probably be better if STATE_AO_GC declared ao
> __attribute__((unused)) as well.
>
> Yang Hongyang, supposing my comaintainers agree, would you care to
> write a patch to do this ?

Sure, I saw that Andrew has already post the patch, is there anything
else that I can do to help on this?

>
>
> Thanks,
> Ian.
> .
>

-- 
Thanks,
Yang.

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