[PATCH] Fix generating TypeSourceInfo for invalid array type

[Olivier Goffart]

On Tuesday 05 August 2014 19:48:00 Richard Smith wrote:
> On Tue, Aug 5, 2014 at 3:16 PM, Olivier Goffart <ogoffart at 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?