[cfe-dev] Warning when calling virtual functions from constructor/desctructor?
Artem Dergachev via cfe-dev
cfe-dev at lists.llvm.org
Tue Dec 18 15:59:04 PST 2018
Static Analyzer's checker already has a mode in which it warns on case
B, but it's off by default because it is surprisingly noisy. In
particular, i vaguely recall that i've seen ~90 warnings on the
OpenOffice codebase - none of them were pure virtual calls and most of
them looked like false positives in the sense that the virtual function
is called intentionally. Which means that i would be generally worried
about this warning. That said, i don't know how warnings are accepted
into Clang and whether there is a threshold on intentional violations. I
suspect that it depends on whether the warning is expected to be on by
default or be part of -Wall or only -Weverything.
Another good thing to do with a warning that may be intentionally
triggered by the user is to add a hint on how to suppress it. Eg., how
-Wparentheses does it:
test.c:2:9: note: place parentheses around the assignment to silence
this warning
if (x = 1) {
^
( )
I guess you could add a note that suggests to qualify the call with a
class name, which is a good thing to do anyway. With that, in my opinion
it'd be a good warning.
Like, even if this is dead code, it should either be removed anyway, or
it's worth it to qualify the call because the user expects it to be
eventually reincarnate. I mean, in any case, it doesn't make any sense
to have such call lexically within a constructor and refuse to add a
qualifier. So i suspect that even without any sophisticated analysis,
this might be quite useful.
If you still want data flow analysis (eg., to suppress any massive
sources of false positives that i didn't think about), consider re-using
existing analyses from lib/Analysis. You might get away with just
combining them, without having to write your own.
In fact, we already have some basic infeasible branch removal in the
Clang CFG, so things like "if (false) { foo(); }" should be easy to avoid.
On 12/17/18 6:04 AM, Arnaud Bienner wrote:
> Thanks all for the feedback :)
> Actually, I think there is two different things here:
> 1) Improving checks on pure virtual functions.
> 2) Adding checks on non-pure virtual functions (my initial suggestion)
>
> I agree that it seems sensible to have 1) being done as part of a
> static analyzer, since as you said, you don't expect the compiler to
> perform (costly) in-depth analysis of the code.
> FWIW note that with today's warning
> (call-to-pure-virtual-from-ctor-dtor), a case like D, but with foo's
> function content directly written in D's constructor will raise a
> warning. i.e. there is no data flow analysis.
> In this case, the correct warning IMO should be to tell the user this
> part of the code will never be executed (i.e. unreachable-code, which
> doesn't catch it, probably for the same reason that it doesn't perform
> data flow analysis)
>
> Going back to 2) my suggestion was about cases like B.
> "diagnosable, but possibly correct": indeed, but IMHO this is the
> point of warnings: to point you to things that are correct but likely
> to be an error.
> I think here the important thing is what ratio "likely" corresponds
> to, as you said.
>
> What is usually the acceptable ratio for new warnings?
>
> In the case of B, IMO the correct code should be:
> struct B {
> B() { B::do_foo(); }
> virtual void do_foo();
> };
> Which does the same thing, but is clearer for anyone reading the code,
> since it removes any ambiguity.
>
> Agreed this could be useful as part of static analyzing as well, but
> having some lightweight checks as part of the compilation process is
> also useful if it catches most obvious cases: as I said, this can save
> developers time if they notice it right away, instead of having to
> wait for their changes to go through CI/static code analysis step (the
> sooner you catch potential errors, the better it is).
>
> Arnaud
>
>
>
>
> Le ven. 14 déc. 2018 à 02:19, Artem Dergachev <noqnoqneo at gmail.com
> <mailto:noqnoqneo at gmail.com>> a écrit :
>
> 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 <mailto: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/20181218/bc8afc80/attachment.html>
More information about the cfe-dev
mailing list