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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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