[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] Fix generating TypeSourceInfo for invalid array type
From: Olivier Goffart <ogoffart () woboq ! com>
Date: 2014-08-06 19:46:56
Message-ID: 1999389.6KtqYB4A9p () finn
[Download RAW message or body]
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 <ogoffart@woboq.com>
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
["0001-Fix-initializing-TypeOfTypeLoc-and-a-crash-in-Recurs.patch" (0001-Fix-initializing-TypeOfTypeLoc-and-a-crash-in-Recurs.patch)]
From f7bdaca3a39042ee19c9198d2bdeac97df596e94 Mon Sep 17 00:00:00 2001
From: Olivier Goffart <ogoffart@woboq.com>
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
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic