[PATCH] Fix generating TypeSourceInfo for invalid array type

[Olivier Goffart]

On Wednesday 06 August 2014 14:58:41 Richard Smith wrote:
> Thanks for the further investigation and testing! Could you recursively
> create a trivial TypeSourceInfo for the TypeOfTypeLoc case?

Any hint on how to create them in this part of the code?


> On 6 Aug 2014 12:47, "Olivier Goffart" <ogoffart at woboq.com> wrote:
> > 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 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?
> > 
> > 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