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" <[email protected]> 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 <[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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
