<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 19, 2015 at 2:52 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Aug 19, 2015 at 5:23 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> It looks like this would only happen for a late-parsed template that the<br>
> analysis code is checking before it is parsed. Should we really be running<br>
> these checks at all in that case?<br>
<br>
</span>This code is being called from DataRecursiveASTVisitor (using an<br>
AnalysisContext) on generic Decl objects to determine whether we<br>
*should* run a checker or not. I think it's reasonable, but am not<br>
overly familiar with this part of the code.<br>
<span class=""><br>
> Also, it looks like this code doesn't actually want the body at all, and<br>
> just wants to get the location of the definition. Asking for the body here<br>
> does not seem like the right approach, because it'll cause the external AST<br>
> source (if there is one) to fault the body in.<br>
><br>
> The right fix is probably something like:<br>
><br>
>   const FunctionDecl *Def;<br>
>   SourceLocation SL = (D->isDefined(Def) ? Def : D)->getLocation();<br>
<br>
</span>I've attached an updated patch with this suggested approach. It<br>
appears to work nicely. If you like the approach, I'll see what I can<br>
come up with for a test case and commit.</blockquote><div><br></div><div>I suspect this patch breaks Objective-C cases (I hadn't previously noticed that D was just a Decl*, not a FunctionDecl*). Since it's using a RecursiveASTVisitor, it's going to deserialize everything anyway, so we don't really gain from formulating it this way. Let's go with your original patch.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
~Aaron<br>
<br>
><br>
> On Wed, Aug 19, 2015 at 1:44 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> The attached patch resolves the null pointer crash I am seeing.<br>
>><br>
>> ~Aaron<br>
>><br>
>> On Wed, Aug 19, 2015 at 1:24 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>> > On Wed, Aug 19, 2015 at 11:33 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >> When I run the following test code through clang-tidy -checks=*, I get<br>
>> >> a crash from some relatively strange behavior with FunctionDecl.<br>
>> >><br>
>> >> template <class T> struct remove_reference      {typedef T type;};<br>
>> >> template <class T> struct remove_reference<T&>  {typedef T type;};<br>
>> >> template <class T> struct remove_reference<T&&> {typedef T type;};<br>
>> >><br>
>> >> template <typename T><br>
>> >> typename remove_reference<T>::type&& move(T&& arg) {<br>
>> >>   return static_cast<typename remove_reference<T>::type&&>(arg);<br>
>> >> }<br>
>> >><br>
>> >> AnalysisConsumer::getModeForDecl() is called, and it has code that<br>
>> >> does: D->hasBody() ? D->getBody()->stuff : stuff;<br>
>> >><br>
>> >> What's strange is that hasBody() returns true, but getBody() returns<br>
>> >> nullptr. I don't think that this should be possible. I'm wondering<br>
>> >> whether it's purposeful that hasBody() does not check<br>
>> >> Definition->Body?<br>
>> ><br>
>> > Looking into this a bit further, the issue is that hasBody() checks<br>
>> > for Definition->Body *or* Definition->IsLateTemplateParsed when<br>
>> > deciding to return true. In my case, Body is null, but<br>
>> > IsLateTemplateParsed is true, so hasBody() returns true. However,<br>
>> > getBody() doesn't have any special logic for late template parsed<br>
>> > function bodies.<br>
>> ><br>
>> >> Also, the FunctionDecl in question is for move(), which does have a<br>
>> >> body, so I do not understand why the definition would claim there is<br>
>> >> no body.<br>
>> >><br>
>> >> Ideas, or should I file a bug report?<br>
>> ><br>
>> > From my further look, I wonder if the correct solution to this is to<br>
>> > simply call getBody() and handle a nullptr return instead of calling<br>
>> > hasBody() first. It basically makes the check to ignore late parsed<br>
>> > template bodies an implicit one.<br>
>> ><br>
>> > ~Aaron<br>
><br>
><br>
</div></div></blockquote></div><br></div></div>