Warning when calling virtual functions from constructor/desctructor?

Clang Frontend
1

Hi all :slight_smile:

I noticed that clang doesn’t emit a warning in this case.
It does when calling a pure virtual function though (a warning for which I added a diagnostic group recently 1).
I think one difference is that calling pure virtual function from constructor is undefined, while calling virtual function is defined (it calls the base class function since the vtable isn’t filled yet with inherited virtual functions reimplementations).
So implementing a warning like this will likely trigger false positives.

However, I think in most cases it denotes a bug, since many IMHO C++ developers won’t get that the function will not behave as they might expect.
Also, we can have a fixit to this, by suggesting the user to to specify the class of the function e.g. write “B::f()” instead of “f()”.

I’m fine (trying to) write a patch for this, but first I wanted to get opinions about whether it’s worth it.
I guess it might not be that much difficult to do for simple cases (when calling virtual function directly). I expect it might be more difficult to catch cases where we call a function that is not virtual, but which calls a virtual function behind the scene.

Also, if you’re worried about having too many false positives, I think we could put this new warning behind Wextra.

What do you think?

Best regards,
Arnaud

2

IIRC we tried this and it had a very high false positive rate. I might be misremembering though.

Note that -Wextra generally tries to be empty, and for -Wall we still don’t want warnings with lots of false positives.

3

Hi all :slight_smile:

