FunctionDecl::getBody() returning nullptr

[Aaron Ballman via cfe-commits]

On Wed, Aug 19, 2015 at 5:23 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> It looks like this would only happen for a late-parsed template that the
> analysis code is checking before it is parsed. Should we really be running
> these checks at all in that case?

This code is being called from DataRecursiveASTVisitor (using an
AnalysisContext) on generic Decl objects to determine whether we
*should* run a checker or not. I think it's reasonable, but am not
overly familiar with this part of the code.

> Also, it looks like this code doesn't actually want the body at all, and
> just wants to get the location of the definition. Asking for the body here
> does not seem like the right approach, because it'll cause the external AST
> source (if there is one) to fault the body in.
>
> The right fix is probably something like:
>
>   const FunctionDecl *Def;
>   SourceLocation SL = (D->isDefined(Def) ? Def : D)->getLocation();

I've attached an updated patch with this suggested approach. It
appears to work nicely. If you like the approach, I'll see what I can
come up with for a test case and commit.

~Aaron

>
> On Wed, Aug 19, 2015 at 1:44 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> The attached patch resolves the null pointer crash I am seeing.
>>
>> ~Aaron
>>
>> On Wed, Aug 19, 2015 at 1:24 PM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>> > On Wed, Aug 19, 2015 at 11:33 AM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >> When I run the following test code through clang-tidy -checks=*, I get
>> >> a crash from some relatively strange behavior with FunctionDecl.
>> >>
>> >> template <class T> struct remove_reference      {typedef T type;};
>> >> template <class T> struct remove_reference<T&>  {typedef T type;};
>> >> template <class T> struct remove_reference<T&&> {typedef T type;};
>> >>
>> >> template <typename T>
>> >> typename remove_reference<T>::type&& move(T&& arg) {
>> >>   return static_cast<typename remove_reference<T>::type&&>(arg);
>> >> }
>> >>
>> >> AnalysisConsumer::getModeForDecl() is called, and it has code that
>> >> does: D->hasBody() ? D->getBody()->stuff : stuff;
>> >>
>> >> What's strange is that hasBody() returns true, but getBody() returns
>> >> nullptr. I don't think that this should be possible. I'm wondering
>> >> whether it's purposeful that hasBody() does not check
>> >> Definition->Body?
>> >
>> > Looking into this a bit further, the issue is that hasBody() checks
>> > for Definition->Body *or* Definition->IsLateTemplateParsed when
>> > deciding to return true. In my case, Body is null, but
>> > IsLateTemplateParsed is true, so hasBody() returns true. However,
>> > getBody() doesn't have any special logic for late template parsed
>> > function bodies.
>> >
>> >> Also, the FunctionDecl in question is for move(), which does have a
>> >> body, so I do not understand why the definition would claim there is
>> >> no body.
>> >>
>> >> Ideas, or should I file a bug report?
>> >
>> > From my further look, I wonder if the correct solution to this is to
>> > simply call getBody() and handle a nullptr return instead of calling
>> > hasBody() first. It basically makes the check to ignore late parsed
>> > template bodies an implicit one.
>> >
>> > ~Aaron
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AnalysisConsumer.cpp.patch
Type: application/octet-stream
Size: 818 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150819/6271558e/attachment.obj>