On Wednesday 06 August 2014 09:10:35 Olivier Goffart wrote: > On Tuesday 05 August 2014 19:48:00 Richard Smith wrote: > > On Tue, Aug 5, 2014 at 3:16 PM, Olivier Goffart <[email protected]> wrote: > > > Hi, > > > > > > This fixes a crash in the RecursiveASTVisitor on such code: > > > __typeof__(struct F*) var[invalid]; > > > > > > The main problem is that when the type is invalid, we don't even > > > try to generate TypeSourceInfo for it, which lead to a crash when we try > > > to visit them in the tools. > > > This is solved in SemaType.cpp by actually generating the TypeSourceInfo > > > even for invalid types. > > > > I'm surprised by this: the code used to call getTrivialTypeSourceInfo in > > this case, which does create a TypeSourceInfo object. How do we end up > > with > > the TSI being null? > > the TSI is not null. It is somehow corrupted. > Valgrind trace show that the RecursiveASTVisitor tries to access > uninitialized memory on this line: > TRY_TO(TraverseTypeLoc(TL.getUnderlyingTInfo()->getTypeLoc())) > > And actually I realize now that the problem is something else: > ASTContext::getTrivialTypeSourceInfo calls TypeOfTypeloc::initializeLocal > but there is no initializeLocal in TypeOfTypeLoc, only in the base class > TypeofLikeTypeLoc, which does not intitialize > TypeOfTypeLocInfo::UnderlyingTInfo, hence the access to uninitialized > memory in the ASTVisitor and then the crash. > So this probably should be fixed as well for the other uses of > getTrivialTypeSourceInfo. > > But getTrivialTypeSourceInfo still looses all the source locations of all > the sub types, and I feel they should be kept even for invalid types in > order to be highlighted properly in IDEs > > > > The second problem is that if there is an error parsing the size of the > > > array, we bail out without actually registering that it should have been > > > an > > > array. Fix that Parser::ParseBracketDeclarator. > > > Move the check for invalid type a bit up in Sema::ActOnUninitializedDecl > > > in order to avoid unnecessary diagnostic > > > > This part looks fine. Is it possible to add a testcase for the improved > > recovery here, without the rest of the patch?
Hi, I tested my patch on a larger codebase and had some different crashes on invalid code. So the original patch is not good. I also tried to initialize getLocalData()->UnderlyingTInfo to nullptr in a new TypeOfTypeLoc::initializeLocal function. But this is not enough to slove the problem as the code in RecursiveASTVisitor assume it is not null (so i guess some other places might assume it not null?) I attached a safest patch which do not try to generate the typeloc for the invalid type, but at least fixes the crash in that case. But I noticed that other type might have the same problem -- Olivier
>From f7bdaca3a39042ee19c9198d2bdeac97df596e94 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <[email protected]> Date: Wed, 6 Aug 2014 21:40:46 +0200 Subject: [PATCH] Fix initializing TypeOfTypeLoc and a crash in RecursiveASTVisitor This fixes a crash in the RecursiveASTVisitor on such code __typeof__(struct F*) var[invalid]; The UnderlyingTInfo of a TypeOfTypeLoc was left uninitialized when created from ASTContext::getTrivialTypeSourceInfo This lead to a crash in RecursiveASTVisitor when trying to access it. Initialize it to nullptr, and check that the underlying type is not null before visiting it in the RecursiveASTVisitor --- include/clang/AST/RecursiveASTVisitor.h | 3 ++- include/clang/AST/TypeLoc.h | 7 +++++++ unittests/Tooling/RecursiveASTVisitorTest.cpp | 11 +++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/include/clang/AST/RecursiveASTVisitor.h b/include/clang/AST/RecursiveASTVisitor.h index ca66631..69d45bd 100644 --- a/include/clang/AST/RecursiveASTVisitor.h +++ b/include/clang/AST/RecursiveASTVisitor.h @@ -1157,7 +1157,8 @@ DEF_TRAVERSE_TYPELOC(TypeOfExprType, { TRY_TO(TraverseStmt(TL.getUnderlyingExpr())); }) DEF_TRAVERSE_TYPELOC(TypeOfType, { - TRY_TO(TraverseTypeLoc(TL.getUnderlyingTInfo()->getTypeLoc())); + if (TL.getUnderlyingTInfo()) + TRY_TO(TraverseTypeLoc(TL.getUnderlyingTInfo()->getTypeLoc())); }) // FIXME: location of underlying expr diff --git a/include/clang/AST/TypeLoc.h b/include/clang/AST/TypeLoc.h index 3648d2a..a8c64bb 100644 --- a/include/clang/AST/TypeLoc.h +++ b/include/clang/AST/TypeLoc.h @@ -1567,6 +1567,13 @@ public: void setUnderlyingTInfo(TypeSourceInfo* TI) const { this->getLocalData()->UnderlyingTInfo = TI; } + + void initializeLocal(ASTContext &Context, SourceLocation Loc) { + TypeofLikeTypeLoc<TypeOfTypeLoc, TypeOfType, TypeOfTypeLocInfo> + ::initializeLocal(Context, Loc); + this->getLocalData()->UnderlyingTInfo = nullptr; + } + }; // FIXME: location of the 'decltype' and parens. diff --git a/unittests/Tooling/RecursiveASTVisitorTest.cpp b/unittests/Tooling/RecursiveASTVisitorTest.cpp index a1a93a5..6b17dc5 100644 --- a/unittests/Tooling/RecursiveASTVisitorTest.cpp +++ b/unittests/Tooling/RecursiveASTVisitorTest.cpp @@ -540,6 +540,17 @@ TEST(RecursiveASTVisitor, VisitsObjCPropertyType) { TypeLocVisitor::Lang_OBJC)); } +TEST(RecursiveASTVisitor, VisitInvalidType) { + TypeLocVisitor Visitor; + // FIXME: It would be nice to have information about subtypes of invalid type + //Visitor.ExpectMatch("typeof(struct F *) []", 1, 1); + // Even if the full type is invalid, it should still find sub types + //Visitor.ExpectMatch("struct F", 1, 19); + EXPECT_FALSE(Visitor.runOver( + "__typeof__(struct F*) var[invalid];\n", + TypeLocVisitor::Lang_C)); +} + TEST(RecursiveASTVisitor, VisitsLambdaExpr) { LambdaExprVisitor Visitor; Visitor.ExpectMatch("", 1, 12); -- 2.0.4
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
