[llvm-bugs] [Bug 39075] Unnecessary notes from cplusplus.VirtualCall

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=39075

Artem Dergachev <noqnoqneo at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |noqnoqneo at gmail.com
         Resolution|---                         |WONTFIX

--- Comment #1 from Artem Dergachev <noqnoqneo at gmail.com> ---
Sorry for the slow reply!

The note itself is valid. It may be unnecessary when the virtual function is
called within the constructor itself, but it becomes extremely important when
the virtual function is called by another non-virtual function g() that is
called by the constructor.

For instance, in this slightly more complicated example:

// header:

  struct Foo {
    Foo(int x) {
      g(x); // 2. Calling g(0)
    }

    void Foo::g(int y) {
      if (y == 0) // 3. Taking true branch because 0 == 0
        f(); // 4. Calling a virtual function during construction
    }

    virtual void f();
  };

// user code:

  int main() {
    Foo foo(0); // 1. Calling constructor for 'Foo'
    return 0;
  }

...the analyzer would indeed emit as many as 3 notes, and it is great to have
all of them because they present a proof that Foo::g() is indeed sometimes
called from Foo::Foo with a zero argument from user code.

All advanced "path-sensitive" checkers in the static analyzer, that are pretty
much the whole point of the analyzer, work this way. It is almost impossible to
use the analyzer without having these notes. In complicated code it sometimes
takes hundreds of notes to explain the warning, which is why we have fancy GUIs
(HTML reports and integration with various IDEs) in which those notes are
interleaved with the code for easier reading and navigation.

In your case the note at foo is actually great, because it explains to you that
the warning is in fact produced from *user* code, despite being "physically"
located in the library. The analyzer doesn't tell you to fix the library, it
tells you to fix your code. Well, in your reduced example, the library is so
broken that it's unfixable, so the analyzer is saying that you should never use
that constructor of foo at all, because it always causes broken behavior. If
you stop using the constructor, the warning would disappear; the analyzer would
not warn in library code until it believes it finds a bug in your code that
would cause you to crash while executing code from library header. In my
modified example, if definition of struct Foo is in a header, then writing 'Foo
foo(1);' instead of 'Foo foo(0)' in main() would prevent the warning from
appearing.

It might make sense to have a look at why is the note is unnecessary in this
particular bug report and if we can avoid it (this is an interesting problem,
because our heuristics here are sometimes pretty vague), but this won't help
you with your suppression problem in general.

I believe that clang-tidy should suppress all notes attached to the warning
when the line of code with the warning has a suppression on it; on some
checkers, it makes sense to put suppressions on the "unique-ing location"
instead (cf. BugReport::getUniqueingLocation()). Eg. for memory leak reports it
is better to put the suppression on the line of code on which the allocation
occurs, rather than on the line of code on which the analyzer managed to figure
out that the pointer is leaking.

Generally, it is also recommended to use assert()s to suppress analyzer
warnings. The analyzer trusts assert()s and believes that their conditions are
never true, which is good for informing the analyzer about hidden contracts
within your code. For example, if in main() you had 'Foo foo(z);', then adding
'assert(z != 0)' before the constructor would suppress the warning. Assertion
would also actually prevent undefined behavior in your program: if it is ever
violated, your program would fail with an assert violation instead of behaving
in an undefined manner. Various useful analyzer-specific suppression techniques
are mostly covered in our FAQ (https://clang-analyzer.llvm.org/faq.html).

Please re-open if there's something else we could do to help clang-tidy dealing
with our execution path notes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20181006/2dc974e0/attachment.html>