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

Mon Jul 1 13:24:06 PDT 2019

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.

> [...] 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