Re: Coverity complaints about Remus in xen-unstable

[Andrew Cooper <andrew.cooper3@citrix.com>]

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