On Wed, Aug 19, 2015 at 2:52 PM, Aaron Ballman <aa...@aaronballman.com>
wrote:
> On Wed, Aug 19, 2015 at 5:23 PM, Richard Smith <rich...@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.
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.
~Aaron
>
> >
> > On Wed, Aug 19, 2015 at 1:44 PM, Aaron Ballman <aa...@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 <aa...@aaronballman.com>
> >> wrote:
> >> > On Wed, Aug 19, 2015 at 11:33 AM, Aaron Ballman <
> aa...@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
> >
> >
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
