[PATCH] Fix missed exception spec checks and crashes
Alp Toker
alp at nuanti.com
Thu Oct 17 19:08:21 PDT 2013
Hi Richard,
The simplification didn't work out because at least
CheckOverridingFunctionExceptionSpec() pushes back specs where the
parent isn't fully defined yet, causing an infinite loop. This was hit
in the libc++ selfhost build.
So the best simplification I can offer is to remove the outer loop that
was in the original patch. Swapping and processing seems the most
reliable way to delay late additions to the list for further checking
given the assumptions in existing code.
I've attached a rework that now passes all tests and the libc++
bootstrap as well. Does this look OK to you?
Alp.
On 17/10/2013 20:21, Alp Toker wrote:
> Landed in r192914 with suggested simplifications.
>
> With the pop_back() approach the two loops are no longer tied -- in the
> off chance this becomes a problem due to the second mutating the first,
> we can take a look at using the heavier approach in the original patch
> again.
>
> Thanks for the review Richard!
>
> Marshall, let us know if this covers all bases.
>
> Alp.
>
> On 07/10/2013 21:36, Richard Smith wrote:
>> On Sun, Oct 6, 2013 at 11:54 AM, Alp Toker <alp at nuanti.com
>> <mailto:alp at nuanti.com>> wrote:
>>
>> This patch fixes a crash and rejects-valid the libcxx developers were
>> hitting.
>>
>> While investigating the defaulted member cases I noticed a similar
>> problem in delayed destructor checking, with checks accumulating but
>> never running or getting cleared if they were instantiated within an
>> exception spec.
>>
>> I'll appreciate thoughts on both parts of the fix, particularly on the
>> correctness of moving the destructor checks from
>> ActOnFinishCXXMemberDecls() to ActOnFinishDelayedMemberInitializers()
>> alongside the member checks, which seems to be the only way to get
>> them
>> handled consistently.
>>
>>
>> Both changes look correct to me. Rather than swapping the worklists
>> out in each iteration of the loop, can you instead process them in
>> reverse order (and pop them as you go)? This should make the code a
>> little simpler (and a little more efficient too).
>>
>>
>> Marshall, can you think of anything else to excercise in the test
>> case?
>>
>> I've attached the patch and crash backtrace. Here's the changelog:
>>
>> Delayed exception specification checking for defaulted members and
>> virtual destructors are both susceptible to mutation during
>> iteration so
>> we need to swap and process the worklists iteratively.
>>
>> This resolves both accepts-invalid and rejects-valid issues and
>> moreover
>> fixes potential invalid memory access as the contents of the vectors
>> change during iteration and recursive template instantiation.
>>
>> This patch also adds two assertions at end of TU to ensure no
>> specs are
>> left unchecked as was happenning before the fix, plus a test case from
>> Marshall Clow for the defaulted member crash extracted from the libcxx
>> headers.
>>
>> Alp.
>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
--
http://www.nuanti.com
the browser experts
-------------- next part --------------
commit 80d5e8fc68c48d3b8cb834cee72cd3550c6279b4
Author: Alp Toker <alp at nuanti.com>
Date: Thu Oct 3 07:18:14 2013 +0100
Fix missed exception spec checks and crashes
Delayed exception specification checking for defaulted members and virtual
destructors are both susceptible to mutation during iteration so we need to
swap and process the worklists.
This resolves both accepts-invalid and rejects-valid issues and moreover fixes
potential invalid memory access as the contents of the vectors change during
iteration and recursive template instantiation.
Checking can be further delayed where parent classes aren't yet fully defined.
This patch adds two assertions at end of TU to ensure no specs are left
unchecked as was happenning before the fix, plus a test case from Marshall Clow
for the defaulted member crash extracted from the libcxx headers.
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 7699980..65f07a8 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -4790,7 +4790,7 @@ public:
void CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD);
void CheckExplicitlyDefaultedMemberExceptionSpec(CXXMethodDecl *MD,
const FunctionProtoType *T);
- void CheckDelayedExplicitlyDefaultedMemberExceptionSpecs();
+ void CheckDelayedMemberExceptionSpecs();
//===--------------------------------------------------------------------===//
// C++ Derived Classes
diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp
index 5964844..41f72a6 100644
--- a/lib/Sema/Sema.cpp
+++ b/lib/Sema/Sema.cpp
@@ -604,8 +604,14 @@ void Sema::ActOnEndOfTranslationUnit() {
// valid, but we could do better by diagnosing if an instantiation uses a
// name that was not visible at its first point of instantiation.
PerformPendingInstantiations();
+ CheckDelayedMemberExceptionSpecs();
}
+ // All delayed member exception specs should be checked or we end up accepting
+ // incompatible declarations.
+ assert(DelayedDefaultedMemberExceptionSpecs.empty());
+ assert(DelayedDestructorExceptionSpecChecks.empty());
+
// Remove file scoped decls that turned out to be used.
UnusedFileScopedDecls.erase(
std::remove_if(UnusedFileScopedDecls.begin(0, true),
diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
index 81283e1..ed5724b 100644
--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -4873,14 +4873,28 @@ void Sema::CheckExplicitlyDefaultedMemberExceptionSpec(
SpecifiedType, MD->getLocation());
}
-void Sema::CheckDelayedExplicitlyDefaultedMemberExceptionSpecs() {
- for (unsigned I = 0, N = DelayedDefaultedMemberExceptionSpecs.size();
- I != N; ++I)
- CheckExplicitlyDefaultedMemberExceptionSpec(
- DelayedDefaultedMemberExceptionSpecs[I].first,
- DelayedDefaultedMemberExceptionSpecs[I].second);
+void Sema::CheckDelayedMemberExceptionSpecs() {
+ SmallVector<std::pair<const CXXDestructorDecl *, const CXXDestructorDecl *>,
+ 2> Checks;
+ SmallVector<std::pair<CXXMethodDecl *, const FunctionProtoType *>, 2> Specs;
- DelayedDefaultedMemberExceptionSpecs.clear();
+ std::swap(Checks, DelayedDestructorExceptionSpecChecks);
+ std::swap(Specs, DelayedDefaultedMemberExceptionSpecs);
+
+ // Perform any deferred checking of exception specifications for virtual
+ // destructors.
+ for (unsigned i = 0, e = Checks.size(); i != e; ++i) {
+ const CXXDestructorDecl *Dtor = Checks[i].first;
+ assert(!Dtor->getParent()->isDependentType() &&
+ "Should not ever add destructors of templates into the list.");
+ CheckOverridingFunctionExceptionSpec(Dtor, Checks[i].second);
+ }
+
+ // Check that any explicitly-defaulted methods have exception specifications
+ // compatible with their implicit exception specifications.
+ for (unsigned I = 0, N = Specs.size(); I != N; ++I)
+ CheckExplicitlyDefaultedMemberExceptionSpec(Specs[I].first,
+ Specs[I].second);
}
namespace {
@@ -8191,9 +8205,8 @@ void Sema::DefineImplicitDefaultConstructor(SourceLocation CurrentLocation,
}
void Sema::ActOnFinishDelayedMemberInitializers(Decl *D) {
- // Check that any explicitly-defaulted methods have exception specifications
- // compatible with their implicit exception specifications.
- CheckDelayedExplicitlyDefaultedMemberExceptionSpecs();
+ // Perform any delayed checks on exception specifications.
+ CheckDelayedMemberExceptionSpecs();
// Once all the member initializers are processed, perform checks to see if
// any unintialized use is happeneing.
@@ -8716,23 +8729,11 @@ void Sema::ActOnFinishCXXMemberDecls() {
// If the context is an invalid C++ class, just suppress these checks.
if (CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(CurContext)) {
if (Record->isInvalidDecl()) {
+ DelayedDefaultedMemberExceptionSpecs.clear();
DelayedDestructorExceptionSpecChecks.clear();
return;
}
}
-
- // Perform any deferred checking of exception specifications for virtual
- // destructors.
- for (unsigned i = 0, e = DelayedDestructorExceptionSpecChecks.size();
- i != e; ++i) {
- const CXXDestructorDecl *Dtor =
- DelayedDestructorExceptionSpecChecks[i].first;
- assert(!Dtor->getParent()->isDependentType() &&
- "Should not ever add destructors of templates into the list.");
- CheckOverridingFunctionExceptionSpec(Dtor,
- DelayedDestructorExceptionSpecChecks[i].second);
- }
- DelayedDestructorExceptionSpecChecks.clear();
}
void Sema::AdjustDestructorExceptionSpec(CXXRecordDecl *ClassDecl,
diff --git a/test/SemaTemplate/exception-spec-crash.cpp b/test/SemaTemplate/exception-spec-crash.cpp
new file mode 100644
index 0000000..4d93559
--- /dev/null
+++ b/test/SemaTemplate/exception-spec-crash.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++11 -fcxx-exceptions -DCXX_EXCEPTIONS -fsyntax-only -verify %s
+
+template <class _Tp> struct is_nothrow_move_constructible {
+ static const bool value = false;
+};
+
+template <class _Tp>
+class allocator;
+
+template <>
+class allocator<char> {};
+
+template <class _Allocator>
+class basic_string {
+ typedef _Allocator allocator_type;
+ basic_string(basic_string &&__str)
+ noexcept(is_nothrow_move_constructible<allocator_type>::value);
+};
+
+class Foo {
+ Foo(Foo &&) noexcept = default;
+#ifdef CXX_EXCEPTIONS
+// expected-error at -2 {{does not match the calculated}}
+#else
+// expected-no-diagnostics
+#endif
+ Foo &operator=(Foo &&) noexcept = default;
+ basic_string<allocator<char> > vectorFoo_;
+};
More information about the cfe-commits
mailing list