<p dir="ltr">Thanks for the further investigation and testing! Could you recursively create a trivial TypeSourceInfo for the TypeOfTypeLoc case?</p>
<div class="gmail_quote">On 6 Aug 2014 12:47, "Olivier Goffart" <<a href="mailto:ogoffart@woboq.com">ogoffart@woboq.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Wednesday 06 August 2014 09:10:35 Olivier Goffart wrote:<br>
> On Tuesday 05 August 2014 19:48:00 Richard Smith wrote:<br>
> > On Tue, Aug 5, 2014 at 3:16 PM, Olivier Goffart <<a href="mailto:ogoffart@woboq.com">ogoffart@woboq.com</a>><br>
wrote:<br>
> > > Hi,<br>
> > ><br>
> > > This fixes a crash in the RecursiveASTVisitor on such code:<br>
> > >  __typeof__(struct F*) var[invalid];<br>
> > ><br>
> > > The main problem is that when the type is invalid, we don't even<br>
> > > try to generate TypeSourceInfo for it, which lead to a crash when we try<br>
> > > to visit them in the tools.<br>
> > > This is solved in SemaType.cpp by actually generating the TypeSourceInfo<br>
> > > even for invalid types.<br>
> ><br>
> > I'm surprised by this: the code used to call getTrivialTypeSourceInfo in<br>
> > this case, which does create a TypeSourceInfo object. How do we end up<br>
> > with<br>
> > the TSI being null?<br>
><br>
> the TSI is not null. It is somehow corrupted.<br>
> Valgrind trace show that the RecursiveASTVisitor tries to access<br>
> uninitialized memory on this line:<br>
> TRY_TO(TraverseTypeLoc(TL.getUnderlyingTInfo()->getTypeLoc()))<br>
><br>
> And actually I realize now that the problem is something else:<br>
> ASTContext::getTrivialTypeSourceInfo  calls TypeOfTypeloc::initializeLocal<br>
> but there is no initializeLocal in TypeOfTypeLoc,  only in the base class<br>
> TypeofLikeTypeLoc,  which does not intitialize<br>
> TypeOfTypeLocInfo::UnderlyingTInfo,  hence the access to uninitialized<br>
> memory in the ASTVisitor and then the crash.<br>
> So this probably should be fixed as well for the other uses of<br>
> getTrivialTypeSourceInfo.<br>
><br>
> But getTrivialTypeSourceInfo still looses all the source locations of all<br>
> the sub types, and I feel they should be kept even for invalid types in<br>
> order to be highlighted properly in IDEs<br>
><br>
> > > The second problem is that if there is an error parsing the size of the<br>
> > > array, we bail out without actually registering that it should have been<br>
> > > an<br>
> > > array. Fix that Parser::ParseBracketDeclarator.<br>
> > > Move the check for invalid type a bit up in Sema::ActOnUninitializedDecl<br>
> > > in order to avoid unnecessary diagnostic<br>
> ><br>
> > This part looks fine. Is it possible to add a testcase for the improved<br>
> > recovery here, without the rest of the patch?<br>
<br>
<br>
Hi,<br>
<br>
I tested my patch on a larger codebase and had some different crashes on<br>
invalid code. So the original patch is not good.<br>
<br>
I also tried to initialize getLocalData()->UnderlyingTInfo to nullptr in a new<br>
TypeOfTypeLoc::initializeLocal function.  But this is not enough to slove the<br>
problem as the code in RecursiveASTVisitor assume it is not null (so i guess<br>
some other places might assume it not null?)<br>
<br>
I attached a safest patch which do not try to generate the typeloc for the<br>
invalid type, but at least fixes the crash in that case.<br>
But I noticed that other type might have the same problem<br>
<br>
<br>
--<br>
Olivier<br>
<br>
<br>
<br>
</blockquote></div>