[cfe-dev] Warning when calling virtual functions from constructor/desctructor?

[Artem Dergachev via cfe-dev]

Yup, Static Analyzer has a checker for this, and its current status is 
"opt-in" (i.e., the checker is `optin.cplusplus.VirtualCall`). So we 
it's stable and more or less full-featured and we encourage you to try 
it out, but it's off by default because it finds non-bugs as well (like 
the case B). We should definitely enable-by-default the part of it that 
finds *pure* virtual calls. I wonder why didn't we do that already. 
There's even an option to enable such behavior.

Yes, indeed, cases like "C" are the reason why this checker was made.

And that's also the reason why this check requires something as powerful 
as a full-featured "symbolic execution" to be useful, which is something 
that's too slow to be a compiler warning. The previous attempt on this 
checker was simply scanning the AST for virtual function calls and went 
through the call graph to see if some of these virtual calls are 
reachable from the constructor. However, this approach was having false 
positives when there was no actual execution path that would result in 
going through the call graph in that order during construction. Eg.,

     struct D {
         bool Flag;
         void setFlag() { Flag = true; } // The flag can be set later.

         D() : Flag(false) { foo(); } // But its initial value is "clear".
         void foo() { if (Flag) bar(); } // Flag is still clear when 
called from D().
         virtual void bar() = 0;
     }

In this case if you look at the AST you'll see that D() calls foo(), 
foo() calls bar(), bar() is pure virtual. But there's no bug in this 
code, because foo() never actually calls bar() when called from the 
constructor. The VirtualCall checker, in its current state, is able to 
understand this sort of stuff as well (up to overall modeling bugs in 
the Static Analyzer).

A warning could still be used to catch some easier patterns, eg., when 
*all* paths through the constructor from a certain point result in a 
pure virtual function call. Eg., if you simplify the problem down to 
finding the bug when D::foo() is defined as something like `if (Flag) 
bar(); else bar();`, it may be possible to come up with an efficient 
data flow analysis that will catch it and have no false positives. But 
it still needs to be inter-procedural, so it's actually still pretty 
hard and we will still probably have to bail out at some stack depth. 
This would most likely become the most sophisticated compiler warning we 
have in Clang: we have a few data flow warnings, such as "variable may 
be used uninitialized in this function", but none of them are 
inter-procedural.

So, yeah, i believe that coming up with a compiler warning is indeed 
relatively hard () and implementing it as a Static Analyzer warning is 
most likely the right approach.


On 12/13/18 12:24 PM, Arthur O'Dwyer via cfe-dev wrote:
> On Thu, Dec 13, 2018 at 1:21 PM Arnaud Bienner via cfe-dev 
> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>
>     What I like about having this kind of diagnostics being part of
>     clang itself is that you can turn them into errors using -Werror=,
>     and prevent bugs to even been written, saving you debugging time.
>     I'm not sure how static code analysis is done on the projects you
>     are working on, but from my experience, it is usually done as part
>     of the CI (so after you commit your changes): this saves you from
>     having bugs reaching the release stage, but doesn't save you the
>     time spent debugging a silly error that could have been catch by
>     the compiler.
>
>
> FWIW, I think most projects run CI on pull requests, so, the code has 
> to pass CI before it's allowed to hit master. (This is how you keep 
> your master CI green all the time, instead of flashing 
> red-green-red-green.)
>
> I think any diagnostic in this area is really too fragile to be 
> useful. Consider:
>
>     struct A {
>         A() { do_foo(); }  // diagnosable and definitely a bug
>         virtual void do_foo() = 0;
>     };
>     struct B {
>         B() { do_foo(); }  // diagnosable, but possibly correct
>         virtual void do_foo();
>     };
>     struct C {
>         C() { foo(); }  // definitely a bug, but not diagnosable
>         void foo() { do_foo(); }  // best practice
>     private:
>         virtual void do_foo() = 0;
>     };
>
> Clang, GCC, and ICC all diagnose "A". Nobody diagnoses "B". Nobody 
> diagnoses "C", even though it's exactly isomorphic to "A" — and "C" 
> (splitting customization points into a public non-virtual interface 
> and a private virtual implementation) is what we want to teach people 
> to do!
>
> I think Clang can't /remove/ its diagnostic for "A" because that would 
> be a regression versus GCC and ICC; but I don't think it's worth 
> spending time teaching Clang to catch "B" (with its attendant false 
> positives) given that "C" is the most interesting case in practice, 
> and Clang will never be able to catch "C".
>
> The static analyzer, though, could definitely catch "C"! I don't know 
> if it does today or not.
>
> my $.02,
> –Arthur
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181213/f837401f/attachment.html>