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

[John McCall via cfe-dev]

On 1 Jul 2019, at 16:24, Aaron Ballman wrote:
> On Mon, Jul 1, 2019 at 4:05 PM Arnaud Bienner 
> <arnaud.bienner at gmail.com> wrote:
>>
>> "I think it makes sense to diagnose the obvious case of this
>> in the compiler and let the static analyzer catch the less-obvious 
>> cases,
>> like when the analysis has to be interprocedural
>
> I'm not certain I agree with this approach in general because it
> introduces unfortunate interactions between the static analyzer and
> the compiler that make the user's life worse in practice. Will the
> static analyzer produce duplicate diagnostics? What if the user
> disables the compiler warning with a -Wno- flag, does the static
> analyzer then go back to reporting the otherwise-duplicate warnings?
> Does enabling the check in the static analyzer disable the compiler
> warning in favor of the more accurate reports from the analyzer? Do we
> need to carefully maintain both checks such that there is no
> overlapping diagnostics between them?
>
> To me, if the check cannot be reliably implemented in the compiler, it
> belongs somewhere else (static analyzer, clang-tidy, etc). If we want
> to split diagnostics across multiple tools, I think we should have
> more of a plan in place for how those tools coordinate reporting
> diagnostics so the user doesn't have a poor experience.

There are dozens of problems that we warn about in the compiler for
direct violations and in the static analyzer for more complex cases:
uninitialized memory, dereferencing null, pure-virtual calls in
constructors, and so on.  Nothing about these compiler warnings is
"unreliable"; they just have false negatives (as does the static
analyzer, of course).

John.

