[PATCH] D52956: Support `-fno-visibility-inlines-hidden`
elsteveogrande accepted this revision. elsteveogrande added a comment. This revision is now accepted and ready to land. lgtm! Repository: rC Clang https://reviews.llvm.org/D52956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50948: lambdas in modules: change storage for LambdaDefinitionData
elsteveogrande added a comment. I did try a copy of https://reviews.llvm.org/D50949 again, locally and without this diff, to see if I can avoid messing with this structure altogether. But then I get a `SEGV`. The reason behind this diff is to try to make the logic a little safer, though it's probably still not bulletproof. It at least does seem to fix, or "fix" (i.e. hide) the crash below. I admit I didn't take the time to really dig into that one, and I'm not yet experienced enough in Clang's code to do so confidently, so it's a bit scary to continue going down rabbit holes :) But I'm happy to revise (or go fix that problem) if you think that's best. Thanks again! Stack dump: 0.Program arguments: /opt/llvm/build/bin/clang -cc1 -internal-isystem /opt/llvm/build/lib/clang/8.0.0/include -nostdsysteminc -fmodules -verify /opt/llvm/llvm/tools/clang/test/Modules/merge-lambdas.cpp -emit-llvm-only 1./opt/llvm/llvm/tools/clang/test/Modules/merge-lambdas.cpp:73:1: at annotation token 2./opt/llvm/build/tools/clang/test/Modules/PR38531.map:11:14: LLVM IR generation of declaration 'foo(Fun)::(anonymous class)::' 0 clang0x0001032cffe8 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40 1 clang0x0001032cf165 llvm::sys::RunSignalHandlers() + 85 2 clang0x0001032d05f2 SignalHandler(int) + 258 3 libsystem_platform.dylib 0x7fff79709f5a _sigtramp + 26 4 libsystem_platform.dylib 0x7ffeed8dbe3f _sigtramp + 1948065535 5 clang0x000104b865b1 (anonymous namespace)::CXXNameMangler::manglePrefix(clang::DeclContext const*, bool) + 545 6 clang0x000104b877d5 (anonymous namespace)::CXXNameMangler::mangleNestedName(clang::NamedDecl const*, clang::DeclContext const*, llvm::SmallVector const*, bool) + 453 7 clang0x000104b8733d (anonymous namespace)::CXXNameMangler::mangleLocalName(clang::Decl const*, llvm::SmallVector const*) + 1613 8 clang0x000104b77bf7 (anonymous namespace)::CXXNameMangler::mangleFunctionEncoding(clang::FunctionDecl const*) + 87 9 clang0x000104b760c5 (anonymous namespace)::ItaniumMangleContextImpl::mangleCXXCtor(clang::CXXConstructorDecl const*, clang::CXXCtorType, llvm::raw_ostream&) + 245 10 clang0x00010368eb21 getMangledNameImpl(clang::CodeGen::CodeGenModule const&, clang::GlobalDecl, clang::NamedDecl const*, bool) + 305 11 clang0x00010368a83b clang::CodeGen::CodeGenModule::getMangledName(clang::GlobalDecl) + 427 12 clang0x000103697135 clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) + 1941 13 clang0x0001036f7529 (anonymous namespace)::ItaniumCXXABI::EmitCXXConstructors(clang::CXXConstructorDecl const*) + 41 14 clang0x00010371401f (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) + 143 15 clang0x00010367a151 clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) + 177 16 clang0x00010367a244 clang::BackendConsumer::HandleInterestingDecl(clang::DeclGroupRef) + 20 17 clang0x00010427c2db clang::ASTReader::PassInterestingDeclsToConsumer() + 123 18 clang0x000104243f8b clang::ASTReader::FinishedDeserializing() + 1867 19 clang0x00010427be48 clang::ASTReader::ReadDeclRecord(unsigned int) + 2808 20 clang0x00010422b219 non-virtual thunk to clang::ASTReader::GetExternalDecl(unsigned int) + 89 21 clang0x000104afe7a1 clang::RedeclarableTemplateDecl::loadLazySpecializationsImpl() const + 241 22 clang0x000104aff1dc clang::ClassTemplateDecl::findSpecialization(llvm::ArrayRef, void*&) + 28 23 clang0x0001048096a2 clang::Sema::CheckTemplateIdType(clang::TemplateName, clang::SourceLocation, clang::TemplateArgumentListInfo&) + 1170 24 clang0x00010480c900 clang::Sema::ActOnTemplateIdType(clang::CXXScopeSpec&, clang::SourceLocation, clang::OpaquePtr, clang::IdentifierInfo*, clang::SourceLocation, clang::SourceLocation, llvm::MutableArrayRef, clang::SourceLocation, bool, bool) + 1552 25 clang0x0001041df3b1 clang::Parser::AnnotateTemplateIdTokenAsType(bool) + 129 26 clang0x00010416449a clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*) + 10410 27 clang0x0001041eb69b clang::Parser::ParseDeclOrFunctionDefInternal(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec&, clang::Acce
[PATCH] D50948: lambdas in modules: change storage for LambdaDefinitionData
elsteveogrande updated this revision to Diff 166148.
elsteveogrande added a comment.
Change `Optional` to `unique_ptr` instead, to at least save an allocation and
keep the same semantics. Reverified with `ninja check-clang`.
Repository:
rC Clang
https://reviews.llvm.org/D50948
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -6010,7 +6010,7 @@
void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
auto &Data = D->data();
- Record->push_back(Data.IsLambda);
+ Record->push_back(Data.hasLambdaData()); // lambda-specific data added below
Record->push_back(Data.UserDeclaredConstructor);
Record->push_back(Data.UserDeclaredSpecialMembers);
Record->push_back(Data.Aggregate);
@@ -6068,8 +6068,6 @@
if (ModulesDebugInfo)
Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
- // IsLambda bit is already saved.
-
Record->push_back(Data.NumBases);
if (Data.NumBases > 0)
AddCXXBaseSpecifiers(Data.bases());
@@ -6085,8 +6083,8 @@
AddDeclRef(D->getFirstFriend());
// Add lambda-specific data.
- if (Data.IsLambda) {
-auto &Lambda = D->getLambdaData();
+ if (Data.hasLambdaData()) {
+auto &Lambda = *Data.LambdaData;
Record->push_back(Lambda.Dependent);
Record->push_back(Lambda.IsGenericLambda);
Record->push_back(Lambda.CaptureDefault);
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1636,7 +1636,7 @@
void ASTDeclReader::ReadCXXDefinitionData(
struct CXXRecordDecl::DefinitionData &Data, const CXXRecordDecl *D) {
- // Note: the caller has deserialized the IsLambda bit already.
+ // Note: the caller has deserialized the bit for `isLambda()` already.
Data.UserDeclaredConstructor = Record.readInt();
Data.UserDeclaredSpecialMembers = Record.readInt();
Data.Aggregate = Record.readInt();
@@ -1703,10 +1703,10 @@
assert(Data.Definition && "Data.Definition should be already set!");
Data.FirstFriend = ReadDeclID();
- if (Data.IsLambda) {
+ if (Data.hasLambdaData()) {
using Capture = LambdaCapture;
-auto &Lambda = static_cast(Data);
+auto &Lambda = *Data.LambdaData;
Lambda.Dependent = Record.readInt();
Lambda.IsGenericLambda = Record.readInt();
Lambda.CaptureDefault = Record.readInt();
@@ -1761,7 +1761,8 @@
PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
-assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
+assert(!DD.hasLambdaData() && !MergeDD.hasLambdaData() &&
+ "faked up lambda definition?");
PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
// Don't change which declaration is the definition; that is required
@@ -1826,7 +1827,6 @@
MATCH_FIELD(ImplicitCopyAssignmentHasConstParam)
OR_FIELD(HasDeclaredCopyConstructorWithConstParam)
OR_FIELD(HasDeclaredCopyAssignmentWithConstParam)
- MATCH_FIELD(IsLambda)
#undef OR_FIELD
#undef MATCH_FIELD
@@ -1845,7 +1845,7 @@
// FIXME: Issue a diagnostic if FirstFriend doesn't match when we come to
// lazily load it.
- if (DD.IsLambda) {
+ if (DD.hasLambdaData()) {
// FIXME: ODR-checking for merging lambdas (this happens, for instance,
// when they occur within the body of a function template specialization).
}
@@ -1860,17 +1860,17 @@
}
void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update) {
- struct CXXRecordDecl::DefinitionData *DD;
ASTContext &C = Reader.getContext();
+ struct CXXRecordDecl::DefinitionData *DD =
+ new (C) struct CXXRecordDecl::DefinitionData(D);
+
// Determine whether this is a lambda closure type, so that we can
- // allocate the appropriate DefinitionData structure.
- bool IsLambda = Record.readInt();
- if (IsLambda)
-DD = new (C) CXXRecordDecl::LambdaDefinitionData(D, nullptr, false, false,
- LCD_None);
- else
-DD = new (C) struct CXXRecordDecl::DefinitionData(D);
+ // set the LambdaDefinitionData (into optional \c LambdaData field) as well.
+ bool isLambda = Record.readInt();
+ if (isLambda) {
+DD->SetLambdaDefinitionData(nullptr, false, false, LCD_None);
+ }
CXXRecordDecl *Canon = D->getCanonicalDecl();
// Set decl definition data before reading it, so that during deserialization
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
[PATCH] D50949: lambdas in modules: handle lambdas in .pcm [de]serialization
elsteveogrande updated this revision to Diff 166130.
elsteveogrande added a comment.
rebase atop https://reviews.llvm.org/D50948 (correctly this time) to exclude
those changes
Repository:
rC Clang
https://reviews.llvm.org/D50949
Files:
include/clang/AST/LambdaCapture.h
lib/Serialization/ASTReaderDecl.cpp
test/Modules/merge-lambdas.cpp
Index: test/Modules/merge-lambdas.cpp
===
--- test/Modules/merge-lambdas.cpp
+++ test/Modules/merge-lambdas.cpp
@@ -49,3 +49,25 @@
#pragma clang module import A
void (*p)() = f();
+
+#pragma clang module build PR38531
+module PR38531 {}
+#pragma clang module contents
+#pragma clang module begin PR38531
+template
+struct S {};
+struct Fun {
+ template ())>
+ Fun(F) {}
+};
+template
+void foo(Fun=[]{}) {}
+struct T {
+ void func() {
+foo();
+ }
+};
+#pragma clang module end
+#pragma clang module endbuild
+#pragma clang module import PR38531
+S s;
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1761,8 +1761,6 @@
PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
-assert(!DD.hasLambdaData() && !MergeDD.hasLambdaData() &&
- "faked up lambda definition?");
PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
// Don't change which declaration is the definition; that is required
@@ -1845,9 +1843,41 @@
// FIXME: Issue a diagnostic if FirstFriend doesn't match when we come to
// lazily load it.
- if (DD.hasLambdaData()) {
-// FIXME: ODR-checking for merging lambdas (this happens, for instance,
-// when they occur within the body of a function template specialization).
+ assert (DD.hasLambdaData() == MergeDD.hasLambdaData() &&
+ "Merging definitions, one of which is a lambda, the other not?");
+ if (DD.hasLambdaData() && MergeDD.hasLambdaData()) {
+auto &LDD = *DD.LambdaData;
+auto &MergeLDD = *MergeDD.LambdaData;
+
+#define MATCH_LAM_FIELD(Field) \
+DetectedOdrViolation |= LDD.Field != MergeLDD.Field;
+MATCH_LAM_FIELD(Dependent)
+MATCH_LAM_FIELD(IsGenericLambda)
+MATCH_LAM_FIELD(CaptureDefault)
+MATCH_LAM_FIELD(NumCaptures)
+MATCH_LAM_FIELD(NumExplicitCaptures)
+MATCH_LAM_FIELD(ManglingNumber)
+#undef MATCH_LAM_FIELD
+
+auto *C1 = LDD.Captures;
+auto *C2 = MergeLDD.Captures;
+for (int I = 0; !DetectedOdrViolation && I < LDD.NumCaptures; ++I) {
+ if (C1[I] != C2[I]) {
+DetectedOdrViolation = true;
+ }
+}
+
+if (LDD.MethodTyInfo || MergeLDD.MethodTyInfo) {
+ if (!LDD.MethodTyInfo ||
+ !MergeLDD.MethodTyInfo ||
+ (LDD.MethodTyInfo->getType().getTypePtrOrNull() !=
+ MergeLDD.MethodTyInfo->getType().getTypePtrOrNull())) {
+DetectedOdrViolation = true;
+ }
+}
+
+// FIXME: Issue a diagnostic if ContextDecl doesn't match when we come to
+// lazily load it.
}
if (D->getODRHash() != MergeDD.ODRHash) {
Index: include/clang/AST/LambdaCapture.h
===
--- include/clang/AST/LambdaCapture.h
+++ include/clang/AST/LambdaCapture.h
@@ -135,6 +135,16 @@
assert(isPackExpansion() && "No ellipsis location for a non-expansion");
return EllipsisLoc;
}
+
+ // [In]Equality operators -- primarily for ODR-compliance checking.
+
+ bool operator==(LambdaCapture const &RHS) const {
+return DeclAndBits == RHS.DeclAndBits;
+ }
+
+ bool operator!=(LambdaCapture const &RHS) const {
+return !operator==(RHS);
+ }
};
} // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50948: lambdas in modules: change storage for LambdaDefinitionData
elsteveogrande added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:1771 auto *Def = DD.Definition; DD = std::move(MergeDD); DD.Definition = Def; Hi @rsmith, thanks again for looking! This is the part that I was concerned about: that `DD` could be either a `DefinitionData` or `LambdaDefinitionData` reference; similar `MergeDD`. It didn't seem that move-assign was well-defined in the cases where they were mixed. If their being different is completely unexpected, though, this whole thing could perhaps be an assertion that they are both lambda, or both not lambda. And an `IsLambda` field on the CXXRecordDecl` (or `TTK_Lambda`). Alternatively, I could make the lambda data a pointer or `std::unique_ptr` inside `DefinitionData` (and maybe make the latter `final`), so as not to allocate much extra space in the non-lambda case. Depends on whether a new tag type is not too much of a breaking change. Also, whether it's known at this point that this record is a lambda (and `DD` is already of the correct type). Repository: rC Clang https://reviews.llvm.org/D50948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50949: lambdas in modules: handle lambdas in .pcm [de]serialization
elsteveogrande updated this revision to Diff 165948.
elsteveogrande added a comment.
Rebase atop updated dependency https://reviews.llvm.org/D50948. Small cleanups
to this diff.
Repository:
rC Clang
https://reviews.llvm.org/D50949
Files:
include/clang/AST/DeclCXX.h
include/clang/AST/LambdaCapture.h
lib/AST/DeclCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/merge-lambdas.cpp
Index: test/Modules/merge-lambdas.cpp
===
--- test/Modules/merge-lambdas.cpp
+++ test/Modules/merge-lambdas.cpp
@@ -49,3 +49,25 @@
#pragma clang module import A
void (*p)() = f();
+
+#pragma clang module build PR38531
+module PR38531 {}
+#pragma clang module contents
+#pragma clang module begin PR38531
+template
+struct S {};
+struct Fun {
+ template ())>
+ Fun(F) {}
+};
+template
+void foo(Fun=[]{}) {}
+struct T {
+ void func() {
+foo();
+ }
+};
+#pragma clang module end
+#pragma clang module endbuild
+#pragma clang module import PR38531
+S s;
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -6010,7 +6010,7 @@
void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
auto &Data = D->data();
- Record->push_back(Data.IsLambda);
+ Record->push_back(Data.hasLambdaData()); // lambda-specific data added below
Record->push_back(Data.UserDeclaredConstructor);
Record->push_back(Data.UserDeclaredSpecialMembers);
Record->push_back(Data.Aggregate);
@@ -6068,8 +6068,6 @@
if (ModulesDebugInfo)
Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
- // IsLambda bit is already saved.
-
Record->push_back(Data.NumBases);
if (Data.NumBases > 0)
AddCXXBaseSpecifiers(Data.bases());
@@ -6085,8 +6083,8 @@
AddDeclRef(D->getFirstFriend());
// Add lambda-specific data.
- if (Data.IsLambda) {
-auto &Lambda = D->getLambdaData();
+ if (Data.hasLambdaData()) {
+auto &Lambda = *Data.LambdaData;
Record->push_back(Lambda.Dependent);
Record->push_back(Lambda.IsGenericLambda);
Record->push_back(Lambda.CaptureDefault);
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1636,7 +1636,7 @@
void ASTDeclReader::ReadCXXDefinitionData(
struct CXXRecordDecl::DefinitionData &Data, const CXXRecordDecl *D) {
- // Note: the caller has deserialized the IsLambda bit already.
+ // Note: the caller has deserialized the bit for `isLambda()` already.
Data.UserDeclaredConstructor = Record.readInt();
Data.UserDeclaredSpecialMembers = Record.readInt();
Data.Aggregate = Record.readInt();
@@ -1703,10 +1703,10 @@
assert(Data.Definition && "Data.Definition should be already set!");
Data.FirstFriend = ReadDeclID();
- if (Data.IsLambda) {
+ if (Data.hasLambdaData()) {
using Capture = LambdaCapture;
-auto &Lambda = static_cast(Data);
+auto &Lambda = *Data.LambdaData;
Lambda.Dependent = Record.readInt();
Lambda.IsGenericLambda = Record.readInt();
Lambda.CaptureDefault = Record.readInt();
@@ -1761,7 +1761,6 @@
PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
-assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
// Don't change which declaration is the definition; that is required
@@ -1826,7 +1825,6 @@
MATCH_FIELD(ImplicitCopyAssignmentHasConstParam)
OR_FIELD(HasDeclaredCopyConstructorWithConstParam)
OR_FIELD(HasDeclaredCopyAssignmentWithConstParam)
- MATCH_FIELD(IsLambda)
#undef OR_FIELD
#undef MATCH_FIELD
@@ -1845,9 +1843,41 @@
// FIXME: Issue a diagnostic if FirstFriend doesn't match when we come to
// lazily load it.
- if (DD.IsLambda) {
-// FIXME: ODR-checking for merging lambdas (this happens, for instance,
-// when they occur within the body of a function template specialization).
+ assert (DD.hasLambdaData() == MergeDD.hasLambdaData() &&
+ "Merging definitions, one of which is a lambda, the other not?");
+ if (DD.hasLambdaData() && MergeDD.hasLambdaData()) {
+auto &LDD = *DD.LambdaData;
+auto &MergeLDD = *MergeDD.LambdaData;
+
+#define MATCH_LAM_FIELD(Field) \
+DetectedOdrViolation |= LDD.Field != MergeLDD.Field;
+MATCH_LAM_FIELD(Dependent)
+MATCH_LAM_FIELD(IsGenericLambda)
+MATCH_LAM_FIELD(CaptureDefault)
+MATCH_LAM_FIELD(NumCaptures)
+MATCH_LAM_FIELD(NumExplicitCaptures)
+MATCH_LAM_FIELD(ManglingNumber)
+#undef MATCH_LAM_FIELD
+
+auto *C1 = LDD.Captures;
[PATCH] D50948: lambdas in modules: change storage for LambdaDefinitionData
elsteveogrande updated this revision to Diff 165947.
elsteveogrande added a comment.
Rebase past dependency commit C341499, fix a conflict
Repository:
rC Clang
https://reviews.llvm.org/D50948
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -6010,7 +6010,7 @@
void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
auto &Data = D->data();
- Record->push_back(Data.IsLambda);
+ Record->push_back(Data.hasLambdaData()); // lambda-specific data added below
Record->push_back(Data.UserDeclaredConstructor);
Record->push_back(Data.UserDeclaredSpecialMembers);
Record->push_back(Data.Aggregate);
@@ -6068,8 +6068,6 @@
if (ModulesDebugInfo)
Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
- // IsLambda bit is already saved.
-
Record->push_back(Data.NumBases);
if (Data.NumBases > 0)
AddCXXBaseSpecifiers(Data.bases());
@@ -6085,8 +6083,8 @@
AddDeclRef(D->getFirstFriend());
// Add lambda-specific data.
- if (Data.IsLambda) {
-auto &Lambda = D->getLambdaData();
+ if (Data.hasLambdaData()) {
+auto &Lambda = *Data.LambdaData;
Record->push_back(Lambda.Dependent);
Record->push_back(Lambda.IsGenericLambda);
Record->push_back(Lambda.CaptureDefault);
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1636,7 +1636,7 @@
void ASTDeclReader::ReadCXXDefinitionData(
struct CXXRecordDecl::DefinitionData &Data, const CXXRecordDecl *D) {
- // Note: the caller has deserialized the IsLambda bit already.
+ // Note: the caller has deserialized the bit for `isLambda()` already.
Data.UserDeclaredConstructor = Record.readInt();
Data.UserDeclaredSpecialMembers = Record.readInt();
Data.Aggregate = Record.readInt();
@@ -1703,10 +1703,10 @@
assert(Data.Definition && "Data.Definition should be already set!");
Data.FirstFriend = ReadDeclID();
- if (Data.IsLambda) {
+ if (Data.hasLambdaData()) {
using Capture = LambdaCapture;
-auto &Lambda = static_cast(Data);
+auto &Lambda = *Data.LambdaData;
Lambda.Dependent = Record.readInt();
Lambda.IsGenericLambda = Record.readInt();
Lambda.CaptureDefault = Record.readInt();
@@ -1761,7 +1761,8 @@
PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
-assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
+assert(!DD.hasLambdaData() && !MergeDD.hasLambdaData() &&
+ "faked up lambda definition?");
PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
// Don't change which declaration is the definition; that is required
@@ -1826,7 +1827,6 @@
MATCH_FIELD(ImplicitCopyAssignmentHasConstParam)
OR_FIELD(HasDeclaredCopyConstructorWithConstParam)
OR_FIELD(HasDeclaredCopyAssignmentWithConstParam)
- MATCH_FIELD(IsLambda)
#undef OR_FIELD
#undef MATCH_FIELD
@@ -1845,7 +1845,7 @@
// FIXME: Issue a diagnostic if FirstFriend doesn't match when we come to
// lazily load it.
- if (DD.IsLambda) {
+ if (DD.hasLambdaData()) {
// FIXME: ODR-checking for merging lambdas (this happens, for instance,
// when they occur within the body of a function template specialization).
}
@@ -1860,17 +1860,17 @@
}
void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update) {
- struct CXXRecordDecl::DefinitionData *DD;
ASTContext &C = Reader.getContext();
+ struct CXXRecordDecl::DefinitionData *DD =
+ new (C) struct CXXRecordDecl::DefinitionData(D);
+
// Determine whether this is a lambda closure type, so that we can
- // allocate the appropriate DefinitionData structure.
- bool IsLambda = Record.readInt();
- if (IsLambda)
-DD = new (C) CXXRecordDecl::LambdaDefinitionData(D, nullptr, false, false,
- LCD_None);
- else
-DD = new (C) struct CXXRecordDecl::DefinitionData(D);
+ // set the LambdaDefinitionData (into optional \c LambdaData field) as well.
+ bool isLambda = Record.readInt();
+ if (isLambda) {
+DD->SetLambdaDefinitionData(nullptr, false, false, LCD_None);
+ }
CXXRecordDecl *Canon = D->getCanonicalDecl();
// Set decl definition data before reading it, so that during deserialization
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -102,7 +102,7 @@
ImplicitCopyConstructorCanHaveConstParamForNonVBase(true),
[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec
elsteveogrande abandoned this revision. elsteveogrande added a comment. that's great news, thanks! Repository: rC Clang https://reviews.llvm.org/D51608 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50947: lambdas in modules: make PendingFakeDefinitionData a set
elsteveogrande abandoned this revision. elsteveogrande added a comment. I'll drop this one, since this is not the logic change we want. Repository: rC Clang https://reviews.llvm.org/D50947 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50947: lambdas in modules: make PendingFakeDefinitionData a set
elsteveogrande abandoned this revision.
elsteveogrande added inline comments.
Comment at: lib/Serialization/ASTReaderDecl.cpp:1760-1765
- if (PFDI != Reader.PendingFakeDefinitionData.end() &&
- PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
+ if (PFDI != Reader.PendingFakeDefinitionData.end()) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
+
+Reader.PendingFakeDefinitionData.erase(PFDI);
+
+// FIXME: handle serialized lambdas
assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
-PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
(2) the `find`, and the flip to `FakeLoaded`
Comment at: lib/Serialization/ASTReaderDecl.cpp:3080-3081
// Track that we did this horrible thing so that we can fix it later.
- Reader.PendingFakeDefinitionData.insert(
- std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
}
(1) the insert
Comment at: lib/Serialization/ASTReaderDecl.cpp:4246
OldDD && (OldDD->Definition != RD ||
!Reader.PendingFakeDefinitionData.count(OldDD));
RD->setParamDestroyedInCallee(Record.readInt());
elsteveogrande wrote:
> rsmith wrote:
> > This does not appear to be NFC: the `FakeLoaded` vs absent-from-map
> > distinction matters here. I think the idea here is that we still need to
> > load the lexical contents of the declaration of the class that we pick to
> > be the definition even if we've already found and merged another definition
> > into it.
> >
> > That said, if this //is// necessary, we should have some test coverage for
> > it, and based on this change not breaking any of our tests, we evidently
> > don't. :( I'll try this change out against our codebase and see if I can
> > find a testcase.
> Thanks for checking this! Yup I missed this; count would be different and
> therefore this changes logic, which surprisingly didn't break things as far
> as I could see. I'll change this back to `Fake` vs. `FakeLoaded`.
Hmm. In the lambda case, the `FakeLoaded` remains in the table, and we get
"faked up a class definition but never saw the real one":
assert(PendingFakeDefinitionData.empty() &&
"faked up a class definition but never saw the real one");
Failing assert here:
[lib/Serialization/ASTReader.cpp](https://github.com/llvm-mirror/clang/blob/master/lib/Serialization/ASTReader.cpp#L9347)
So I'm digging into why `PendingFakeDefinitionData` isn't emptying out.
Recapping the logic in this class (to make sure I know what's going on here /
to help explain it to myself)
1. a `Fake` insert happens around 3081, after creating a fake/empty
`DefinitionData` and setting on a new `CXXRecordDecl` (and its canonical decl
if different)
2. this table is checked in `MergeDefinitionData` from 1765; if there's a
`Fake` entry, flip it to `FakeLoaded`
3. the only removal that I could find is when a decl is updated with
`UPD_CXX_INSTANTIATED_CLASS_DEFINITION` around line 4256, only if there wasn't
an update and there's a lexical DC. (In any event the
`UPD_CXX_INSTANTIATED_...` wouldn't fire in my case)
Comment at: lib/Serialization/ASTReaderDecl.cpp:4255
Record.readLexicalDeclContextStorage(LexicalOffset, RD);
Reader.PendingFakeDefinitionData.erase(OldDD);
}
(3) ... and finally the erase
Repository:
rC Clang
https://reviews.llvm.org/D50947
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50947: lambdas in modules: make PendingFakeDefinitionData a set
elsteveogrande added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:1766 + +// FIXME: handle serialized lambdas assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?"); rsmith wrote: > Did you mean to add this FIXME? Yup: to indicate this is assert about `faked up lambda definition` will be addressed in an upcoming diff, i.e., fixing the lambda issues :) See D50948, D50949 Comment at: lib/Serialization/ASTReaderDecl.cpp:4246 OldDD && (OldDD->Definition != RD || !Reader.PendingFakeDefinitionData.count(OldDD)); RD->setParamDestroyedInCallee(Record.readInt()); rsmith wrote: > This does not appear to be NFC: the `FakeLoaded` vs absent-from-map > distinction matters here. I think the idea here is that we still need to load > the lexical contents of the declaration of the class that we pick to be the > definition even if we've already found and merged another definition into it. > > That said, if this //is// necessary, we should have some test coverage for > it, and based on this change not breaking any of our tests, we evidently > don't. :( I'll try this change out against our codebase and see if I can find > a testcase. Thanks for checking this! Yup I missed this; count would be different and therefore this changes logic, which surprisingly didn't break things as far as I could see. I'll change this back to `Fake` vs. `FakeLoaded`. Repository: rC Clang https://reviews.llvm.org/D50947 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec
elsteveogrande added a comment. Hi @rsmith ! Thanks for taking a look at this. I'd much prefer to fix the underlying problem and not swat at these symptoms, so thanks for the analysis of what's actually going on here. :) Let me take a stab at fixing the real problem in the manner you describe. Repository: rC Clang https://reviews.llvm.org/D51608 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50942: SemaExceptionSpec: ensure old/new specs are both parsed and evaluated when comparing
elsteveogrande abandoned this revision. elsteveogrande added a comment. Abandoning in favor of https://reviews.llvm.org/D51608 which works better. Repository: rC Clang https://reviews.llvm.org/D50942 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec
elsteveogrande created this revision.
elsteveogrande added reviewers: rsmith, dblaikie.
Herald added a subscriber: cfe-commits.
(PR38627)
After deserializing, the `PendingExceptionSpecUpdates` table is iterated over
to apply exception specs to functions. There may be `EST_None`-type in this
table. However if there is a method in the redecl chain already having some
other non-`EST_None` type, this should not be clobbered with none.
If we see `EST_None` avoid setting the exception spec. (This is the default
anyway, so it's safe to skip in the legit case.)
There may be *better* ways to actually fix this, rather than a simple 1-line
defensive check like I did here. It would be more complicated and quite
outside my area of experience though.
Test Plan: Passes `ninja check-clang`, including a new test case.
Andrew Gallagher provided the test case (distilling it from a much more complex
instance in our code base).
Repository:
rC Clang
https://reviews.llvm.org/D51608
Files:
lib/Serialization/ASTReader.cpp
test/Modules/Inputs/PR38627/a.h
test/Modules/Inputs/PR38627/module.modulemap
test/Modules/pr38627.cpp
Index: test/Modules/pr38627.cpp
===
--- /dev/null
+++ test/Modules/pr38627.cpp
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: %clang -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/PR38627 -c %s
+// expected-no-diagnostics
+
+// PR38627: Exception specifications sometimes go missing / are incorrect
+// after deserializing from a module.
+// https://bugs.llvm.org/show_bug.cgi?id=38627
+
+namespace test_with_unserialized_decls {
+
+// X/Y/Z are the same as A/B/C from: Inputs/PR38627/a.h
+class X {
+ public:
+ virtual ~X() = default;
+};
+class Y {
+ public:
+ virtual ~Y() {
+z.func();
+ }
+ struct Z {
+void func() {}
+friend Y::~Y();
+ };
+ Z z;
+};
+
+// ~Y's exception spec should be calculated to be noexcept.
+static_assert(noexcept(Y().~Y()));
+
+} // end test_with_unserialized_decls
+
+// Same definitions of A and B (but without the namespace) in this module
+#include "a.h"
+
+namespace test_with_deserialized_decls {
+
+static_assert(noexcept(B().~B()), "PR38627");
+
+class D : public A, public B {
+ public:
+ // Error should be gone with bug fix:
+ // "exception specification of overriding function is more lax than base version"
+ virtual ~D() override = default;
+};
+
+} // end test_with_deserialized_decls
Index: test/Modules/Inputs/PR38627/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR38627/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/PR38627/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR38627/a.h
@@ -0,0 +1,17 @@
+#pragma once
+
+class A {
+ public:
+ virtual ~A() = default;
+};
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+ struct C {
+void func() {}
+friend B::~B();
+ };
+ C c;
+};
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -11551,10 +11551,17 @@
ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
auto *FPT = Update.second->getType()->castAs();
auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
-if (auto *Listener = getContext().getASTMutationListener())
- Listener->ResolvedExceptionSpec(cast(Update.second));
-for (auto *Redecl : Update.second->redecls())
- getContext().adjustExceptionSpec(cast(Redecl), ESI);
+// Ignore updates with EST_None sorts of exception specifications.
+// That is the default anyway; also, in the event a FunctionDecl does
+// have a non-None type, we'll avoid clobbering it.
+if (ESI.Type != ExceptionSpecificationType::EST_None) {
+ if (auto *Listener = getContext().getASTMutationListener())
+Listener->ResolvedExceptionSpec(cast(Update.second));
+ for (auto *Redecl : Update.second->redecls()) {
+// FIXME: If the exception spec is already set, ensure it matches.
+getContext().adjustExceptionSpec(cast(Redecl), ESI);
+ }
+}
}
auto DTUpdates = std::move(PendingDeducedTypeUpdates);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51580: (WIP) fix spurious exception spec error, PR38627
elsteveogrande created this revision.
Herald added a subscriber: cfe-commits.
Repository:
rC Clang
https://reviews.llvm.org/D51580
Files:
test/Modules/Inputs/lax-base-except/a.h
test/Modules/Inputs/lax-base-except/module.modulemap
test/Modules/lax-base-except.cpp
Index: test/Modules/lax-base-except.cpp
===
--- /dev/null
+++ test/Modules/lax-base-except.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang -c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I
%S/Inputs/lax-base-except %s -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
+
+class D : public A, public B {
+ public:
+ virtual ~D() override = default;
+};
Index: test/Modules/Inputs/lax-base-except/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/lax-base-except/a.h
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/a.h
@@ -0,0 +1,18 @@
+class A {
+ public:
+ virtual ~A() = default;
+};
+
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+
+ struct C{
+void func() {}
+friend B::~B();
+ };
+
+ C c;
+};
Index: test/Modules/lax-base-except.cpp
===
--- /dev/null
+++ test/Modules/lax-base-except.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang -c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/lax-base-except %s -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
+
+class D : public A, public B {
+ public:
+ virtual ~D() override = default;
+};
Index: test/Modules/Inputs/lax-base-except/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/lax-base-except/a.h
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/a.h
@@ -0,0 +1,18 @@
+class A {
+ public:
+ virtual ~A() = default;
+};
+
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+
+ struct C{
+void func() {}
+friend B::~B();
+ };
+
+ C c;
+};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50942: SemaExceptionSpec: ensure old/new specs are both parsed and evaluated when comparing
elsteveogrande planned changes to this revision. elsteveogrande marked an inline comment as done. elsteveogrande added a comment. This broke some tests. It fixes the test case added here under `test/Modules` but causes errors in `test/CodeGenCXX` and other dirs. Repository: rC Clang https://reviews.llvm.org/D50942 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50949: Handle lambdas in .pcm [de]serialization
elsteveogrande created this revision.
elsteveogrande added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.
- Drop a couple of asserts preventing this (in the merge function)
- Merge all fields as usual
- Ensure select fields match, for ODR checking
- add previously-failing test to merge-lambdas.cpp
Test case by Andrew Gallagher
Test Plan: Andrew's repro from https://bugs.llvm.org/show_bug.cgi?id=38531
broke before this change, works after.
Repository:
rC Clang
https://reviews.llvm.org/D50949
Files:
include/clang/AST/LambdaCapture.h
lib/Serialization/ASTReaderDecl.cpp
test/Modules/merge-lambdas.cpp
Index: test/Modules/merge-lambdas.cpp
===
--- test/Modules/merge-lambdas.cpp
+++ test/Modules/merge-lambdas.cpp
@@ -49,3 +49,25 @@
#pragma clang module import A
void (*p)() = f();
+
+#pragma clang module build PR38531
+module PR38531 {}
+#pragma clang module contents
+#pragma clang module begin PR38531
+template
+struct S {};
+struct Fun {
+ template ())>
+ Fun(F) {}
+};
+template
+void foo(Fun=[]{}) {}
+struct T {
+ void func() {
+foo();
+ }
+};
+#pragma clang module end
+#pragma clang module endbuild
+#pragma clang module import PR38531
+S s;
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1763,12 +1763,6 @@
Reader.PendingFakeDefinitionData.erase(PFDI);
-// FIXME: handle serialized lambdas
-assert(!(DD.hasLambdaData() || MergeDD.hasLambdaData()) &&
- "faked up lambda definition?");
-
-assert(DD.hasLambdaData() == MergeDD.hasLambdaData());
-
// Don't change which declaration is the definition; that is required
// to be invariant once we select it.
auto *Def = DD.Definition;
@@ -1849,9 +1843,44 @@
// FIXME: Issue a diagnostic if FirstFriend doesn't match when we come to
// lazily load it.
- if (DD.hasLambdaData()) {
-// FIXME: ODR-checking for merging lambdas (this happens, for instance,
-// when they occur within the body of a function template specialization).
+ assert (DD.hasLambdaData() == MergeDD.hasLambdaData() &&
+ "Merging definitions, one of which is a lambda, the other not?");
+ if (DD.hasLambdaData() && MergeDD.hasLambdaData()) {
+auto &LDD = *DD.LambdaData;
+auto &MergeLDD = *MergeDD.LambdaData;
+
+#define MATCH_LAM_FIELD(Field) \
+DetectedOdrViolation |= LDD.Field != MergeLDD.Field;
+MATCH_LAM_FIELD(Dependent)
+MATCH_LAM_FIELD(IsGenericLambda)
+MATCH_LAM_FIELD(CaptureDefault)
+MATCH_LAM_FIELD(NumCaptures)
+MATCH_LAM_FIELD(NumExplicitCaptures)
+MATCH_LAM_FIELD(ManglingNumber)
+#undef MATCH_LAM_FIELD
+
+auto *C1 = LDD.Captures;
+auto *C2 = MergeLDD.Captures;
+for (int I = 0; !DetectedOdrViolation && I < LDD.NumCaptures; ++I) {
+ if (C1[I] != C2[I]) {
+DetectedOdrViolation = true;
+ }
+}
+
+if (LDD.MethodTyInfo || MergeLDD.MethodTyInfo) {
+ if (!LDD.MethodTyInfo ||
+ !MergeLDD.MethodTyInfo ||
+ (LDD.MethodTyInfo->getType().getTypePtrOrNull() !=
+ MergeLDD.MethodTyInfo->getType().getTypePtrOrNull())) {
+DetectedOdrViolation = true;
+llvm::errs() << "@@@ T1 " << LDD.MethodTyInfo->getType().getTypePtrOrNull()
+ << " T2 " << MergeLDD.MethodTyInfo->getType().getTypePtrOrNull()
+ << '\n';
+ }
+}
+
+// FIXME: Issue a diagnostic if ContextDecl doesn't match when we come to
+// lazily load it.
}
if (D->getODRHash() != MergeDD.ODRHash) {
Index: include/clang/AST/LambdaCapture.h
===
--- include/clang/AST/LambdaCapture.h
+++ include/clang/AST/LambdaCapture.h
@@ -135,6 +135,16 @@
assert(isPackExpansion() && "No ellipsis location for a non-expansion");
return EllipsisLoc;
}
+
+ // [In]Equality operators -- primarily for ODR-compliance checking.
+
+ bool operator==(LambdaCapture const &RHS) const {
+return DeclAndBits == RHS.DeclAndBits;
+ }
+
+ bool operator!=(LambdaCapture const &RHS) const {
+return !operator==(RHS);
+ }
};
} // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50948: Change how LambdaDefinitionData is stored
elsteveogrande created this revision.
Herald added a subscriber: cfe-commits.
Previously `LambdaDefinitionData` extended `DefinitionData` and either might
have been used interchangably in a `CXXRecordDecl`'s `DefinitionData` field.
However there are cases where `LambdaDefinitionData` ("LDD") aren't copied or
moved completely, into this `DefinitionData` ("DD") field, as it has additional
members of its own.
This patch:
- makes LDD a separate structure, no longer using DD as the base class, but
otherwise kept the same so as to encapsulate lambda stuff properly
- stores lambda data within an `Optional` called `LambdaData`
- replaces `IsLambda` bit with `bool isLambda() const`, true IFF lambda data
are present in that optional field
- as the formerly-subclass's LDD constructor set some members on DD, this is
now done in a new method `SetLambdaData`; this sets those fields to values
sensible for lambdas, and emplaces lambda data in the optional field
This solves one case where lambda data were not handled quite properly.
Note that this doesn't solve all lambda-handling in `pcm`'s (clang modules) but
is more of a refactoring pre-step.
Test Plan: `ninja check-clang-modules`
Repository:
rC Clang
https://reviews.llvm.org/D50948
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -6017,7 +6017,7 @@
void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
auto &Data = D->data();
- Record->push_back(Data.IsLambda);
+ Record->push_back(Data.hasLambdaData()); // lambda-specific data added below
Record->push_back(Data.UserDeclaredConstructor);
Record->push_back(Data.UserDeclaredSpecialMembers);
Record->push_back(Data.Aggregate);
@@ -6075,8 +6075,6 @@
if (ModulesDebugInfo)
Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
- // IsLambda bit is already saved.
-
Record->push_back(Data.NumBases);
if (Data.NumBases > 0)
AddCXXBaseSpecifiers(Data.bases());
@@ -6092,8 +6090,8 @@
AddDeclRef(D->getFirstFriend());
// Add lambda-specific data.
- if (Data.IsLambda) {
-auto &Lambda = D->getLambdaData();
+ if (Data.hasLambdaData()) {
+auto &Lambda = *Data.LambdaData;
Record->push_back(Lambda.Dependent);
Record->push_back(Lambda.IsGenericLambda);
Record->push_back(Lambda.CaptureDefault);
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1636,7 +1636,7 @@
void ASTDeclReader::ReadCXXDefinitionData(
struct CXXRecordDecl::DefinitionData &Data, const CXXRecordDecl *D) {
- // Note: the caller has deserialized the IsLambda bit already.
+ // Note: the caller has deserialized the bit for `isLambda()` already.
Data.UserDeclaredConstructor = Record.readInt();
Data.UserDeclaredSpecialMembers = Record.readInt();
Data.Aggregate = Record.readInt();
@@ -1703,10 +1703,10 @@
assert(Data.Definition && "Data.Definition should be already set!");
Data.FirstFriend = ReadDeclID();
- if (Data.IsLambda) {
+ if (Data.hasLambdaData()) {
using Capture = LambdaCapture;
-auto &Lambda = static_cast(Data);
+auto &Lambda = *Data.LambdaData;
Lambda.Dependent = Record.readInt();
Lambda.IsGenericLambda = Record.readInt();
Lambda.CaptureDefault = Record.readInt();
@@ -1764,7 +1764,10 @@
Reader.PendingFakeDefinitionData.erase(PFDI);
// FIXME: handle serialized lambdas
-assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
+assert(!(DD.hasLambdaData() || MergeDD.hasLambdaData()) &&
+ "faked up lambda definition?");
+
+assert(DD.hasLambdaData() == MergeDD.hasLambdaData());
// Don't change which declaration is the definition; that is required
// to be invariant once we select it.
@@ -1828,7 +1831,6 @@
MATCH_FIELD(ImplicitCopyAssignmentHasConstParam)
OR_FIELD(HasDeclaredCopyConstructorWithConstParam)
OR_FIELD(HasDeclaredCopyAssignmentWithConstParam)
- MATCH_FIELD(IsLambda)
#undef OR_FIELD
#undef MATCH_FIELD
@@ -1847,7 +1849,7 @@
// FIXME: Issue a diagnostic if FirstFriend doesn't match when we come to
// lazily load it.
- if (DD.IsLambda) {
+ if (DD.hasLambdaData()) {
// FIXME: ODR-checking for merging lambdas (this happens, for instance,
// when they occur within the body of a function template specialization).
}
@@ -1862,17 +1864,17 @@
}
void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update) {
- struct CXXRecordDecl::DefinitionData *DD;
ASTContext &C = Reader.getContext();
+ struct CXXRecordDecl::DefinitionData *DD =
+ new (C) struct CXXRecordDecl::DefinitionData(D)
[PATCH] D50947: make PendingFakeDefinitionData a set, not map of decl data -> enum
elsteveogrande created this revision.
Herald added a subscriber: cfe-commits.
Of the three enums, only two are used (Fake and FakeLoaded), and even when
switching the mapped value to FakeLoaded, it's leading to an unexpected state
anyway (i.e. there's no check for only Fake's in the map). Discard the enum,
make this a simple set to test for membership / emptiness.
Test Plan: `ninja check-clang-modules`
Repository:
rC Clang
https://reviews.llvm.org/D50947
Files:
include/clang/Serialization/ASTReader.h
lib/Serialization/ASTReaderDecl.cpp
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1757,12 +1757,14 @@
}
auto PFDI = Reader.PendingFakeDefinitionData.find(&DD);
- if (PFDI != Reader.PendingFakeDefinitionData.end() &&
- PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
+ if (PFDI != Reader.PendingFakeDefinitionData.end()) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
+
+Reader.PendingFakeDefinitionData.erase(PFDI);
+
+// FIXME: handle serialized lambdas
assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
-PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
// Don't change which declaration is the definition; that is required
// to be invariant once we select it.
@@ -3077,8 +3079,7 @@
RD->getCanonicalDecl()->DefinitionData = DD;
// Track that we did this horrible thing so that we can fix it later.
- Reader.PendingFakeDefinitionData.insert(
- std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
+ Reader.PendingFakeDefinitionData.insert(DD);
}
return DD->Definition;
Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -527,11 +527,9 @@
/// to apply once we finish processing an import.
llvm::SmallVector PendingUpdateRecords;
- enum class PendingFakeDefinitionKind { NotFake, Fake, FakeLoaded };
-
/// The DefinitionData pointers that we faked up for class definitions
/// that we needed but hadn't loaded yet.
- llvm::DenseMap PendingFakeDefinitionData;
+ llvm::DenseSet PendingFakeDefinitionData;
/// Exception specification updates that have been loaded but not yet
/// propagated across the relevant redeclaration chain. The map key is the
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1757,12 +1757,14 @@
}
auto PFDI = Reader.PendingFakeDefinitionData.find(&DD);
- if (PFDI != Reader.PendingFakeDefinitionData.end() &&
- PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
+ if (PFDI != Reader.PendingFakeDefinitionData.end()) {
// We faked up this definition data because we found a class for which we'd
// not yet loaded the definition. Replace it with the real thing now.
+
+Reader.PendingFakeDefinitionData.erase(PFDI);
+
+// FIXME: handle serialized lambdas
assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
-PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
// Don't change which declaration is the definition; that is required
// to be invariant once we select it.
@@ -3077,8 +3079,7 @@
RD->getCanonicalDecl()->DefinitionData = DD;
// Track that we did this horrible thing so that we can fix it later.
- Reader.PendingFakeDefinitionData.insert(
- std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
+ Reader.PendingFakeDefinitionData.insert(DD);
}
return DD->Definition;
Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -527,11 +527,9 @@
/// to apply once we finish processing an import.
llvm::SmallVector PendingUpdateRecords;
- enum class PendingFakeDefinitionKind { NotFake, Fake, FakeLoaded };
-
/// The DefinitionData pointers that we faked up for class definitions
/// that we needed but hadn't loaded yet.
- llvm::DenseMap PendingFakeDefinitionData;
+ llvm::DenseSet PendingFakeDefinitionData;
/// Exception specification updates that have been loaded but not yet
/// propagated across the relevant redeclaration chain. The map key is the
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50942: SemaExceptionSpec: ensure old/new specs are both parsed and evaluated when comparing
elsteveogrande updated this revision to Diff 161399.
elsteveogrande marked an inline comment as done.
elsteveogrande added a comment.
fix dopey copy-paste error. Tested again with `ninja check-clang-modules`
Repository:
rC Clang
https://reviews.llvm.org/D50942
Files:
lib/Sema/SemaExceptionSpec.cpp
test/Modules/Inputs/lax-base-except/a.h
test/Modules/Inputs/lax-base-except/module.modulemap
test/Modules/lax-base-except.cpp
Index: test/Modules/lax-base-except.cpp
===
--- /dev/null
+++ test/Modules/lax-base-except.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang -c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/lax-base-except %s -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
+
+class D : public A, public B {
+ public:
+ virtual ~D() override = default;
+};
Index: test/Modules/Inputs/lax-base-except/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/lax-base-except/a.h
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/a.h
@@ -0,0 +1,19 @@
+class A {
+ public:
+ virtual ~A() = default;
+};
+
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+
+ struct C{
+void func() {}
+friend B::~B();
+ };
+
+ C c;
+};
+
Index: lib/Sema/SemaExceptionSpec.cpp
===
--- lib/Sema/SemaExceptionSpec.cpp
+++ lib/Sema/SemaExceptionSpec.cpp
@@ -899,11 +899,28 @@
bool Sema::CheckOverridingFunctionExceptionSpec(const CXXMethodDecl *New,
const CXXMethodDecl *Old) {
- // If the new exception specification hasn't been parsed yet, skip the check.
+ // If the new exception spec hasn't been determined yet, skip the check.
// We'll get called again once it's been parsed.
- if (New->getType()->castAs()->getExceptionSpecType() ==
- EST_Unparsed)
+ switch (New->getType()->castAs()->getExceptionSpecType()) {
+ case EST_Unparsed:
+ case EST_Unevaluated:
return false;
+ default:
+;
+ }
+
+ // If the old exception spec hasn't been determined yet, remember that
+ // we need to perform this check when we get to the end of the outermost
+ // lexically-surrounding class.
+ switch (Old->getType()->castAs()->getExceptionSpecType()) {
+ case EST_Unparsed:
+ case EST_Unevaluated:
+DelayedExceptionSpecChecks.push_back(std::make_pair(New, Old));
+return false;
+ default:
+;
+ }
+
if (getLangOpts().CPlusPlus11 && isa(New)) {
// Don't check uninstantiated template destructors at all. We can only
// synthesize correct specs after the template is instantiated.
@@ -916,17 +933,11 @@
return false;
}
}
- // If the old exception specification hasn't been parsed yet, remember that
- // we need to perform this check when we get to the end of the outermost
- // lexically-surrounding class.
- if (Old->getType()->castAs()->getExceptionSpecType() ==
- EST_Unparsed) {
-DelayedExceptionSpecChecks.push_back(std::make_pair(New, Old));
-return false;
- }
+
unsigned DiagID = diag::err_override_exception_spec;
if (getLangOpts().MicrosoftExt)
DiagID = diag::ext_override_exception_spec;
+
return CheckExceptionSpecSubset(PDiag(DiagID),
PDiag(diag::err_deep_exception_specs_differ),
PDiag(diag::note_overridden_virtual_function),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50942: SemaExceptionSpec: ensure old/new specs are both parsed and evaluated when comparing
elsteveogrande marked an inline comment as done.
elsteveogrande added inline comments.
Comment at: lib/Sema/SemaExceptionSpec.cpp:915
+ // lexically-surrounding class.
+ switch (New->getType()->castAs()->getExceptionSpecType())
{
+ case EST_Unparsed:
vitaut wrote:
> Did you mean `Old` here?
facepalm, haha. Thanks @vitaut ! :)
Repository:
rC Clang
https://reviews.llvm.org/D50942
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50943: Decl: allow "setInvalidDecl" for TagDecls
elsteveogrande created this revision.
elsteveogrande added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.
For `TagDecl`s, complete or incomplete, allow `setInvalidDecl` calls.
Other `assert`s are interfering but we'll deal with them separately.
https://bugs.llvm.org/show_bug.cgi?id=38531
Repository:
rC Clang
https://reviews.llvm.org/D50943
Files:
lib/AST/DeclBase.cpp
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -131,11 +131,12 @@
void Decl::setInvalidDecl(bool Invalid) {
InvalidDecl = Invalid;
- assert(!isa(this) || !cast(this)->isCompleteDefinition());
if (!Invalid) {
return;
}
+ assert(!isa(this) || !cast(this)->isCompleteDefinition());
+
if (!isa(this)) {
// Defensive maneuver for ill-formed code: we're likely not to make it to
// a point where we set the access specifier, so default it to "public"
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -131,11 +131,12 @@
void Decl::setInvalidDecl(bool Invalid) {
InvalidDecl = Invalid;
- assert(!isa(this) || !cast(this)->isCompleteDefinition());
if (!Invalid) {
return;
}
+ assert(!isa(this) || !cast(this)->isCompleteDefinition());
+
if (!isa(this)) {
// Defensive maneuver for ill-formed code: we're likely not to make it to
// a point where we set the access specifier, so default it to "public"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50942: SemaExceptionSpec: ensure old/new specs are both parsed and evaluated when comparing
elsteveogrande created this revision.
elsteveogrande added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.
Before checking that the overriding definition of a method is not more lax than
the overridden one, ensure the exception spec is parsed, and also evaluated.
Test case by Andrew Gallagher!
Repository:
rC Clang
https://reviews.llvm.org/D50942
Files:
lib/Sema/SemaExceptionSpec.cpp
test/Modules/Inputs/lax-base-except/a.h
test/Modules/Inputs/lax-base-except/module.modulemap
test/Modules/lax-base-except.cpp
Index: test/Modules/lax-base-except.cpp
===
--- /dev/null
+++ test/Modules/lax-base-except.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang -c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/lax-base-except %s -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
+
+class D : public A, public B {
+ public:
+ virtual ~D() override = default;
+};
Index: test/Modules/Inputs/lax-base-except/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/lax-base-except/a.h
===
--- /dev/null
+++ test/Modules/Inputs/lax-base-except/a.h
@@ -0,0 +1,19 @@
+class A {
+ public:
+ virtual ~A() = default;
+};
+
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+
+ struct C{
+void func() {}
+friend B::~B();
+ };
+
+ C c;
+};
+
Index: lib/Sema/SemaExceptionSpec.cpp
===
--- lib/Sema/SemaExceptionSpec.cpp
+++ lib/Sema/SemaExceptionSpec.cpp
@@ -899,11 +899,28 @@
bool Sema::CheckOverridingFunctionExceptionSpec(const CXXMethodDecl *New,
const CXXMethodDecl *Old) {
- // If the new exception specification hasn't been parsed yet, skip the check.
+ // If the new exception spec hasn't been determined yet, skip the check.
// We'll get called again once it's been parsed.
- if (New->getType()->castAs()->getExceptionSpecType() ==
- EST_Unparsed)
+ switch (New->getType()->castAs()->getExceptionSpecType()) {
+ case EST_Unparsed:
+ case EST_Unevaluated:
return false;
+ default:
+;
+ }
+
+ // If the old exception spec hasn't been determined yet, remember that
+ // we need to perform this check when we get to the end of the outermost
+ // lexically-surrounding class.
+ switch (New->getType()->castAs()->getExceptionSpecType()) {
+ case EST_Unparsed:
+ case EST_Unevaluated:
+DelayedExceptionSpecChecks.push_back(std::make_pair(New, Old));
+return false;
+ default:
+;
+ }
+
if (getLangOpts().CPlusPlus11 && isa(New)) {
// Don't check uninstantiated template destructors at all. We can only
// synthesize correct specs after the template is instantiated.
@@ -916,17 +933,11 @@
return false;
}
}
- // If the old exception specification hasn't been parsed yet, remember that
- // we need to perform this check when we get to the end of the outermost
- // lexically-surrounding class.
- if (Old->getType()->castAs()->getExceptionSpecType() ==
- EST_Unparsed) {
-DelayedExceptionSpecChecks.push_back(std::make_pair(New, Old));
-return false;
- }
+
unsigned DiagID = diag::err_override_exception_spec;
if (getLangOpts().MicrosoftExt)
DiagID = diag::ext_override_exception_spec;
+
return CheckExceptionSpecSubset(PDiag(DiagID),
PDiag(diag::err_deep_exception_specs_differ),
PDiag(diag::note_overridden_virtual_function),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande added a comment. hi @vsk + @arphaman , it's been a while -- I brought this old diff up to date, wondering if you could take a look again. Thanks! Repository: rC Clang https://reviews.llvm.org/D42043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande updated this revision to Diff 161370.
elsteveogrande added a comment.
Rebase + fix conflicts for very old diff. Works again.
`ninja check-clang` with MSAN-enabled build:
Before:
Failing Tests (2):
Clang :: CodeGen/signed_metadata.cpp
Clang :: Index/comment-to-html-xml-conversion.cpp
Expected Passes: 12777
Expected Failures : 19
Unsupported Tests : 291
Unexpected Failures: 2
FAILED: tools/clang/test/CMakeFiles/check-clang
cd /data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test &&
/bin/python2.7 /data/users/steveo/llvm-ct/build/msan/clang/./bin/llvm-lit -sv
--param
clang_site_config=/data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test/lit.site.cfg
--param USE_Z3_SOLVER=0
/data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test
ninja: build stopped: subcommand failed.
After:
Failing Tests (1):
Clang :: CodeGen/signed_metadata.cpp
Expected Passes: 12778
Expected Failures : 19
Unsupported Tests : 291
Unexpected Failures: 1
FAILED: tools/clang/test/CMakeFiles/check-clang
cd /data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test &&
/bin/python2.7 /data/users/steveo/llvm-ct/build/msan/clang/./bin/llvm-lit -sv
--param
clang_site_config=/data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test/lit.site.cfg
--param USE_Z3_SOLVER=0
/data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test
ninja: build stopped: subcommand failed.
Repository:
rC Clang
https://reviews.llvm.org/D42043
Files:
include/clang-c/CXString.h
tools/c-index-test/c-index-test.c
tools/libclang/CXString.cpp
Index: tools/libclang/CXString.cpp
===
--- tools/libclang/CXString.cpp
+++ tools/libclang/CXString.cpp
@@ -15,99 +15,130 @@
#include "CXString.h"
#include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
#include "clang-c/Index.h"
#include "clang/Frontend/ASTUnit.h"
#include "llvm/Support/ErrorHandling.h"
+#include
using namespace clang;
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
- /// CXString contains a 'const char *' that it doesn't own.
- CXS_Unmanaged,
-
- /// CXString contains a 'const char *' that it allocated with malloc().
- CXS_Malloc,
-
- /// CXString contains a CXStringBuf that needs to be returned to the
- /// CXStringPool.
- CXS_StringBuf
-};
+static_assert(sizeof(CXString) <= 16, "");
namespace clang {
namespace cxstring {
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage. We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1. When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented. When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be deleted.
+ */
+struct RefCountedCharRange {
+ const char *Data;
+ const char *CString;
+ unsigned Size;
+ unsigned Count;
+};
+
//===--===//
// Basic generation of CXStrings.
//===--===//
-CXString createEmpty() {
+CXString createRef(const char *String) {
CXString Str;
- Str.data = "";
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) String;
+ if (String) {
+Str.Size = strlen(String);
+Str.IsNullTerminated = true;
+ } else {
+Str.Size = 0;
+Str.IsNullTerminated = false;
+ }
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
-CXString createNull() {
- CXString Str;
- Str.data = nullptr;
- Str.private_flags = CXS_Unmanaged;
- return Str;
+CXString createEmpty() {
+ return createRef("");
}
-CXString createRef(const char *String) {
- if (String && String[0] == '\0')
-return createEmpty();
+CXString createNull() {
+ return createRef(nullptr);
+}
- CXString Str;
- Str.data = String;
- Str.private_flags = CXS_Unmanaged;
- return Str;
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+ char *Spelling = (char *) malloc(Size + 1);
+ assert(Spelling);
+ if (CS) {
+memcpy(Spelling, CS, Size);
+ }
+ Spelling[Size] = 0;
+ return Spelling;
}
-CXString createDup(const char *String) {
- if (!String)
+ CXString createDup(const char *String) {
+ if (!String) {
return createNull();
-
- if (String[0] == '\0')
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = strdup(String);
- Str.private_flags = CXS_
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande abandoned this revision. elsteveogrande added a comment. Dropped in favor of other fix as mentioned above Repository: rC Clang https://reviews.llvm.org/D38320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande added inline comments.
Comment at: lib/Serialization/ASTReaderDecl.cpp:3448-3456
if (!inheritDefaultTemplateArgument(Context, FTTP, ToParam))
break;
} else if (auto *FNTTP = dyn_cast(FromParam)) {
if (!inheritDefaultTemplateArgument(Context, FNTTP, ToParam))
break;
} else {
if (!inheritDefaultTemplateArgument(
rsmith wrote:
> The idea is to reconstruct the 'inherited default template argument'
> information when we read a template declaration and find it's a redeclaration
> of another one, rather than serializing it and deserializing it. (In
> particular, if an imported template redeclares a non-imported template or a
> template imported from an unrelated module, it won't know that the prior
> declaration had a default argument because it doesn't know what the prior
> declaration was.)
>
> The problem with our implementation of that idea is these `break`s: we're
> (incorrectly) assuming that a template parameter cannot have a default
> argument if there's a later template parameter that does not have one.
>
> I removed the `break`s in r338438, and it fixed your testcase (which I
> committed alongside that change). Can you check to see if that also fixes the
> original problem from which this was reduced? Thanks!
Thanks @rsmith! It sounds like that ought to do it. We'll try again on our
end and let you know. Thanks for the quick fix!
Repository:
rC Clang
https://reviews.llvm.org/D38320
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande added a comment. hi @rsmith have a look and let me know if this change looks sensible :) Please make sure I have the right mental model for inheritance in the module case Repository: rC Clang https://reviews.llvm.org/D38320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande added a comment. cc @rsmith as this has to do with modules. Verified that (1) existing tests pass on trunk; (2) the new test included here fails when applied to trunk; (3) all tests (including new test) pass when this patch is applied. Repository: rC Clang https://reviews.llvm.org/D38320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande updated this revision to Diff 157682.
elsteveogrande added a comment.
Herald added a subscriber: chrib.
Rebase very old diff
Repository:
rC Clang
https://reviews.llvm.org/D38320
Files:
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/PCH/cxx-templates.cpp
test/PCH/cxx-templates.h
Index: test/PCH/cxx-templates.h
===
--- test/PCH/cxx-templates.h
+++ test/PCH/cxx-templates.h
@@ -361,3 +361,38 @@
namespace MemberSpecializationLocation {
template struct A { static int n; };
}
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+
+// case 1: defaulted `NonTypeTemplateParmDecl`, non-defaulted 2nd tpl param
+template
+int func1(T const &);
+
+template
+int func1(T const &) {
+ return foo;
+}
+
+// case 2: defaulted `TemplateTypeParmDecl`, non-defaulted 2nd tpl param
+template
+A func2(B const &);
+
+template
+A func2(B const &) {
+ return A(20.0f);
+}
+
+// case 3: defaulted `TemplateTemplateParmDecl`, non-defaulted 2nd tpl param
+template
+struct Container { T const &item; };
+
+template class C = Container, class D>
+C func3(D const &);
+
+template class C, class D>
+C func3(D const &d) {
+ return Container{d};
+}
+
+} // end namespace PR34728
Index: test/PCH/cxx-templates.cpp
===
--- test/PCH/cxx-templates.cpp
+++ test/PCH/cxx-templates.cpp
@@ -116,3 +116,19 @@
#endif
int k = A::n;
}
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+int test() {
+ // Verify with several TemplateParmDecl kinds, using PCH (incl. modules).
+ int z1 = func1(/*ignored*/2.718);
+ int z2 = func2(/*ignored*/3.142);
+ int tmp3 = 30;
+ Container c = func3(tmp3);
+ int z3 = c.item;
+
+ // Return value is meaningless. Just "use" all these values to avoid
+ // warning about unused vars / values.
+ return z1 + z2 + z3;
+}
+} // end namespace PR34728
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -1558,11 +1558,24 @@
Record.push_back(D->wasDeclaredWithTypename());
- bool OwnsDefaultArg = D->hasDefaultArgument() &&
-!D->defaultArgumentWasInherited();
- Record.push_back(OwnsDefaultArg);
- if (OwnsDefaultArg)
-Record.AddTypeSourceInfo(D->getDefaultArgumentInfo());
+ TypeSourceInfo *OwnedDefaultArg = nullptr;
+ Decl const *InheritsFromDecl = nullptr;
+ if (D->hasDefaultArgument()) {
+if (D->defaultArgumentWasInherited()) {
+ InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+} else {
+ OwnedDefaultArg = D->getDefaultArgumentInfo();
+}
+ }
+
+ Record.push_back(OwnedDefaultArg != nullptr);
+ if (OwnedDefaultArg) {
+Record.AddTypeSourceInfo(OwnedDefaultArg);
+ }
+ Record.push_back(InheritsFromDecl != nullptr);
+ if (InheritsFromDecl) {
+Record.AddDeclRef(InheritsFromDecl);
+ }
Code = serialization::DECL_TEMPLATE_TYPE_PARM;
}
@@ -1589,11 +1602,26 @@
} else {
// Rest of NonTypeTemplateParmDecl.
Record.push_back(D->isParameterPack());
-bool OwnsDefaultArg = D->hasDefaultArgument() &&
- !D->defaultArgumentWasInherited();
-Record.push_back(OwnsDefaultArg);
-if (OwnsDefaultArg)
- Record.AddStmt(D->getDefaultArgument());
+
+Expr *OwnedDefaultArg = nullptr;
+Decl const *InheritsFromDecl = nullptr;
+if (D->hasDefaultArgument()) {
+ if (D->defaultArgumentWasInherited()) {
+InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+ } else {
+OwnedDefaultArg = D->getDefaultArgument();
+ }
+}
+
+Record.push_back(OwnedDefaultArg != nullptr);
+if (OwnedDefaultArg) {
+ Record.AddStmt(OwnedDefaultArg);
+}
+Record.push_back(InheritsFromDecl != nullptr);
+if (InheritsFromDecl) {
+ Record.AddDeclRef(InheritsFromDecl);
+}
+
Code = serialization::DECL_NON_TYPE_TEMPLATE_PARM;
}
}
@@ -1618,11 +1646,26 @@
} else {
// Rest of TemplateTemplateParmDecl.
Record.push_back(D->isParameterPack());
-bool OwnsDefaultArg = D->hasDefaultArgument() &&
- !D->defaultArgumentWasInherited();
-Record.push_back(OwnsDefaultArg);
-if (OwnsDefaultArg)
- Record.AddTemplateArgumentLoc(D->getDefaultArgument());
+
+llvm::Optional OwnedDefaultArg;
+Decl const *InheritsFromDecl = nullptr;
+if (D->hasDefaultArgument()) {
+ if (D->defaultArgumentWasInherited()) {
+InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+ } else {
+OwnedDefaultArg = D->getDefaultArgument();
+ }
+}
+
+Record.push_back(OwnedDefaultArg.hasValue());
+if (OwnedDefaultArg.hasValue()) {
+ Record.AddTemplateArgumentLoc(*OwnedDefaultArg);
+}
+Record.push_b
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande added a comment. Hi @vsk + @arphaman : I did find such problems with either MSAN (with poison-in-dtor) and ASAN, so we should be able to discover these instances pretty easily :) Repository: rC Clang https://reviews.llvm.org/D42043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande updated this revision to Diff 130557.
elsteveogrande added a comment.
remove unneeded changes
Repository:
rC Clang
https://reviews.llvm.org/D42043
Files:
include/clang-c/CXString.h
tools/c-index-test/c-index-test.c
tools/libclang/CXString.cpp
Index: tools/libclang/CXString.cpp
===
--- tools/libclang/CXString.cpp
+++ tools/libclang/CXString.cpp
@@ -15,99 +15,130 @@
#include "CXString.h"
#include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
#include "clang-c/Index.h"
#include "clang/Frontend/ASTUnit.h"
#include "llvm/Support/ErrorHandling.h"
+#include
-using namespace clang;
-
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
- /// CXString contains a 'const char *' that it doesn't own.
- CXS_Unmanaged,
-
- /// CXString contains a 'const char *' that it allocated with malloc().
- CXS_Malloc,
+static_assert(sizeof(CXString) <= 16, "");
- /// CXString contains a CXStringBuf that needs to be returned to the
- /// CXStringPool.
- CXS_StringBuf
-};
+using namespace clang;
namespace clang {
namespace cxstring {
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage. We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1. When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented. When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be deleted.
+ */
+struct RefCountedCharRange {
+ const char *Data;
+ const char *CString;
+ unsigned Size;
+ unsigned Count;
+};
+
//===--===//
// Basic generation of CXStrings.
//===--===//
-CXString createEmpty() {
+CXString createRef(const char *String) {
CXString Str;
- Str.data = "";
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) String;
+ if (String) {
+Str.Size = strlen(String);
+Str.IsNullTerminated = true;
+ } else {
+Str.Size = 0;
+Str.IsNullTerminated = false;
+ }
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
-CXString createNull() {
- CXString Str;
- Str.data = nullptr;
- Str.private_flags = CXS_Unmanaged;
- return Str;
+CXString createEmpty() {
+ return createRef("");
}
-CXString createRef(const char *String) {
- if (String && String[0] == '\0')
-return createEmpty();
+CXString createNull() {
+ return createRef(nullptr);
+}
- CXString Str;
- Str.data = String;
- Str.private_flags = CXS_Unmanaged;
- return Str;
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+ char *Spelling = (char *) malloc(Size + 1);
+ assert(Spelling);
+ if (CS) {
+memcpy(Spelling, CS, Size);
+ }
+ Spelling[Size] = 0;
+ return Spelling;
}
CXString createDup(const char *String) {
- if (!String)
+ if (!String) {
return createNull();
-
- if (String[0] == '\0')
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = strdup(String);
- Str.private_flags = CXS_Malloc;
+ Str.Size = strlen(String);
+ Str.Contents = (const void *) copyCharRange(String, Str.Size);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = true;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(StringRef String) {
- // If the string is not nul-terminated, we have to make a copy.
-
- // FIXME: This is doing a one past end read, and should be removed! For memory
- // we don't manage, the API string can become unterminated at any time outside
- // our control.
-
- if (!String.empty() && String.data()[String.size()] != 0)
-return createDup(String);
-
- CXString Result;
- Result.data = String.data();
- Result.private_flags = (unsigned) CXS_Unmanaged;
- return Result;
+ assert (String.size() <= std::numeric_limits::max());
+ CXString Str;
+ Str.Size = unsigned(String.size());
+ Str.IsNullTerminated = false;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
+ auto *RC = new RefCountedCharRange {
+/* Data */ String.data(),
+/* CString */ nullptr,
+/* Size */ Str.Size,
+/* Count */ 1,
+ };
+ Str.Contents = (const void *) RC;
+ return Str;
}
CXString createDup(StringRef String) {
- CXString Result;
- char *Spelling = static_cast(malloc(String.size() + 1));
- memmove(Spelling, String.data(), String.size());
- Spelling[String.size()] = 0;
- Result.data = Spelling;
- Result.private_flags = (unsigned) CXS_Malloc;
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande updated this revision to Diff 130556.
elsteveogrande marked an inline comment as done.
elsteveogrande added a comment.
Fixes, but first, a question for reviewers:
Looking at the description of `clang_disposeString`:
/**
* \brief Free the given string.
*/
CINDEX_LINKAGE void clang_disposeString(CXString string);
Does it seem incorrect to acquire some `const char *` pointer into this string,
dispose the string, and then access the characters?
I've seen this happen a couple of times now. As I make changes to my code I
run into this pattern. (Since now this triggers a use-after-free abort.)
I wanted to ask because, though it seemed obvious to me that this is incorrect
usage, I'm now wondering if the expectation is that it's ok. Or maybe wasn't
technically ok, and we just "got away with it" before. :)
Anyway, assuming it's only correct to use the string before disposing it, then
the fixes this time around are:
- Fix an ASAN use-after-free in `c-index-test` mentioned above. Get the
`CXString`, pass it around, then dispose when we're done with it.
- Change `createEmpty` and `createNull` to delegate through `createRef`
- `createRef` tolerates `nullptr` correctly.
- I previously ran into ASAN aborts due to mixing malloc/free/operator
new/delete. I had changed everything to use operator new/delete. However from
C-land I can't `new char[length]`, only `malloc` (or `strdup` or whatever).
Change everything to malloc/free instead.
Repository:
rC Clang
https://reviews.llvm.org/D42043
Files:
.watchmanconfig
include/clang-c/CXString.h
tools/c-index-test/c-index-test.c
tools/libclang/CXString.cpp
tools/libclang/CXString.h
Index: tools/libclang/CXString.h
===
--- tools/libclang/CXString.h
+++ tools/libclang/CXString.h
@@ -106,4 +106,3 @@
}
#endif
-
Index: tools/libclang/CXString.cpp
===
--- tools/libclang/CXString.cpp
+++ tools/libclang/CXString.cpp
@@ -15,99 +15,130 @@
#include "CXString.h"
#include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
#include "clang-c/Index.h"
#include "clang/Frontend/ASTUnit.h"
#include "llvm/Support/ErrorHandling.h"
+#include
-using namespace clang;
-
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
- /// CXString contains a 'const char *' that it doesn't own.
- CXS_Unmanaged,
-
- /// CXString contains a 'const char *' that it allocated with malloc().
- CXS_Malloc,
+static_assert(sizeof(CXString) <= 16, "");
- /// CXString contains a CXStringBuf that needs to be returned to the
- /// CXStringPool.
- CXS_StringBuf
-};
+using namespace clang;
namespace clang {
namespace cxstring {
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage. We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1. When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented. When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be deleted.
+ */
+struct RefCountedCharRange {
+ const char *Data;
+ const char *CString;
+ unsigned Size;
+ unsigned Count;
+};
+
//===--===//
// Basic generation of CXStrings.
//===--===//
-CXString createEmpty() {
+CXString createRef(const char *String) {
CXString Str;
- Str.data = "";
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) String;
+ if (String) {
+Str.Size = strlen(String);
+Str.IsNullTerminated = true;
+ } else {
+Str.Size = 0;
+Str.IsNullTerminated = false;
+ }
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
-CXString createNull() {
- CXString Str;
- Str.data = nullptr;
- Str.private_flags = CXS_Unmanaged;
- return Str;
+CXString createEmpty() {
+ return createRef("");
}
-CXString createRef(const char *String) {
- if (String && String[0] == '\0')
-return createEmpty();
+CXString createNull() {
+ return createRef(nullptr);
+}
- CXString Str;
- Str.data = String;
- Str.private_flags = CXS_Unmanaged;
- return Str;
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+ char *Spelling = (char *) malloc(Size + 1);
+ assert(Spelling);
+ if (CS) {
+memcpy(Spelling, CS, Size);
+ }
+ Spelling[Size] = 0;
+ return Spelling;
}
CXString createDup(const char *String) {
-
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande marked 2 inline comments as done. elsteveogrande added inline comments. Comment at: tools/c-index-test/c-index-test.c:3268 - filename = clang_getFileName(file); - index_data->main_filename = clang_getCString(filename); - clang_disposeString(filename); + index_data->main_filename = clang_getFileName(file); elsteveogrande wrote: > vsk wrote: > > This looks like a separate bug fix. Is it possible to separate the > > main_filename changes from this patch? > Will do! Done: https://reviews.llvm.org/D42259 Repository: rC Clang https://reviews.llvm.org/D42043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42259: c-index-test: small fix to CXString handling and disposal
elsteveogrande created this revision.
elsteveogrande added reviewers: vsk, benlangmuir, akyrtzi.
Herald added a subscriber: cfe-commits.
(Separating some unrelated changes out of https://reviews.llvm.org/D42043)
Repository:
rC Clang
https://reviews.llvm.org/D42259
Files:
tools/c-index-test/c-index-test.c
Index: tools/c-index-test/c-index-test.c
===
--- tools/c-index-test/c-index-test.c
+++ tools/c-index-test/c-index-test.c
@@ -1128,6 +1128,13 @@
}
}
+static CXString createCXString(const char *CS) {
+ CXString Str;
+ Str.data = (const void *) CS;
+ Str.private_flags = 0;
+ return Str;
+}
+
/**/
/* Callbacks. */
/**/
@@ -3090,7 +3097,7 @@
int first_check_printed;
int fail_for_error;
int abort;
- const char *main_filename;
+ CXString main_filename;
ImportedASTFilesData *importedASTs;
IndexDataStringList *strings;
CXTranslationUnit TU;
@@ -3129,6 +3136,7 @@
const char *cname;
CXIdxClientFile file;
unsigned line, column;
+ const char *main_filename;
int isMainFile;
index_data = (IndexData *)client_data;
@@ -3143,7 +3151,8 @@
}
filename = clang_getFileName((CXFile)file);
cname = clang_getCString(filename);
- if (strcmp(cname, index_data->main_filename) == 0)
+ main_filename = clang_getCString(index_data->main_filename);
+ if (strcmp(cname, main_filename) == 0)
isMainFile = 1;
else
isMainFile = 0;
@@ -3345,14 +3354,11 @@
static CXIdxClientFile index_enteredMainFile(CXClientData client_data,
CXFile file, void *reserved) {
IndexData *index_data;
- CXString filename;
index_data = (IndexData *)client_data;
printCheck(index_data);
- filename = clang_getFileName(file);
- index_data->main_filename = clang_getCString(filename);
- clang_disposeString(filename);
+ index_data->main_filename = clang_getFileName(file);
printf("[enteredMainFile]: ");
printCXIndexFile((CXIdxClientFile)file);
@@ -3591,7 +3597,7 @@
index_data.first_check_printed = 0;
index_data.fail_for_error = 0;
index_data.abort = 0;
- index_data.main_filename = "";
+ index_data.main_filename = createCXString("");
index_data.importedASTs = importedASTs;
index_data.strings = NULL;
index_data.TU = NULL;
@@ -3607,6 +3613,7 @@
if (index_data.fail_for_error)
result = -1;
+ clang_disposeString(index_data.main_filename);
free_client_data(&index_data);
return result;
}
@@ -3628,7 +3635,7 @@
index_data.first_check_printed = 0;
index_data.fail_for_error = 0;
index_data.abort = 0;
- index_data.main_filename = "";
+ index_data.main_filename = createCXString("");
index_data.importedASTs = importedASTs;
index_data.strings = NULL;
index_data.TU = TU;
@@ -3641,6 +3648,7 @@
result = -1;
clang_disposeTranslationUnit(TU);
+ clang_disposeString(index_data.main_filename);
free_client_data(&index_data);
return result;
}
@@ -4133,9 +4141,7 @@
if (!isUSR(I[2]))
return not_usr("", I[2]);
else {
-CXString x;
-x.data = (void*) I[2];
-x.private_flags = 0;
+CXString x = createCXString(I[2]);
print_usr(clang_constructUSR_ObjCIvar(I[1], x));
}
@@ -4160,9 +4166,7 @@
if (!isUSR(I[3]))
return not_usr("", I[3]);
else {
-CXString x;
-x.data = (void*) I[3];
-x.private_flags = 0;
+CXString x = createCXString(I[3]);
print_usr(clang_constructUSR_ObjCMethod(I[1], atoi(I[2]), x));
}
I += 4;
@@ -4190,9 +4194,7 @@
if (!isUSR(I[2]))
return not_usr("", I[2]);
else {
-CXString x;
-x.data = (void*) I[2];
-x.private_flags = 0;
+CXString x = createCXString(I[2]);
print_usr(clang_constructUSR_ObjCProperty(I[1], x));
}
I += 3;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande added a comment.
Thanks very much for looking at this @vsk ! I actually found an ASAN bug in my
new code, mixing and matching `malloc/free` and `operator`s `new/delete`.
Comment at: tools/c-index-test/c-index-test.c:3268
- filename = clang_getFileName(file);
- index_data->main_filename = clang_getCString(filename);
- clang_disposeString(filename);
+ index_data->main_filename = clang_getFileName(file);
vsk wrote:
> This looks like a separate bug fix. Is it possible to separate the
> main_filename changes from this patch?
Will do!
Comment at: tools/libclang/CXString.cpp:59
CXString createEmpty() {
CXString Str;
+ Str.Contents = (const void *) "";
vsk wrote:
> Why shouldn't this be defined as createRef("")?
Hmm, good question. `createRef` below actually calls back to `createEmpty` and
`createNull` in the nonnull-but-empty and null cases.
I think I'll do this the other way around, and let `createRef` have the
responsibility of dealing with these fields and nulls and whatnot.
Comment at: tools/libclang/CXString.cpp:213
+ if (string.IsNullTerminated) {
+CString = (const char *) string.Contents;
+ } else {
vsk wrote:
> Basic question: If a non-owning CXString is null-terminated, what provides
> the guarantee that the string is in fact valid when getCString() is called?
> Is the user of the C API responsible for ensuring the lifetime of the string
> is valid?
I believe the API itself is the one building `CXString` instances, and the user
of the C API doesn't really create them, only use them. So the API has to
ensure the string stays "good" while there may be references to it.
(Which feels a little fragile. But I think that's the tradeoff being made.
You'll get either "fast" strings, or data guaranteed to be sane. I'd opt for
safer data but I don't know who's using this C API and am afraid to introduce a
serious perf regression. So it'll stay this way and I'll try my best to solve
*-SAN issues with these constraints :) )
Repository:
rC Clang
https://reviews.llvm.org/D42043
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande updated this revision to Diff 130225.
elsteveogrande added a comment.
Add a needed null-check on input string's data ptr.
Repository:
rC Clang
https://reviews.llvm.org/D42043
Files:
include/clang-c/CXString.h
tools/c-index-test/c-index-test.c
tools/libclang/CXString.cpp
tools/libclang/CXString.h
Index: tools/libclang/CXString.h
===
--- tools/libclang/CXString.h
+++ tools/libclang/CXString.h
@@ -106,4 +106,3 @@
}
#endif
-
Index: tools/libclang/CXString.cpp
===
--- tools/libclang/CXString.cpp
+++ tools/libclang/CXString.cpp
@@ -15,99 +15,141 @@
#include "CXString.h"
#include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
#include "clang-c/Index.h"
#include "clang/Frontend/ASTUnit.h"
#include "llvm/Support/ErrorHandling.h"
+#include
-using namespace clang;
-
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
- /// CXString contains a 'const char *' that it doesn't own.
- CXS_Unmanaged,
+static_assert(sizeof(CXString) <= 16, "");
- /// CXString contains a 'const char *' that it allocated with malloc().
- CXS_Malloc,
-
- /// CXString contains a CXStringBuf that needs to be returned to the
- /// CXStringPool.
- CXS_StringBuf
-};
+using namespace clang;
namespace clang {
namespace cxstring {
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage. We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1. When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented. When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be freed with \b free .
+ */
+struct RefCountedCharRange {
+ const char *Data;
+ const char *CString;
+ unsigned Size;
+ unsigned Count;
+};
+
//===--===//
// Basic generation of CXStrings.
//===--===//
CXString createEmpty() {
CXString Str;
- Str.data = "";
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) "";
+ Str.Size = 0;
+ Str.IsNullTerminated = true;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
CXString createNull() {
CXString Str;
- Str.data = nullptr;
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = nullptr;
+ Str.Size = 0;
+ Str.IsNullTerminated = false;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(const char *String) {
- if (String && String[0] == '\0')
+ if (!String) {
+return createNull();
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = String;
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) String;
+ Str.Size = strlen(String);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+ char *Spelling = static_cast(malloc(Size + 1));
+ memcpy(Spelling, CS, Size);
+ Spelling[Size] = 0;
+ return Spelling;
+}
+
CXString createDup(const char *String) {
- if (!String)
+ if (!String) {
return createNull();
-
- if (String[0] == '\0')
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = strdup(String);
- Str.private_flags = CXS_Malloc;
+ Str.Size = strlen(String);
+ Str.Contents = (const void *) copyCharRange(String, Str.Size);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = true;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(StringRef String) {
- // If the string is not nul-terminated, we have to make a copy.
-
- // FIXME: This is doing a one past end read, and should be removed! For memory
- // we don't manage, the API string can become unterminated at any time outside
- // our control.
-
- if (!String.empty() && String.data()[String.size()] != 0)
-return createDup(String);
-
- CXString Result;
- Result.data = String.data();
- Result.private_flags = (unsigned) CXS_Unmanaged;
- return Result;
+ assert (String.size() <= std::numeric_limits::max());
+ CXString Str;
+ Str.Size = unsigned(String.size());
+ Str.IsNullTerminated = false;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
+ auto *RC = new RefCountedCharRange {
+/* Data */ String.data(),
+/* CString */ nullptr,
+/* Size */ Str.Size,
+/* Count */ 1,
+ };
+ S
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande updated this revision to Diff 129959.
elsteveogrande added a comment.
Change string-copy-on-demand logic; do only if `not IsNullTerminated`.
Repository:
rC Clang
https://reviews.llvm.org/D42043
Files:
include/clang-c/CXString.h
tools/c-index-test/c-index-test.c
tools/libclang/CXString.cpp
tools/libclang/CXString.h
Index: tools/libclang/CXString.h
===
--- tools/libclang/CXString.h
+++ tools/libclang/CXString.h
@@ -106,4 +106,3 @@
}
#endif
-
Index: tools/libclang/CXString.cpp
===
--- tools/libclang/CXString.cpp
+++ tools/libclang/CXString.cpp
@@ -15,99 +15,141 @@
#include "CXString.h"
#include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
#include "clang-c/Index.h"
#include "clang/Frontend/ASTUnit.h"
#include "llvm/Support/ErrorHandling.h"
+#include
-using namespace clang;
-
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
- /// CXString contains a 'const char *' that it doesn't own.
- CXS_Unmanaged,
+static_assert(sizeof(CXString) <= 16, "");
- /// CXString contains a 'const char *' that it allocated with malloc().
- CXS_Malloc,
-
- /// CXString contains a CXStringBuf that needs to be returned to the
- /// CXStringPool.
- CXS_StringBuf
-};
+using namespace clang;
namespace clang {
namespace cxstring {
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage. We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1. When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented. When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be freed with \b free .
+ */
+struct RefCountedCharRange {
+ const char *Data;
+ const char *CString;
+ unsigned Size;
+ unsigned Count;
+};
+
//===--===//
// Basic generation of CXStrings.
//===--===//
CXString createEmpty() {
CXString Str;
- Str.data = "";
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) "";
+ Str.Size = 0;
+ Str.IsNullTerminated = true;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
CXString createNull() {
CXString Str;
- Str.data = nullptr;
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = nullptr;
+ Str.Size = 0;
+ Str.IsNullTerminated = false;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(const char *String) {
- if (String && String[0] == '\0')
+ if (!String) {
+return createNull();
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = String;
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) String;
+ Str.Size = strlen(String);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+ char *Spelling = static_cast(malloc(Size + 1));
+ memcpy(Spelling, CS, Size);
+ Spelling[Size] = 0;
+ return Spelling;
+}
+
CXString createDup(const char *String) {
- if (!String)
+ if (!String) {
return createNull();
-
- if (String[0] == '\0')
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = strdup(String);
- Str.private_flags = CXS_Malloc;
+ Str.Size = strlen(String);
+ Str.Contents = (const void *) copyCharRange(String, Str.Size);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = true;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(StringRef String) {
- // If the string is not nul-terminated, we have to make a copy.
-
- // FIXME: This is doing a one past end read, and should be removed! For memory
- // we don't manage, the API string can become unterminated at any time outside
- // our control.
-
- if (!String.empty() && String.data()[String.size()] != 0)
-return createDup(String);
-
- CXString Result;
- Result.data = String.data();
- Result.private_flags = (unsigned) CXS_Unmanaged;
- return Result;
+ assert (String.size() <= std::numeric_limits::max());
+ CXString Str;
+ Str.Size = unsigned(String.size());
+ Str.IsNullTerminated = false;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
+ auto *RC = new RefCountedCharRange {
+/* Data */ String.data(),
+/* CString */ nullptr,
+/* Size */ Str.Size,
+/* Cou
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
elsteveogrande created this revision.
Herald added a subscriber: cfe-commits.
Previous impl would read the byte past the end of a string (a
`llvm::StringRef`), possibly exceeding the allocation for that memory and
raising an MSAN issue.
This will instead save the character range specified by the `StringRef` and
copy it on-demand to a new C string.
Passes existing tests, and now passes with MSAN as well.
Repository:
rC Clang
https://reviews.llvm.org/D42043
Files:
include/clang-c/CXString.h
tools/c-index-test/c-index-test.c
tools/libclang/CXString.cpp
tools/libclang/CXString.h
Index: tools/libclang/CXString.h
===
--- tools/libclang/CXString.h
+++ tools/libclang/CXString.h
@@ -106,4 +106,3 @@
}
#endif
-
Index: tools/libclang/CXString.cpp
===
--- tools/libclang/CXString.cpp
+++ tools/libclang/CXString.cpp
@@ -15,99 +15,141 @@
#include "CXString.h"
#include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
#include "clang-c/Index.h"
#include "clang/Frontend/ASTUnit.h"
#include "llvm/Support/ErrorHandling.h"
+#include
-using namespace clang;
-
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
- /// CXString contains a 'const char *' that it doesn't own.
- CXS_Unmanaged,
+static_assert(sizeof(CXString) <= 16, "");
- /// CXString contains a 'const char *' that it allocated with malloc().
- CXS_Malloc,
-
- /// CXString contains a CXStringBuf that needs to be returned to the
- /// CXStringPool.
- CXS_StringBuf
-};
+using namespace clang;
namespace clang {
namespace cxstring {
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage. We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1. When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented. When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be freed with \b free .
+ */
+struct RefCountedCharRange {
+ const char *Data;
+ const char *CString;
+ unsigned Size;
+ unsigned Count;
+};
+
//===--===//
// Basic generation of CXStrings.
//===--===//
CXString createEmpty() {
CXString Str;
- Str.data = "";
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) "";
+ Str.Size = 0;
+ Str.IsNullTerminated = true;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
CXString createNull() {
CXString Str;
- Str.data = nullptr;
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = nullptr;
+ Str.Size = 0;
+ Str.IsNullTerminated = false;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(const char *String) {
- if (String && String[0] == '\0')
+ if (!String) {
+return createNull();
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = String;
- Str.private_flags = CXS_Unmanaged;
+ Str.Contents = (const void *) String;
+ Str.Size = strlen(String);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = false;
+ Str.IsPooled = false;
return Str;
}
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+ char *Spelling = static_cast(malloc(Size + 1));
+ memcpy(Spelling, CS, Size);
+ Spelling[Size] = 0;
+ return Spelling;
+}
+
CXString createDup(const char *String) {
- if (!String)
+ if (!String) {
return createNull();
-
- if (String[0] == '\0')
+ }
+ if (String[0] == '\0') {
return createEmpty();
+ }
CXString Str;
- Str.data = strdup(String);
- Str.private_flags = CXS_Malloc;
+ Str.Size = strlen(String);
+ Str.Contents = (const void *) copyCharRange(String, Str.Size);
+ Str.IsNullTerminated = true;
+ Str.IsOwned = true;
+ Str.IsPooled = false;
return Str;
}
CXString createRef(StringRef String) {
- // If the string is not nul-terminated, we have to make a copy.
-
- // FIXME: This is doing a one past end read, and should be removed! For memory
- // we don't manage, the API string can become unterminated at any time outside
- // our control.
-
- if (!String.empty() && String.data()[String.size()] != 0)
-return createDup(String);
-
- CXString Result;
- Result.data = String.data();
- Result.private_flags = (unsigned) CXS_Unmanaged;
- return Result;
+ assert (String.size() <= std::numeric_limits::max());
+ CXString Str;
+
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande updated this revision to Diff 128593.
elsteveogrande added a comment.
Update: now works properly with modules as well.
This needed to handle the case where a `ParmDecl` is already inheriting, but
it's innocuously inheriting from the same decl. Now
`inheritDefaultTemplateArgument` will return successfully and avoid the
assertion error saying that `default argument already inherited`.
We'll now possibly step once or twice through the inheritance list, so when a
`ParmDecl` is inheriting from another `ParmDecl`, which *itself* is inheriting
from a third decl. This is the case where `inheritDefaultTemplateArgument` is
called with `From` being an inheriting (and not "owning") template parameter.
Modules make use of the `Chain` struct as well, and IIUC this can lead to one
extra level of indirection, as it too has a `ParmDecl *` field. So this now
handles up to 2 indirections before validating that the inheritance is
consistent.
Passes existing and new tests; also works with a mini project that uses
modules, which was previously broken.
Repository:
rC Clang
https://reviews.llvm.org/D38320
Files:
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/PCH/cxx-templates.cpp
test/PCH/cxx-templates.h
Index: test/PCH/cxx-templates.h
===
--- test/PCH/cxx-templates.h
+++ test/PCH/cxx-templates.h
@@ -361,3 +361,38 @@
namespace MemberSpecializationLocation {
template struct A { static int n; };
}
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+
+// case 1: defaulted `NonTypeTemplateParmDecl`, non-defaulted 2nd tpl param
+template
+int func1(T const &);
+
+template
+int func1(T const &) {
+ return foo;
+}
+
+// case 2: defaulted `TemplateTypeParmDecl`, non-defaulted 2nd tpl param
+template
+A func2(B const &);
+
+template
+A func2(B const &) {
+ return A(20.0f);
+}
+
+// case 3: defaulted `TemplateTemplateParmDecl`, non-defaulted 2nd tpl param
+template
+struct Container { T const &item; };
+
+template class C = Container, class D>
+C func3(D const &);
+
+template class C, class D>
+C func3(D const &d) {
+ return Container{d};
+}
+
+} // end namespace PR34728
Index: test/PCH/cxx-templates.cpp
===
--- test/PCH/cxx-templates.cpp
+++ test/PCH/cxx-templates.cpp
@@ -116,3 +116,19 @@
#endif
int k = A::n;
}
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+int test() {
+ // Verify with several TemplateParmDecl kinds, using PCH (incl. modules).
+ int z1 = func1(/*ignored*/2.718);
+ int z2 = func2(/*ignored*/3.142);
+ int tmp3 = 30;
+ Container c = func3(tmp3);
+ int z3 = c.item;
+
+ // Return value is meaningless. Just "use" all these values to avoid
+ // warning about unused vars / values.
+ return z1 + z2 + z3;
+}
+} // end namespace PR34728
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -1542,11 +1542,24 @@
Record.push_back(D->wasDeclaredWithTypename());
- bool OwnsDefaultArg = D->hasDefaultArgument() &&
-!D->defaultArgumentWasInherited();
- Record.push_back(OwnsDefaultArg);
- if (OwnsDefaultArg)
-Record.AddTypeSourceInfo(D->getDefaultArgumentInfo());
+ TypeSourceInfo *OwnedDefaultArg = nullptr;
+ Decl const *InheritsFromDecl = nullptr;
+ if (D->hasDefaultArgument()) {
+if (D->defaultArgumentWasInherited()) {
+ InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+} else {
+ OwnedDefaultArg = D->getDefaultArgumentInfo();
+}
+ }
+
+ Record.push_back(OwnedDefaultArg != nullptr);
+ if (OwnedDefaultArg) {
+Record.AddTypeSourceInfo(OwnedDefaultArg);
+ }
+ Record.push_back(InheritsFromDecl != nullptr);
+ if (InheritsFromDecl) {
+Record.AddDeclRef(InheritsFromDecl);
+ }
Code = serialization::DECL_TEMPLATE_TYPE_PARM;
}
@@ -1573,11 +1586,26 @@
} else {
// Rest of NonTypeTemplateParmDecl.
Record.push_back(D->isParameterPack());
-bool OwnsDefaultArg = D->hasDefaultArgument() &&
- !D->defaultArgumentWasInherited();
-Record.push_back(OwnsDefaultArg);
-if (OwnsDefaultArg)
- Record.AddStmt(D->getDefaultArgument());
+
+Expr *OwnedDefaultArg = nullptr;
+Decl const *InheritsFromDecl = nullptr;
+if (D->hasDefaultArgument()) {
+ if (D->defaultArgumentWasInherited()) {
+InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+ } else {
+OwnedDefaultArg = D->getDefaultArgument();
+ }
+}
+
+Record.push_back(OwnedDefaultArg != nullptr);
+if (OwnedDefaultArg) {
+ Record.AddStmt(OwnedDefaultArg);
+}
+Record.push_back(InheritsFromDecl != nullptr);
+if (InheritsFromDecl) {
+ Record.AddDeclRef(InheritsFromDecl);
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande updated this revision to Diff 128466.
elsteveogrande edited the summary of this revision.
elsteveogrande added a comment.
Fixed local var names. Using `arc diff` which hopefully submits latest changes
w/ full context
Repository:
rC Clang
https://reviews.llvm.org/D38320
Files:
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/PCH/cxx-templates.cpp
test/PCH/cxx-templates.h
Index: test/PCH/cxx-templates.h
===
--- test/PCH/cxx-templates.h
+++ test/PCH/cxx-templates.h
@@ -361,3 +361,38 @@
namespace MemberSpecializationLocation {
template struct A { static int n; };
}
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+
+// case 1: defaulted `NonTypeTemplateParmDecl`, non-defaulted 2nd tpl param
+template
+int func1(T const &);
+
+template
+int func1(T const &) {
+ return foo;
+}
+
+// case 2: defaulted `TemplateTypeParmDecl`, non-defaulted 2nd tpl param
+template
+A func2(B const &);
+
+template
+A func2(B const &) {
+ return 69;
+}
+
+// case 3: defaulted `TemplateTemplateParmDecl`, non-defaulted 2nd tpl param
+template
+struct Container { T const &item; };
+
+template class C = Container, class D>
+C func3(D const &);
+
+template class C, class D>
+C func3(D const &d) {
+ return Container{d};
+}
+
+} // end namespace PR34728
Index: test/PCH/cxx-templates.cpp
===
--- test/PCH/cxx-templates.cpp
+++ test/PCH/cxx-templates.cpp
@@ -116,3 +116,14 @@
#endif
int k = A::n;
}
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+int test() {
+ int z1 = func1(2.718);
+ int z2 = func2(3.142);
+ int z3 = 12;
+ Container c = func3(z3);
+ return z1 + z2 + c.item;
+}
+} // end namespace PR34728
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -1542,11 +1542,24 @@
Record.push_back(D->wasDeclaredWithTypename());
- bool OwnsDefaultArg = D->hasDefaultArgument() &&
-!D->defaultArgumentWasInherited();
- Record.push_back(OwnsDefaultArg);
- if (OwnsDefaultArg)
-Record.AddTypeSourceInfo(D->getDefaultArgumentInfo());
+ TypeSourceInfo *OwnedDefaultArg = nullptr;
+ Decl const *InheritsFromDecl = nullptr;
+ if (D->hasDefaultArgument()) {
+if (D->defaultArgumentWasInherited()) {
+ InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+} else {
+ OwnedDefaultArg = D->getDefaultArgumentInfo();
+}
+ }
+
+ Record.push_back(OwnedDefaultArg != nullptr);
+ if (OwnedDefaultArg) {
+Record.AddTypeSourceInfo(OwnedDefaultArg);
+ }
+ Record.push_back(InheritsFromDecl != nullptr);
+ if (InheritsFromDecl) {
+Record.AddDeclRef(InheritsFromDecl);
+ }
Code = serialization::DECL_TEMPLATE_TYPE_PARM;
}
@@ -1573,11 +1586,26 @@
} else {
// Rest of NonTypeTemplateParmDecl.
Record.push_back(D->isParameterPack());
-bool OwnsDefaultArg = D->hasDefaultArgument() &&
- !D->defaultArgumentWasInherited();
-Record.push_back(OwnsDefaultArg);
-if (OwnsDefaultArg)
- Record.AddStmt(D->getDefaultArgument());
+
+Expr *OwnedDefaultArg = nullptr;
+Decl const *InheritsFromDecl = nullptr;
+if (D->hasDefaultArgument()) {
+ if (D->defaultArgumentWasInherited()) {
+InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+ } else {
+OwnedDefaultArg = D->getDefaultArgument();
+ }
+}
+
+Record.push_back(OwnedDefaultArg != nullptr);
+if (OwnedDefaultArg) {
+ Record.AddStmt(OwnedDefaultArg);
+}
+Record.push_back(InheritsFromDecl != nullptr);
+if (InheritsFromDecl) {
+ Record.AddDeclRef(InheritsFromDecl);
+}
+
Code = serialization::DECL_NON_TYPE_TEMPLATE_PARM;
}
}
@@ -1602,11 +1630,26 @@
} else {
// Rest of TemplateTemplateParmDecl.
Record.push_back(D->isParameterPack());
-bool OwnsDefaultArg = D->hasDefaultArgument() &&
- !D->defaultArgumentWasInherited();
-Record.push_back(OwnsDefaultArg);
-if (OwnsDefaultArg)
- Record.AddTemplateArgumentLoc(D->getDefaultArgument());
+
+llvm::Optional OwnedDefaultArg;
+Decl const *InheritsFromDecl = nullptr;
+if (D->hasDefaultArgument()) {
+ if (D->defaultArgumentWasInherited()) {
+InheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+ } else {
+OwnedDefaultArg = D->getDefaultArgument();
+ }
+}
+
+Record.push_back(OwnedDefaultArg.hasValue());
+if (OwnedDefaultArg.hasValue()) {
+ Record.AddTemplateArgumentLoc(*OwnedDefaultArg);
+}
+Record.push_back(InheritsFromDecl != nullptr);
+if (InheritsFromDecl) {
+ Record.AddDeclRef(InheritsFromDecl);
+}
+
Code = serialization::DECL_
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande added a comment. Hi @arphaman, thanks very much! Sorry, I didn't notice there was activity on this old patch until now. I'll make these changes as suggested and re-send it with full context. Repository: rL LLVM https://reviews.llvm.org/D38320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types
elsteveogrande added a comment. Hi @bruno, @rsmith, does this approach look ok? I couldn't easily figure out a better way to store inherited-default-info; doing it alongside the default value seemed the cleanest. Note: I saw there are three ways to store these data inside the `DefaultArgStorage` struct, see here: https://reviews.llvm.org/diffusion/L/browse/cfe/trunk/include/clang/AST/DeclTemplate.h;315074$272 There's (1) the default arg; (2) the reference to the `Decl` having the default arg (inherited by this `Decl`); and (3) a pointer to a `Chain` link, which is the case when modules are used. The serializer already had (1) covered, and I'm adding (2) here. Maybe the most proper fix is to handle (3) as well. Although it seems no unit tests broke, so this is either ok as is, or there's no test for it :) So: right now the serialization stream generated in `ASTWriterDecl.cpp` looks sort of like: [ . . . other fields] bool defaultArgFollows [Stmt defaultArgStmtRef (if above is true)] bool inheritedFromDeclFollows [Decl inheritedFromDecl (if above is true)] This could be changed to: [ . . . other fields] bool isPartOfChain <- add this bool defaultArgFollows [Stmt defaultArgStmtRef (if above is true)] bool inheritedFromDeclFollows [Decl inheritedFromDecl (if above is true)] so this can store the `DefaultArgStorage` with better fidelity. Thoughts? Thanks! :) Repository: rL LLVM https://reviews.llvm.org/D38320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
