<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
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.<br>
<br>
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:<br>
<br>
test.c:2:9: note: place parentheses around the assignment to
silence this warning<br>
if (x = 1) {<br>
^<br>
( )<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
<br>
<div class="moz-cite-prefix">On 12/17/18 6:04 AM, Arnaud Bienner
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CALZjR+1+LrAEt8xXq=PGEWL1dh0T-0WkBhP2Ggg-=d8aeMJbVA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">
<div>Thanks all for the feedback :)</div>
<div>Actually, I think there is two different things here:</div>
<div>1) Improving checks on pure virtual functions. <br>
</div>
<div>2) Adding checks on non-pure virtual functions (my
initial suggestion)<br>
</div>
<div><br>
</div>
<div>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.<br>
</div>
<div>FWIW note that with today's warning (<span
class="gmail-blob-code-inner
gmail-blob-code-marker-addition">call-to-pure-virtual-from-ctor-dtor)</span>,
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.</div>
<div>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)</div>
<div><br>
</div>
<div>Going back to 2) my suggestion was about cases like B.</div>
<div>"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.</div>
<div>I think here the important thing is what ratio "likely"
corresponds to, as you said.<br>
</div>
<div><br>
</div>
<div>What is usually the acceptable ratio for new warnings?</div>
<div><br>
</div>
<div>In the case of B, IMO the correct code should be:</div>
<div>
<div>struct B {</div>
<div> B() { B::do_foo(); }<br>
</div>
<div> virtual void do_foo();</div>
<div>};</div>
</div>
<div>Which does the same thing, but is clearer for anyone
reading the code, since it removes any ambiguity.</div>
<div><br>
</div>
<div>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).<br>
</div>
<div><br>
</div>
<div>Arnaud</div>
<div><br>
</div>
</div>
</div>
<div dir="ltr">
<div><br>
</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">Le ven. 14 déc. 2018 à 02:19, Artem Dergachev
<<a href="mailto:noqnoqneo@gmail.com" target="_blank"
moz-do-not-send="true">noqnoqneo@gmail.com</a>> a écrit :<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF"> 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.<br>
<br>
Yes, indeed, cases like "C" are the reason why this checker
was made.<br>
<br>
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.,<br>
<br>
struct D {<br>
bool Flag;<br>
void setFlag() { Flag = true; } // The flag can be
set later.<br>
<br>
D() : Flag(false) { foo(); } // But its initial
value is "clear".<br>
void foo() { if (Flag) bar(); } // Flag is still
clear when called from D().<br>
virtual void bar() = 0;<br>
}<br>
<br>
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).<br>
<br>
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.<br>
<br>
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.<br>
<br>
<br>
<div
class="gmail-m_4759335717445627117gmail-m_428474311560325971gmail-m_6064893394642050137moz-cite-prefix">On
12/13/18 12:24 PM, Arthur O'Dwyer via cfe-dev wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">On Thu, Dec 13, 2018 at 1:21 PM Arnaud
Bienner via cfe-dev <<a
href="mailto:cfe-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">cfe-dev@lists.llvm.org</a>>
wrote:<br>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px
0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div>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.</div>
<div>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.</div>
</div>
</blockquote>
<div><br>
</div>
<div>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.)</div>
<div><br>
</div>
<div>I think any diagnostic in this area is really
too fragile to be useful. Consider:</div>
<div><br>
</div>
<div> struct A {</div>
<div> A() { do_foo(); } // diagnosable and
definitely a bug</div>
<div> virtual void do_foo() = 0;<br>
</div>
<div> };</div>
<div>
<div> struct B {</div>
<div> B() { do_foo(); } // diagnosable,
but possibly correct</div>
<div> virtual void do_foo();</div>
<div> };</div>
</div>
<div> struct C {</div>
<div> C() { foo(); } // definitely a bug,
but not diagnosable</div>
<div> void foo() { do_foo(); } // best
practice</div>
<div> private:</div>
<div> virtual void do_foo() = 0;</div>
<div> };</div>
<div><br>
</div>
<div>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!</div>
<div><br>
</div>
<div>I think Clang can't <i>remove</i> 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".</div>
<div><br>
</div>
<div>The static analyzer, though, could definitely
catch "C"! I don't know if it does today or not.</div>
<div><br>
</div>
<div>my $.02,</div>
<div>–Arthur</div>
</div>
</div>
</div>
</div>
<br>
<fieldset
class="gmail-m_4759335717445627117gmail-m_428474311560325971gmail-m_6064893394642050137mimeAttachmentHeader"></fieldset>
<pre class="gmail-m_4759335717445627117gmail-m_428474311560325971gmail-m_6064893394642050137moz-quote-pre">_______________________________________________
cfe-dev mailing list
<a class="gmail-m_4759335717445627117gmail-m_428474311560325971gmail-m_6064893394642050137moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" target="_blank" moz-do-not-send="true">cfe-dev@lists.llvm.org</a>
<a class="gmail-m_4759335717445627117gmail-m_428474311560325971gmail-m_6064893394642050137moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank" moz-do-not-send="true">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>