>
>> [...] it's just generally better to
>> diagnose things in the compiler when possible"
>> Exactly: from my experience, static analysis is performed at the 
>> commit stage/during CI.
>> Having the possibility to catch issues ealier, as developers are 
>> writing the code (through Werror) can save a lot of time (this is why 
>> I'm insisting a bit on getting this into the compiler :) )
>
> I definitely agree that we want to catch issues earlier rather than
> later when possible. What I don't think is a good idea is having two
> different ways to catch the same issues, where one of the ways will
> have a higher false negative rate compared to the other.
>
> ~Aaron
>
>>
>> Arnaud
>>
>>
>> Le lun. 1 juil. 2019 à 19:37, John McCall <jmccall at apple.com> a 
>> écrit :
>>>
>>> On 1 Jul 2019, at 11:23, Aaron Ballman wrote:
>>>
>>> On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner 
>>> <arnaud.bienner at gmail.com> wrote:
>>>
>>> Thinking again about this, I wonder if having this warning 
>>> implemented, off by default (since Aaron's proposal is not 
>>> implemented yet) couldn't be a good idea anyway.
>>> I was initially convinced by Aaron that adding default-off warnings 
>>> might not be interesting, since not many people are likely to use 
>>> it.
>>> But I realized I will not be the only one to use it: the C++ coding 
>>> rules we are using at the company I'm working in forbid to call 
>>> virtual functions in constructors/destructors, and I realized 
>>> Google's C++ styleguide (which as far as I know I also used by many 
>>> non-Google projects) also forbids this: 
>>> https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
>>> (maybe some Google C++ developers here can comment on how strictly 
>>> those guidelines are enforced in their projects).
>>> CppCoreGuidelines also discourage this: 
>>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-ctor-virtual
>>>
>>> Having a warning to check this behavior (with -Werror) would help 
>>> ensure the code conforms to the coding style and force developers to 
>>> follow this rule.
>>>
>>> With this in mind, do you think we can reconsider having such a 
>>> warning implemented in clang?
>>>
>>> I agree that there is utility in the check and that people will want
>>> something like this. CERT also has a secure coding rule covering 
>>> this
>>> (https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors).
>>>
>>> However, we have the optin.cplusplus.VirtualCall check already
>>> implemented in the static analyzer which should meet all those needs
>>> without adding an off-by-default diagnostic to the compiler. I've 
>>> not
>>> seen much discussion on why we shouldn't be improving that check
>>> instead.
>>>
>>> Abstractly, I think it makes sense to diagnose the obvious case of 
>>> this
>>> in the compiler and let the static analyzer catch the less-obvious 
>>> cases,
>>> like when the analysis has to be interprocedural. That's a standard
>>> trade-off we make for a lot of diagnostics: it's just generally 
>>> better to
>>> diagnose things in the compiler when possible.
>>>
>>> If we take that as given, then the question is just whether our 
>>> general
>>> policy of "(mostly) no new opt-in warnings" should have an exception 
>>> for
>>> warnings that we'd like to be opt-out except for the lack of 
>>> something
>>> like -Wversion.
>>>
>>> John.
>>>
>>> Le lun. 14 janv. 2019 à 20:49, Aaron Ballman 
>>> <aaron at aaronballman.com> a écrit :
>>>
>>> On Mon, Jan 14, 2019 at 2:44 PM John McCall <jmccall at apple.com> 
>>> wrote:
>>>
>>> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
>>>
>>> On Mon, Jan 14, 2019 at 2:31 PM John McCall <jmccall at apple.com> 
>>> wrote:
>>>
>>> On 14 Jan 2019, at 14:23, Aaron Ballman wrote:
>>>
>>> On Mon, Jan 14, 2019 at 1:59 PM John McCall via cfe-dev
>>> <cfe-dev at lists.llvm.org> wrote:
>>>
>>> On 11 Jan 2019, at 11:23, Arnaud Bienner wrote:
>>>
>>> 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.
>>>
>>> I agree.
>>>
>>> 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?
>>>
>>> I think it should go in, default-off, and we should implement my
>>> proposal
>>>
>>> AIUI, experience has shown that off-by-default warning flags almost
>>> never get enabled by users. I don't think we should have a new,
>>> default off warnings unless there is a strong use case suggesting
>>> someone will actually enable it.
>>>
>>> See my proposal earlier in the thread for how we can evolve the set
>>> of
>>> "always"-on warnings more aggressively without breaking source
>>> compatibility.
>>>
>>> I saw it (and think it's a good proposal), but without something 
>>> like
>>> that, this diagnostic is unlikely to ever be enabled by default, so
>>> why should we adopt it when there are better near-term alternatives
>>> for the feature (such as improving the static analyzer)?
>>>
>>> Okay, so your argument is that we should try moving forward with 
>>> that
>>> proposal as a pre-requisite to adding warnings that would otherwise 
>>> have
>>> to be default-off? That seems reasonable.
>>>
>>> Yes, that would be my preference. It's a more conservative approach,
>>> but I think that's not too bad given that the warnings would 
>>> otherwise
>>> be off-by-default anyway.
>>>
>>> ~Aaron
>>>
>>> John.
>>>
>>> ~Aaron
>>>
>>> John.
>>>
>>> ~Aaron
>>>
>>> as a way of eventually making it default-on. Unfortunately, I
>>> don't
>>> really
>>> have time to implement my proposal right now; I'm way backed up as
>>> it
>>> is.
>>>
>>> John.
>>>
>>> Arnaud
>>>
>>>
>>>
>>>
>>>
>>> Le mer. 9 janv. 2019 à 01:14, John McCall <jmccall at apple.com> a
>>> écrit :
>>>
>>> On 8 Jan 2019, at 15:03, Arthur O'Dwyer wrote:
>>>
>>> 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.
>>>
>>> On Tue, Jan 8, 2019 at 2:37 PM John McCall <jmccall at apple.com>
>>> wrote:
>>>
>>> On 8 Jan 2019, at 13:45, Arthur O'Dwyer wrote:
>>>
>>> On Tue, Jan 8, 2019 at 1:05 PM John McCall via cfe-dev <
>>> cfe-dev at lists.llvm.org> wrote:
>>>
>>> On 8 Jan 2019, at 8:37, Arnaud Bienner wrote:
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev