This is an archive of the discontinued LLVM Phabricator instance.

Warn about UB at member function calls from base class ctor initializers.
Needs ReviewPublic

Authored by teemperor on Apr 20 2016, 2:00 AM.

Details

Reviewers
rsmith
Summary
According to [C++14/17 12.6.2 p16] [C++11 12.6.2 p13] calling member functions of the current
class before all the base classes are initialized is undefined behavior.
Some compilers (such as GCC 5.3 + ubsan) can produce a segfaulting code 
in the program when such code is encountered (see GCC bug #70035),
so clang should generate a warning about the UB.

Diff Detail

Event Timeline

teemperor updated this revision to Diff 54330.Apr 20 2016, 2:00 AM
teemperor retitled this revision from to Warn about UB at member function calls from base class ctor initializers..
teemperor updated this object.
teemperor added a reviewer: rsmith.
teemperor added a subscriber: cfe-commits.
teemperor updated this revision to Diff 54343.Apr 20 2016, 4:41 AM
  • Fixed indentation
filcab added a subscriber: filcab.Apr 20 2016, 7:20 AM

You might want to mention that it's 12.6.2p16 in C++14/17 but p13 in C++11.
I wonder if we should have the example in the standard, verbatim. (Plus the added tests you made)

teemperor updated this object.Apr 20 2016, 8:16 AM

I meant changing the diff, but if you prefer to have a smaller comment, I'm ok with the different "paths" in the standards being mentioned only in the commit message.

lib/Sema/SemaDeclCXX.cpp
3981

For someone reading the source code, it's probably best to mention the different "paths" in the standards here too.
For my usual source browsing, as long as it's in the comment *or* the commit message, I'll eventually see it. But it might throw some people off when they look at C++11, and there's no p16 in that place :-)

Oh, totally forgot that I also had a reference in the code, thanks! Will fix it ASAP.

Adding the example from the standard to the test cases sounds good. It actually tests a few things that aren't covered by the current test.

teemperor updated this revision to Diff 54464.Apr 21 2016, 12:42 AM
  • Standard reference is now specifying what standard (C++11)
  • Added test based on the standard example
  • Also check base classes now for member calls (to pass the test from the standard example).

The patch needs to get through clang-format.
Should we deal with typeid and dynamic_cast, since they're mentioned in the same paragraph in the standard?

lib/Sema/SemaDeclCXX.cpp
3964

What's the rationale for warning if getRecordDecl returns nullptr?

3966

Please run clang-format on the patch.

3980

clang-format

3981

"before the base class is initialized"?

rtrieu added a subscriber: rtrieu.Apr 21 2016, 2:13 PM

Have you looked at http://reviews.llvm.org/D6561 which was a previous attempt at this warning?

teemperor marked 4 inline comments as done.Apr 22 2016, 2:53 PM

No, didn't saw the thread so far. I assume the comments in there are still valid, so I'll update this patch.
Thanks for the feedback so far!

teemperor updated this revision to Diff 55189.Apr 27 2016, 5:09 AM
  • Moved checks to the UninitializedFieldVisitor
  • Also check for dynamic_cast on this during construction
teemperor updated this revision to Diff 55245.Apr 27 2016, 9:06 AM
  • Added checks and tests for typeid
  • Moved warnings into same warning group
  • Check that only possibly evaluated expressions are checked
teemperor updated this revision to Diff 55716.Apr 30 2016, 7:51 AM

Merged all tests into one file.

NoQ added a subscriber: NoQ.Jul 8 2019, 5:39 PM

Sorry, couldn't help it. This patch looks so useful and so lonely :)

include/clang/Basic/DiagnosticSemaKinds.td
6782

I don't see these things referred to as "ctor-initializers" in other diagnostics. I'd rather say "initializer".

Also "has undefined behavior" is shorter and nicer.

lib/Sema/SemaDeclCXX.cpp
2295

I'd be scared not to initialize this variable to null.

Also, this visitor scans only one constructor throughout its lifetime, right?

2588–2589

Ugh.

This is worth a test case!