From 39ad513e9628e6555b038de19d23864bf5a9b95e Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 13 Oct 2021 16:51:53 -0400 Subject: [PATCH 1/6] Draft fix for _Ptr TypeLoc with tests (enhanced 2021-10-13) --- clang/include/clang/Sema/DeclSpec.h | 13 +++ clang/lib/AST/Decl.cpp | 8 +- clang/lib/AST/TypeLoc.cpp | 9 ++ clang/lib/Parse/ParseDecl.cpp | 2 + clang/lib/Sema/DeclSpec.cpp | 8 ++ clang/lib/Sema/SemaType.cpp | 10 ++ clang/unittests/AST/SourceLocationTest.cpp | 105 +++++++++++++++++++++ 7 files changed, 153 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h index 1d49358b025e..cff6975684a2 100644 --- a/clang/include/clang/Sema/DeclSpec.h +++ b/clang/include/clang/Sema/DeclSpec.h @@ -410,6 +410,8 @@ class DeclSpec { /// otherwise, it is the same as TSTLoc. Hence, the pair TSTLoc and /// TSTNameLoc provides source range info for tag types. SourceLocation TSTNameLoc; + // Corresponds to PointerTypeLoc. + SourceLocation CheckedPtrKWLoc, CheckedPtrLeftSymLoc, CheckedPtrRightSymLoc; SourceRange TypeofParensRange; SourceLocation TQ_constLoc, TQ_restrictLoc, TQ_volatileLoc, TQ_atomicLoc, TQ_unalignedLoc; @@ -562,6 +564,13 @@ class DeclSpec { assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename); return TSTNameLoc; } + SourceLocation getCheckedPtrKWLoc() const { return CheckedPtrKWLoc; } + SourceLocation getCheckedPtrLeftSymLoc() const { + return CheckedPtrLeftSymLoc; + } + SourceLocation getCheckedPtrRightSymLoc() const { + return CheckedPtrRightSymLoc; + } SourceRange getTypeofParensRange() const { return TypeofParensRange; } void setTypeofParensRange(SourceRange range) { TypeofParensRange = range; } @@ -843,6 +852,10 @@ class DeclSpec { bool SetConstexprSpec(ConstexprSpecKind ConstexprKind, SourceLocation Loc, const char *&PrevSpec, unsigned &DiagID); + // TODO: Error checks? + void setSpecCheckedPtr(SourceLocation KWLoc, SourceLocation LeftSymLoc, + SourceLocation RightSymLoc); + bool isFriendSpecified() const { return Friend_specified; } SourceLocation getFriendSpecLoc() const { return FriendLoc; } diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index a07076111819..63db857e8f5f 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -1900,9 +1900,13 @@ static bool typeIsPostfix(QualType QT) { switch (T->getTypeClass()) { default: return false; - case Type::Pointer: - QT = cast(T)->getPointeeType(); + case Type::Pointer: { + const PointerType *PT = cast(T); + if (PT->isChecked()) + return false; + QT = PT->getPointeeType(); break; + } case Type::BlockPointer: QT = cast(T)->getPointeeType(); break; diff --git a/clang/lib/AST/TypeLoc.cpp b/clang/lib/AST/TypeLoc.cpp index 222b1abac510..d3a515e711e6 100644 --- a/clang/lib/AST/TypeLoc.cpp +++ b/clang/lib/AST/TypeLoc.cpp @@ -212,6 +212,12 @@ SourceLocation TypeLoc::getBeginLoc() const { case Qualified: Cur = Cur.getNextTypeLoc(); continue; + case Pointer: + if (Cur.castAs().getTypePtr()->isChecked()) { + LeftMost = Cur; + break; + } + LLVM_FALLTHROUGH; default: if (Cur.getLocalSourceRange().getBegin().isValid()) LeftMost = Cur; @@ -249,6 +255,9 @@ SourceLocation TypeLoc::getEndLoc() const { Last = Cur; break; case Pointer: + if (Cur.castAs().getTypePtr()->isChecked()) + return Cur.getLocalSourceRange().getEnd(); + LLVM_FALLTHROUGH; case BlockPointer: case MemberPointer: case LValueReference: diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp index 34d2d59ac845..d29653d59285 100644 --- a/clang/lib/Parse/ParseDecl.cpp +++ b/clang/lib/Parse/ParseDecl.cpp @@ -7837,6 +7837,7 @@ void Parser::ParseCheckedPointerSpecifiers(DeclSpec &DS) { "Not a checked pointer specifier"); tok::TokenKind Kind = Tok.getKind(); SourceLocation StartLoc = ConsumeToken(); + SourceLocation LeftLoc = Tok.getLocation(); if (ExpectAndConsume(tok::less)) { return; } @@ -7884,6 +7885,7 @@ void Parser::ParseCheckedPointerSpecifiers(DeclSpec &DS) { DiagID, Result.get(), Actions.getASTContext().getPrintingPolicy())) Diag(StartLoc, DiagID) << PrevSpec; + DS.setSpecCheckedPtr(StartLoc, LeftLoc, EndLoc); } /// [Checked C] Parse a specifier for an existential type. diff --git a/clang/lib/Sema/DeclSpec.cpp b/clang/lib/Sema/DeclSpec.cpp index fb146aec724e..a31a145c3c3a 100644 --- a/clang/lib/Sema/DeclSpec.cpp +++ b/clang/lib/Sema/DeclSpec.cpp @@ -1184,6 +1184,14 @@ bool DeclSpec::SetConstexprSpec(ConstexprSpecKind ConstexprKind, return false; } +void DeclSpec::setSpecCheckedPtr(SourceLocation KWLoc, + SourceLocation LeftSymLoc, + SourceLocation RightSymLoc) { + this->CheckedPtrKWLoc = KWLoc; + this->CheckedPtrLeftSymLoc = LeftSymLoc; + this->CheckedPtrRightSymLoc = RightSymLoc; +} + void DeclSpec::SaveWrittenBuiltinSpecs() { writtenBS.Sign = static_cast(getTypeSpecSign()); writtenBS.Width = static_cast(getTypeSpecWidth()); diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp index 3d67743895f1..6c952bd9a2ae 100644 --- a/clang/lib/Sema/SemaType.cpp +++ b/clang/lib/Sema/SemaType.cpp @@ -6289,6 +6289,16 @@ namespace { TL.setNameLoc(DS.getTypeSpecTypeLoc()); } + void VisitPointerTypeLoc(PointerTypeLoc TL) { + // TODO: Error checks? + TL.setKWLoc(DS.getCheckedPtrKWLoc()); + TL.setLeftSymLoc(DS.getCheckedPtrLeftSymLoc()); + TL.setRightSymLoc(DS.getCheckedPtrRightSymLoc()); + TypeSourceInfo *TInfo = nullptr; + Sema::GetTypeFromParser(DS.getRepAsType(), &TInfo); + TL.getNextTypeLoc().initializeFullCopy(TInfo->getTypeLoc()); + } + void VisitTypeLoc(TypeLoc TL) { // FIXME: add other typespec types and change this to an assert. TL.initialize(Context, DS.getTypeSpecTypeLoc()); diff --git a/clang/unittests/AST/SourceLocationTest.cpp b/clang/unittests/AST/SourceLocationTest.cpp index c1565c8f4c30..05d4057d034e 100644 --- a/clang/unittests/AST/SourceLocationTest.cpp +++ b/clang/unittests/AST/SourceLocationTest.cpp @@ -293,6 +293,111 @@ TEST(TypeLoc, LongLongConstRange) { EXPECT_TRUE(Verifier.match("long long const a = 0;", typeLoc())); } +class VarDeclTypeLocRangeVerifier : public RangeVerifier { +private: + unsigned int Depth; + +public: + VarDeclTypeLocRangeVerifier(unsigned int Depth) : Depth(Depth) {} + +protected: + SourceRange getRange(const VarDecl &Node) override { + TypeLoc TL = Node.getTypeSourceInfo()->getTypeLoc(); + for (unsigned int i = 0; i < Depth; i++) + TL = TL.getNextTypeLoc(); + return TL.getSourceRange(); + } +}; + +// Test that we get the right source range for the entire _Ptr<...>. +// +// If we just use `typeLoc()` as the matcher, we seem to get an inner TypeLoc. +// Instead, explicitly get the TypeLoc of the VarDecl. +TEST(CheckedPtr, TypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 11); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); +} + +// Test that we copied the TypeLoc for all levels of the pointee too. +TEST(CheckedPtr, TypeLocPointee) { + VarDeclTypeLocRangeVerifier Verifier{1}; + Verifier.expectRange(1, 6, 1, 10); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); +} +TEST(CheckedPtr, TypeLocPointee2) { + VarDeclTypeLocRangeVerifier Verifier{2}; + Verifier.expectRange(1, 6, 1, 6); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); +} + +// Test inner type layers that go on the outside of an unchecked pointer in +// inside-out C declarator syntax (and thus generally take priority in +// determining the outer limits of the source range) but go on the inside of +// _Ptr<...> (which should generally take priority over any inner type layer). + +// Tests of an (unchecked or checked) pointer to an array. + +TEST(UncheckedPtr, ArrayTypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 12); + EXPECT_TRUE(Verifier.match("int (*p)[10];", varDecl(), Lang_C99)); +} +// The *Postfix tests exercise DeclaratorDecl::getSourceRange, which uses +// typeIsPostfix to decide whether the type extends past the name in order to +// choose either the name or the end of the type as the end of the source range. +// typeIsPostfix has logic roughly parallel to TypeLoc::getEndLoc. (Could the +// code be refactored to remove this duplication?) +// +// In this case, typeIsPostfix should return true. If it returns false, the +// range would incorrectly end at the `p`. +TEST(UncheckedPtr, ArrayPostfix) { + RangeVerifier Verifier; + Verifier.expectRange(1, 1, 1, 12); + EXPECT_TRUE(Verifier.match("int (*p)[10];", varDecl(), Lang_C99)); +} +TEST(CheckedPtr, ArrayTypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 13); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); +} +// In this case, typeIsPostfix should return false. If it returns true, the +// range would incorrectly end at the `>`. +TEST(CheckedPtr, ArrayPostfix) { + RangeVerifier Verifier; + Verifier.expectRange(1, 1, 1, 15); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); +} + +// Tests of an (unchecked or checked) pointer to a function. Here we have to use +// hasGlobalStorage to ensure we match only the global VarDecl, not the +// ParmVarDecl of the function. + +TEST(UncheckedPtr, FunctionTypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 13); + EXPECT_TRUE( + Verifier.match("int (*p)(int);", varDecl(hasGlobalStorage()), Lang_C99)); +} +TEST(CheckedPtr, FunctionTypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 15); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(hasGlobalStorage()), + Lang_C99)); +} +TEST(UncheckedPtr, FunctionPostfix) { + RangeVerifier Verifier; + Verifier.expectRange(1, 1, 1, 13); + EXPECT_TRUE( + Verifier.match("int (*p)(int);", varDecl(hasGlobalStorage()), Lang_C99)); +} +TEST(CheckedPtr, FunctionPostfix) { + RangeVerifier Verifier; + Verifier.expectRange(1, 1, 1, 17); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(hasGlobalStorage()), + Lang_C99)); +} + TEST(CXXConstructorDecl, NoRetFunTypeLocRange) { RangeVerifier Verifier; Verifier.expectRange(1, 11, 1, 13); From 680fa4176f2bd0503a02d0e7370a5b66026a35c6 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 13 Oct 2021 17:10:44 -0400 Subject: [PATCH 2/6] Fix getEndLoc --- clang/lib/AST/TypeLoc.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/clang/lib/AST/TypeLoc.cpp b/clang/lib/AST/TypeLoc.cpp index d3a515e711e6..cb57bb02372d 100644 --- a/clang/lib/AST/TypeLoc.cpp +++ b/clang/lib/AST/TypeLoc.cpp @@ -255,8 +255,11 @@ SourceLocation TypeLoc::getEndLoc() const { Last = Cur; break; case Pointer: - if (Cur.castAs().getTypePtr()->isChecked()) - return Cur.getLocalSourceRange().getEnd(); + if (Cur.castAs().getTypePtr()->isChecked()) { + if (!Last) + Last = Cur; + return Last.getLocalSourceRange().getEnd(); + } LLVM_FALLTHROUGH; case BlockPointer: case MemberPointer: From 57b00784ec471ba0a0f7511c928999280b626695 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 13 Oct 2021 17:12:22 -0400 Subject: [PATCH 3/6] 3C changes for testing --- clang/lib/3C/ConstraintVariables.cpp | 2 +- clang/lib/3C/Utils.cpp | 2 ++ clang/test/3C/partial_checked.c | 3 +++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 2e699c354439..9227bf7e8116 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -2267,7 +2267,7 @@ FVComponentVariable::FVComponentVariable(const QualType &QT, const SourceManager &SM = C.getSourceManager(); SourceLocation DLoc = D->getLocation(); CharSourceRange CSR; - if (SM.isBeforeInTranslationUnit(DRange.getEnd(), DLoc)) { + if (false /*SM.isBeforeInTranslationUnit(DRange.getEnd(), DLoc)*/) { // It's not clear to me why, but the end of the SourceRange for the // declaration can come before the SourceLocation for the declaration. // This can result in SourceDeclaration failing to capture the whole diff --git a/clang/lib/3C/Utils.cpp b/clang/lib/3C/Utils.cpp index 780455412cd0..51e6cc811c07 100644 --- a/clang/lib/3C/Utils.cpp +++ b/clang/lib/3C/Utils.cpp @@ -622,6 +622,7 @@ SourceRange getDeclSourceRangeWithAnnotations( return SR; SourceLocation DeclEnd = SR.getEnd(); +#if 0 // Partial workaround for a compiler bug where if D has certain checked // pointer types such as `_Ptr` (seen in the partial_checked.c // regression test), D->getSourceRange() returns only the _Ptr token (TODO: @@ -631,6 +632,7 @@ SourceRange getDeclSourceRangeWithAnnotations( SourceLocation DeclLoc = D->getLocation(); if (SM.isBeforeInTranslationUnit(DeclEnd, DeclLoc)) DeclEnd = DeclLoc; +#endif SourceLocation AnnotationsEnd = getCheckedCAnnotationsEnd(D); if (AnnotationsEnd.isValid() && diff --git a/clang/test/3C/partial_checked.c b/clang/test/3C/partial_checked.c index d288ac94ea06..d5e23ad5825a 100644 --- a/clang/test/3C/partial_checked.c +++ b/clang/test/3C/partial_checked.c @@ -76,6 +76,9 @@ void test4() { // getDeclSourceRangeWithAnnotations for more information. _Ptr gm; // CHECK: _Ptr<_Ptr (void)> gm = ((void *)0); +_Ptr gma[10]; +// CHECK_NOALL: _Ptr<_Ptr (void)> gma[10] = {((void *)0)}; +// CHECK_ALL: _Ptr<_Ptr (void)> gma _Checked[10] = {((void *)0)}; void test5(_Ptr a, _Ptr b, _Ptr<_Ptr> c, int **d) { // CHECK: void test5(_Ptr<_Ptr> a, _Ptr b : itype(_Ptr<_Ptr>), _Ptr<_Ptr> c, _Ptr<_Ptr> d) { From 3726ed1d358f5535e2d1a67aa54f59836fb287da Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 13 Oct 2021 17:16:08 -0400 Subject: [PATCH 4/6] Add unit tests for the getEndLog bug I just fixed. --- clang/unittests/AST/SourceLocationTest.cpp | 24 ++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/clang/unittests/AST/SourceLocationTest.cpp b/clang/unittests/AST/SourceLocationTest.cpp index 05d4057d034e..06ff45956eee 100644 --- a/clang/unittests/AST/SourceLocationTest.cpp +++ b/clang/unittests/AST/SourceLocationTest.cpp @@ -379,18 +379,18 @@ TEST(UncheckedPtr, FunctionTypeLoc) { EXPECT_TRUE( Verifier.match("int (*p)(int);", varDecl(hasGlobalStorage()), Lang_C99)); } -TEST(CheckedPtr, FunctionTypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; - Verifier.expectRange(1, 1, 1, 15); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(hasGlobalStorage()), - Lang_C99)); -} TEST(UncheckedPtr, FunctionPostfix) { RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 13); EXPECT_TRUE( Verifier.match("int (*p)(int);", varDecl(hasGlobalStorage()), Lang_C99)); } +TEST(CheckedPtr, FunctionTypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 15); + EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(hasGlobalStorage()), + Lang_C99)); +} TEST(CheckedPtr, FunctionPostfix) { RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 17); @@ -398,6 +398,18 @@ TEST(CheckedPtr, FunctionPostfix) { Lang_C99)); } +// Test with array levels both inside and outside a checked pointer level. +TEST(CheckedPtr, SandwichArrayTypeLoc) { + VarDeclTypeLocRangeVerifier Verifier{0}; + Verifier.expectRange(1, 1, 1, 18); + EXPECT_TRUE(Verifier.match("_Ptr p[1];", varDecl(), Lang_C99)); +} +TEST(CheckedPtr, SandwichArrayPostfix) { + RangeVerifier Verifier; + Verifier.expectRange(1, 1, 1, 18); + EXPECT_TRUE(Verifier.match("_Ptr p[1];", varDecl(), Lang_C99)); +} + TEST(CXXConstructorDecl, NoRetFunTypeLocRange) { RangeVerifier Verifier; Verifier.expectRange(1, 11, 1, 13); From 1ccd488c5fe891ebeaad2d7a1af692b597124a50 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 13 Oct 2021 20:25:26 -0400 Subject: [PATCH 5/6] Clean up comments and unit tests. --- clang/include/clang/Sema/DeclSpec.h | 1 - clang/lib/AST/Decl.cpp | 1 + clang/lib/AST/TypeLoc.cpp | 8 ++ clang/lib/Sema/SemaType.cpp | 2 +- clang/unittests/AST/SourceLocationTest.cpp | 123 +++++++-------------- 5 files changed, 50 insertions(+), 85 deletions(-) diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h index cff6975684a2..b38886fe9bd9 100644 --- a/clang/include/clang/Sema/DeclSpec.h +++ b/clang/include/clang/Sema/DeclSpec.h @@ -852,7 +852,6 @@ class DeclSpec { bool SetConstexprSpec(ConstexprSpecKind ConstexprKind, SourceLocation Loc, const char *&PrevSpec, unsigned &DiagID); - // TODO: Error checks? void setSpecCheckedPtr(SourceLocation KWLoc, SourceLocation LeftSymLoc, SourceLocation RightSymLoc); diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index 63db857e8f5f..d8b209ce355b 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -1903,6 +1903,7 @@ static bool typeIsPostfix(QualType QT) { case Type::Pointer: { const PointerType *PT = cast(T); if (PT->isChecked()) + // See the comment about checked pointers in TypeLoc::getBeginLoc. return false; QT = PT->getPointeeType(); break; diff --git a/clang/lib/AST/TypeLoc.cpp b/clang/lib/AST/TypeLoc.cpp index cb57bb02372d..2fd803f48725 100644 --- a/clang/lib/AST/TypeLoc.cpp +++ b/clang/lib/AST/TypeLoc.cpp @@ -214,6 +214,13 @@ SourceLocation TypeLoc::getBeginLoc() const { continue; case Pointer: if (Cur.castAs().getTypePtr()->isChecked()) { + // Unlike the usual "inside-out" C declarator syntax, checked pointer + // types (_Ptr, etc.) are "right-side-out". Since T is syntactically + // contained in _Ptr, it can't affect the source range of the whole + // type. Thus, the source range code can ignore T and treat _Ptr the + // same way as a single-token type such as a primitive or typedef. Here, + // a single-token type would hit the `default` case with + // Cur.getNextTypeLoc().isNull() returning true. LeftMost = Cur; break; } @@ -256,6 +263,7 @@ SourceLocation TypeLoc::getEndLoc() const { break; case Pointer: if (Cur.castAs().getTypePtr()->isChecked()) { + // See the comment about checked pointers in TypeLoc::getBeginLoc. if (!Last) Last = Cur; return Last.getLocalSourceRange().getEnd(); diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp index 6c952bd9a2ae..3f50fbdd9f20 100644 --- a/clang/lib/Sema/SemaType.cpp +++ b/clang/lib/Sema/SemaType.cpp @@ -6290,7 +6290,7 @@ namespace { } void VisitPointerTypeLoc(PointerTypeLoc TL) { - // TODO: Error checks? + // REVIEW: Should we have any error checks here? TL.setKWLoc(DS.getCheckedPtrKWLoc()); TL.setLeftSymLoc(DS.getCheckedPtrLeftSymLoc()); TL.setRightSymLoc(DS.getCheckedPtrRightSymLoc()); diff --git a/clang/unittests/AST/SourceLocationTest.cpp b/clang/unittests/AST/SourceLocationTest.cpp index 06ff45956eee..a477cb19e148 100644 --- a/clang/unittests/AST/SourceLocationTest.cpp +++ b/clang/unittests/AST/SourceLocationTest.cpp @@ -293,118 +293,75 @@ TEST(TypeLoc, LongLongConstRange) { EXPECT_TRUE(Verifier.match("long long const a = 0;", typeLoc())); } -class VarDeclTypeLocRangeVerifier : public RangeVerifier { -private: - unsigned int Depth; - -public: - VarDeclTypeLocRangeVerifier(unsigned int Depth) : Depth(Depth) {} - -protected: - SourceRange getRange(const VarDecl &Node) override { - TypeLoc TL = Node.getTypeSourceInfo()->getTypeLoc(); - for (unsigned int i = 0; i < Depth; i++) - TL = TL.getNextTypeLoc(); - return TL.getSourceRange(); - } -}; - -// Test that we get the right source range for the entire _Ptr<...>. -// -// If we just use `typeLoc()` as the matcher, we seem to get an inner TypeLoc. -// Instead, explicitly get the TypeLoc of the VarDecl. +// For multi-level types such as pointers, if we just used typeLoc() as above, +// it would match the TypeLocs of all the levels and the RangeVerifier would +// verify an arbitrary one of them. Instead, we want to verify a specific level. +internal::BindableMatcher typeLocAtDepth(unsigned int Depth) { + internal::BindableMatcher Matcher = typeLoc(hasParent(varDecl())); + for (unsigned int I = 0; I < Depth; I++) + Matcher = typeLoc(hasParent(Matcher)); + return Matcher; +} + +// Test the source range of a checked pointer type and all levels of its +// pointee. TEST(CheckedPtr, TypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; + RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 11); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); + EXPECT_TRUE(Verifier.match("_Ptr p;", typeLocAtDepth(0), Lang_C99)); } - -// Test that we copied the TypeLoc for all levels of the pointee too. TEST(CheckedPtr, TypeLocPointee) { - VarDeclTypeLocRangeVerifier Verifier{1}; + RangeVerifier Verifier; Verifier.expectRange(1, 6, 1, 10); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); + EXPECT_TRUE(Verifier.match("_Ptr p;", typeLocAtDepth(1), Lang_C99)); } TEST(CheckedPtr, TypeLocPointee2) { - VarDeclTypeLocRangeVerifier Verifier{2}; + RangeVerifier Verifier; Verifier.expectRange(1, 6, 1, 6); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); + EXPECT_TRUE(Verifier.match("_Ptr p;", typeLocAtDepth(2), Lang_C99)); } -// Test inner type layers that go on the outside of an unchecked pointer in -// inside-out C declarator syntax (and thus generally take priority in -// determining the outer limits of the source range) but go on the inside of -// _Ptr<...> (which should generally take priority over any inner type layer). - -// Tests of an (unchecked or checked) pointer to an array. - -TEST(UncheckedPtr, ArrayTypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; - Verifier.expectRange(1, 1, 1, 12); - EXPECT_TRUE(Verifier.match("int (*p)[10];", varDecl(), Lang_C99)); -} -// The *Postfix tests exercise DeclaratorDecl::getSourceRange, which uses +// Tests of checked pointers in combination with arrays (as an example of a type +// layer that uses the usual inside-out C declarator syntax). Also, some +// corresponding tests of unchecked pointers to verify that the special handling +// of checked pointers doesn't unintentionally affect unchecked pointers. +// +// For each example declaration, we test the source ranges of both the TypeLoc +// and the VarDecl. The VarDecl uses DeclaratorDecl::getSourceRange, which uses // typeIsPostfix to decide whether the type extends past the name in order to // choose either the name or the end of the type as the end of the source range. // typeIsPostfix has logic roughly parallel to TypeLoc::getEndLoc. (Could the // code be refactored to remove this duplication?) -// -// In this case, typeIsPostfix should return true. If it returns false, the -// range would incorrectly end at the `p`. -TEST(UncheckedPtr, ArrayPostfix) { - RangeVerifier Verifier; - Verifier.expectRange(1, 1, 1, 12); - EXPECT_TRUE(Verifier.match("int (*p)[10];", varDecl(), Lang_C99)); -} + TEST(CheckedPtr, ArrayTypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; + RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 13); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); + EXPECT_TRUE(Verifier.match("_Ptr p;", typeLocAtDepth(0), Lang_C99)); } -// In this case, typeIsPostfix should return false. If it returns true, the -// range would incorrectly end at the `>`. -TEST(CheckedPtr, ArrayPostfix) { +TEST(CheckedPtr, ArrayDecl) { RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 15); EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(), Lang_C99)); } - -// Tests of an (unchecked or checked) pointer to a function. Here we have to use -// hasGlobalStorage to ensure we match only the global VarDecl, not the -// ParmVarDecl of the function. - -TEST(UncheckedPtr, FunctionTypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; - Verifier.expectRange(1, 1, 1, 13); - EXPECT_TRUE( - Verifier.match("int (*p)(int);", varDecl(hasGlobalStorage()), Lang_C99)); -} -TEST(UncheckedPtr, FunctionPostfix) { - RangeVerifier Verifier; - Verifier.expectRange(1, 1, 1, 13); - EXPECT_TRUE( - Verifier.match("int (*p)(int);", varDecl(hasGlobalStorage()), Lang_C99)); -} -TEST(CheckedPtr, FunctionTypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; - Verifier.expectRange(1, 1, 1, 15); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(hasGlobalStorage()), - Lang_C99)); +TEST(CheckedPtr, UncheckedArrayTypeLoc) { + RangeVerifier Verifier; + Verifier.expectRange(1, 1, 1, 12); + EXPECT_TRUE(Verifier.match("int (*p)[10];", typeLocAtDepth(0), Lang_C99)); } -TEST(CheckedPtr, FunctionPostfix) { +TEST(CheckedPtr, UncheckedArrayDecl) { RangeVerifier Verifier; - Verifier.expectRange(1, 1, 1, 17); - EXPECT_TRUE(Verifier.match("_Ptr p;", varDecl(hasGlobalStorage()), - Lang_C99)); + Verifier.expectRange(1, 1, 1, 12); + EXPECT_TRUE(Verifier.match("int (*p)[10];", varDecl(), Lang_C99)); } // Test with array levels both inside and outside a checked pointer level. TEST(CheckedPtr, SandwichArrayTypeLoc) { - VarDeclTypeLocRangeVerifier Verifier{0}; + RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 18); - EXPECT_TRUE(Verifier.match("_Ptr p[1];", varDecl(), Lang_C99)); + EXPECT_TRUE( + Verifier.match("_Ptr p[1];", typeLocAtDepth(0), Lang_C99)); } -TEST(CheckedPtr, SandwichArrayPostfix) { +TEST(CheckedPtr, SandwichArrayDecl) { RangeVerifier Verifier; Verifier.expectRange(1, 1, 1, 18); EXPECT_TRUE(Verifier.match("_Ptr p[1];", varDecl(), Lang_C99)); From cff93a373578e0f02901b2f110dfced97cd1da10 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Thu, 14 Oct 2021 11:07:41 -0400 Subject: [PATCH 6/6] Some 3C tests for ParmVarDecl source ranges. Needs more work, plus we should add tests for the FVComponentVariable::SourceDeclaration cases. --- clang/test/3C/partial_checked.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/clang/test/3C/partial_checked.c b/clang/test/3C/partial_checked.c index d5e23ad5825a..06c9c5e9184c 100644 --- a/clang/test/3C/partial_checked.c +++ b/clang/test/3C/partial_checked.c @@ -80,6 +80,10 @@ _Ptr gma[10]; // CHECK_NOALL: _Ptr<_Ptr (void)> gma[10] = {((void *)0)}; // CHECK_ALL: _Ptr<_Ptr (void)> gma _Checked[10] = {((void *)0)}; +// TODO: Better names and CHECK comments +void gmf(_Ptr a, int *b) {} +void gmaf(_Ptr a[10], int *b) {} + void test5(_Ptr a, _Ptr b, _Ptr<_Ptr> c, int **d) { // CHECK: void test5(_Ptr<_Ptr> a, _Ptr b : itype(_Ptr<_Ptr>), _Ptr<_Ptr> c, _Ptr<_Ptr> d) { *b = 1;