I noticed that clang doesn't emit a warning in this case.
It does when calling a *pure* virtual function though (a warning for which I added a diagnostic group recently [1]).
I think one difference is that calling pure virtual function from constructor is undefined, while calling virtual function is defined (it calls the base class function since the vtable isn't filled yet with inherited virtual functions reimplementations).
So implementing a warning like this will likely trigger false positives.

However, I think in most cases it denotes a bug, since many IMHO C++ developers won't get that the function will not behave as they might expect.
Also, we can have a fixit to this, by suggesting the user to to specify the class of the function e.g. write "B::f()" instead of "f()".

I'm fine (trying to) write a patch for this, but first I wanted to get opinions about whether it's worth it.
I guess it might not be that much difficult to do for simple cases (when calling virtual function directly). I expect it might be more difficult to catch cases where we call a function that is not virtual, but which calls a virtual function behind the scene.
Also, if you're worried about having too many false positives, I think we could put this new warning behind Wextra.

What do you think?

The static analyzer currently has alpha.cplusplus.VirtualCall that
should catch this sort of issue. Given the concerns about false
positives, perhaps that check could be improved enough to pull it out
of alpha status instead of working on a check within clang itself?

~Aaron

4

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.
On the other hand, I understand that you don’t want this kind of diagnostic to significantly slow down the compilation process (I guess that would be the case here, but I’m not sure).

Arnaud

5

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

6

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.

7

Thanks all for the feedback :slight_smile:
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

8

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.

9

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.

In general, warnings in Clang are expected to be on-by-default and
have an extremely low false positive rate (certainly lower than for
the static analyzer). I suspect this warning would not meet the usual
bar for inclusion as a Clang warning given the number of false
positives it's shown in the past. I think the static analyzer is a
better place for the functionality to live, especially given that it
requires actual static analysis of runtime control flows to reduce
those false positives.

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.

Agreed that this would be a sensible way for users to suppress the
diagnostic -- it clarifies that the programmer understands the
behavior of the call.

~Aaron

10

So I wrote a patch to diagnose simple cases like B.
https://reviews.llvm.org/D56366
I think most complicated cases should be catch by a static analyzer indeed, especially since they might be harder to fix (if the you call a function ‘f’ from the constructor, and ‘f’ calls a virtual function: what if if ‘f’ is also called after the object has been created?)
Cases like B are easy to fix, and only benefit from being fixed, as noted earlier.

I run my patched-clang on the Firefox codebase, without any occurrence of the warning. I guess that might be because calling virtual functions from constructors/destructors is likely to create issues (like said previously), and so is not common in this codebase.

If someone wants to try this patch on another codebase and share some feedback, I would be curious to know how it goes!

Or if you have suggestion about some additional open source projects I could try building with this new warning, I’m interested too.

Arnaud

11

Maybe the Firefox devs already use cppcheck, which can already diagnose that issue:
<https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/&gt;

12

Agreed.

In general, whether or not to add a new default-on warning comes down to two
somewhat orthogonal considerations:

  • Are we willing to say that the code pattern is Wrong?

    This involves weighing the harm of unknowing uses of the code pattern
    against the harm of avoiding the diagnostic.

  • Are knowing, correct uses of the code pattern too common in practice
    to justify annoying a large fraction of the community?

    This involves weighing the diagnostic’s impact on all code, which is
    impossible, so we have to muddle through with incomplete testing.

For this particular diagnostic, I’m not sure about either of those points:

  • The code pattern is definitely not always harmful. Most importantly, the
    behavior is only confusing if the method is actually overridden in a subclass.

  • There’s probably a lot of code which does this innocuously. The usual
    admonition that running code is usually correct is stronger than usual here
    because most code in a constructor or destructor will actually be run on the
    common path.

  • Some programmers will reasonably object that they know the semantics here.

  • The workaround can be a bit verbose.

  • The best workaround is to name the current class, but programmers might find
    that weird if it’s not where the method is actually defined. If they instead
    try to name the defining class, the workaround is making the code more brittle.

So I’m not sure if this is a good warning to add. I agree that it may be better
suited for the static analyzer, which can try to ameliorate at least some of
these concerns.

That said, I’m concerned about the growing pressure to not add new
warnings. That’s not an intended goal of the pressure, but it is the basic
outcome. There is a tightly-woven web of different factors that have the
effect of discouraging the addition of any new diagnostics:

  • We discourage adding new default-off warnings on the theories that
    (1) warnings should have maximal impact and (2) we don’t want unused
    complexity in the compiler.

  • New default-on warnings are immediately exercised on a massive amount of
    code and will be reverted if they cause any problems for existing code because
    we try to keep the codebase as close as possible to an acceptable release
    state at all times.

  • Project maintainers quite reasonably feel that (1) they did not ask for this
    new warning and (2) they don’t really want to deal with it right now but (3)
    it’s being forced on them anyway and (4) they are rather upset about that.

The basic problem here is that we have no way for projects to evolve with
default-on warnings, and so we necessarily conflate two very different ideas:

  • We want the warning to eventually be useful to as many people as possible.

  • We want the warning to be useful to everyone right now.

But the first doesn’t require the second. It would make a lot more sense
if we curated an ever-expanding but versioned set of default-on warnings
and allowed users to ratchet their version forward at their own pace.
More precisely:

  1. We would add a flag to control the current default diagnostic settings,
    something like -Wversion=clang-7 to say to use Clang 7’s default warning
    behavior.

  2. The first version of that would turn on a whole bunch of warnings that we’ve
    always thought ought to be on by default but aren’t because of GCC compatibility.

  3. In the absence of an explicit -Wversion flag, the compiler would continue
    using the supposedly-GCC-compatible defaults.

  4. It would be understood that -Wversion=X+1 will generally be defined as
    -Wversion=X -Wfoo -Wbar -Wbaz. (Not literally in terms of the set-algebra
    of warnings flags, just in terms of each version adding new warnings.)

  5. Project maintainers can roll forward their per-compiler warning versions
    when they’re ready to deal with the consequences.

  6. The compiler would not complain if it’s passed a future version; it
    would just use its most recent rules. This allows projects to easily
    roll their warnings version forward while still working on older compilers.
    We would clearly document that projects which intentionally post-date
    their warnings version do so at their own risk.

  7. Platform maintainers and distributors like Google and Apple (and other
    motivated projects like e.g. Mozilla) would periodically qualify the current
    -Wversion on their codebases in order to gather feedback on the current
    set of newly-added warnings.

  8. Tools like Xcode can make their own decisions about rolling forward warning
    versions. For example, a new project might be configured to use the latest
    warning settings but then stay with that version until manually updated.

This flag isn’t meant to be a guarantee that we’ll emit exactly the same
diagnostics as the target version. For one, of course, we might change
diagnostic text or emit the diagnostic somewhere else or add a note or whatever.
More importantly, I think we’d still reserve the right to add new warnings
that are unconditionally on by default if we think they’re important or
unproblematic enough.

This wouldn’t be a blank check to add new default-on warnings: the two basic
considerations above would still apply. It’s just that the second
consideration would be far more focused:

  • It wouldn’t need to account for the impact on legacy projects that might
    lack an active maintainer. Someone who wants to audit the new warnings on
    that project can easily do so, but otherwise the build is unaffected.

  • It wouldn’t need to account for the inter-project politics of compilers
    forcing unwanted new warnings on other programmers. A project maintainer
    who intentionally rolls their version forward but doesn’t like one of the
    new warnings can easily deal with that problem themselves.

  • Instead, it would just evaluate the impact from the perspective of a project
    maintainer who has voluntarily requested a larger set of warnings. Again,
    that wouldn’t be a blank check: the warning still needs to be worthwhile.
    But it’s necessarily a less fraught prospect.

There would also be a very clear path for warnings under active development:
our expectation would be that most new warnings would eventually be added to
the set of default-on warnings. In my opinion, that leaves room for warnings
to remain default-off as long as we expect that they will eventually become
acceptable to include.

John.

13

Thanks for the detailed answer.

The code pattern is definitely not always harmful. Most importantly, the

behavior is only confusing if the method is actually overridden in a subclass.

In this regard, note that my patch won’t warn if the class is final.
Otherwise, indeed it might work as expected now but won’t once reimplemented in a subclass. Qualifying the call make things clearer.

The best workaround is to name the current class, but programmers might find
that weird if it’s not where the method is actually defined. If they instead
try to name the defining class, the workaround is making the code more brittle.

Yes indeed: I named the current class instead of the defining one intentionally in the fixit, because that would make the code behaves like if it wasn’t qualify, if one day the method gets reimplemented in the current class.
If the developer decides qualify the call using the defining class anyway, at least it will be clear what implementation will get called (but agreed this would make the code more brittle).

I realized I didn’t put “DefaultIgnore” on this warning.
I’m not experienced enough with clang to know what’s the best way to deal with new warnings, but my feeling is that it would be sensible to have this new warning DefaultIgnore for now, in -Wall, and make it default once we have some feedback from the community: while not all C++ projects use -Wall (or -Wextra) I believe enough do to give us a chance to get some feedback.

What do you think?

Arnaud

14

Thanks for the detailed answer.

The code pattern is definitely not always harmful. Most importantly, the
behavior is only confusing if the method is actually overridden in a

subclass.

In this regard, note that my patch won't warn if the class is final.

Sure, that's a very reasonable improvement.

Otherwise, indeed it might work as expected *now* but won't once
reimplemented in a subclass. Qualifying the call make things clearer.

I don't disagree, but this doesn't diminish the fact that the behavior is
actually innocuous today (and it will probably be re-compiled / re-checked
after such a subclass is introduced).

The best workaround is to name the current class, but programmers might

find

that weird if it's not where the method is actually defined. If they

instead

try to name the defining class, the workaround is making the code more

brittle.

Yes indeed: I named the *current* class instead of the defining one
intentionally in the fixit, because that would make the code behaves like
if it wasn't qualify, if one day the method gets reimplemented in the
current class.

Good.

If the developer decides qualify the call using the defining class anyway,
at least it will be clear what implementation will get called (but agreed
this would make the code more brittle).

I realized I didn't put "DefaultIgnore" on this warning.
I'm not experienced enough with clang to know what's the best way to deal
with new warnings, but my feeling is that it would be sensible to have this
new warning DefaultIgnore for now, in -Wall, and make it default once we
have some feedback from the community: while not all C++ projects use -Wall
(or -Wextra) I believe enough do to give us a chance to get some feedback.

What do you think?

We generally don't add things to -Wall. That's why I went into my whole spiel
about versioning: I think it's a conversation we need to have before we're
ready to accept this as a warning that's anything but hidden permanently
behind its own opt-in flag.

John.

15

John: Wha? Clang frequently adds things to -Wall! -Wall includes -Wmost which includes a bunch of other categories, so while we don’t often put new diagnostics directly under -Wall, pretty much every “reasonable” diagnostic eventually winds up in there somehow — which is the intent.

However, Arnaud: I don’t think there’s much difference between “on-by-default” and “on only with -Wall”, because the received wisdom has always been to always compile with -W -Wall [-Wextra]. You’re saying “don’t worry, my change will affect only the people who pass -Wall” — but that’s actually a huge percentage of the world, like maybe 90% (wild unscientific guess). If you’re worried that your diagnostic might be “unreasonable,” then it doesn’t belong in -Wall any more than it belongs on-by-default.

I think Richard Smith’s suggestion was right: the decision of where to put this diagnostic will become simpler if the diagnostic itself becomes more targeted and loses some of its current false positives.

my $.02,
Arthur

16

I realized I didn’t put “DefaultIgnore” on this warning.
I’m not experienced enough with clang to know what’s the best way to deal
with new warnings, but my feeling is that it would be sensible to have this
new warning DefaultIgnore for now, in -Wall, and make it default once we
have some feedback from the community: while not all C++ projects use -Wall
(or -Wextra) I believe enough do to give us a chance to get some feedback.

What do you think?

We generally don’t add things to -Wall. That’s why I went into my whole spiel
about versioning: I think it’s a conversation we need to have before we’re
ready to accept this as a warning that’s anything but hidden permanently
behind its own opt-in flag.

John: Wha? Clang frequently adds things to -Wall! -Wall includes -Wmost which includes a bunch of other categories, so while we don’t often put new diagnostics directly under -Wall, pretty much every “reasonable” diagnostic eventually winds up in there somehow — which is the intent.

However, Arnaud: I don’t think there’s much difference between “on-by-default” and “on only with -Wall”, because the received wisdom has always been to always compile with -W -Wall [-Wextra]. You’re saying “don’t worry, my change will affect only the people who pass -Wall” — but that’s actually a huge percentage of the world, like maybe 90% (wild unscientific guess). If you’re worried that your diagnostic might be “unreasonable,” then it doesn’t belong in -Wall any more than it belongs on-by-default.

I think Richard Smith’s suggestion was right: the decision of where to put this diagnostic will become simpler if the diagnostic itself becomes more targeted and loses some of its current false positives.

OOPS: sorry, Richard’s comment that I was thinking of was on https://reviews.llvm.org/D56405, which is also about a “virtual”-related diagnostic but not directly relevant to Arnaud’s diagnostic IIUC.

17

I don't think we "frequently" add things to -Wall or -Wmost. We do somewhat
frequently add warnings that are unconditionally default-on, but the groups
have a conventional meaning that we don't generally touch. What recently-added
warnings are you thinking of that are not default-on but which are included
in a group like -Wall or -Wmost?

John.

18

I was thinking of -Wreturn-std-move, which is -Wmove/-Wmost/-Wall but not always-on.
Grepping the code for DefaultIgnore, I see that -Wfor-loop-analysis is another example (but 4 years old).

19

I was thinking of -Wreturn-std-move, which is -Wmove/-Wmost/-Wall but not
always-on.

I honestly don’t know why this isn’t default-on.

Grepping the code for DefaultIgnore, I see that -Wfor-loop-analysis is
another example (but 4 years old).

This is a harder call. At any rate, I think my point stands that this is not
“frequent”.

There’s a fairly substantial difference between warnings that target patterns
that are inarguably bad, like the std::move problem (which in some sense is
a language defect that people just need to be taught how to work around), and
warnings that target code patterns that might be confusing or which have a
higher-than-normal chance of being a typo. -Wparentheses is the classic
example here: it unquestionably catches a common mistake that C unfortunately
otherwise masks, but it’s still perennially controversial because the
assign-and-test idiom is so common in C programming, and there are a lot of
people who still swear by reversing the equality test (0 == foo) instead of
relying on the warning.

John.

20

There’s a fairly substantial difference between warnings that target patterns
that are inarguably bad, like the std::move problem (which in some sense is
a language defect that people just need to be taught how to work around), and
warnings that target code patterns that might be confusing or which have a
higher-than-normal chance of being a typo. -Wparentheses is the classic

example here

Yes, and this warning is definitely like -Wparentheses: something that could be wrong, but is not necessarily.
Still, I think it will catch tricky bugs.

What should we do about this warning/patch?
Deactivate it by default, with its own flag? without putting it in Wall/extra?
Or wait for something like your proposal to be implemented?

Or just forget about it for now and simply wait for more feedback from the mailing before deciding?

Arnaud