Fix for Clang PR 8426, 8427, & 8433

Hi Ted,

I have a fix for Clang static analyzer bugs 8426, 8427, & 8433 (they
turned out to be caused by the same thing). The patch is attached. I
also uploaded them to Issue 2716044: Fix Clang PR 8426, 8427, and 8433 - Code Review for easy
viewing/reviewing. Would you be able to review it? Thanks.

I have commit privilege, so I can commit it myself when you are OK
with it. Thanks.

pr8426.patch (3.11 KB)

Hi Zhanyong,

Thanks for working on this. I admit that I haven't had a chance to investigate those PRs myself.

The changes look good, but I had two questions/comments.

(1) Do we even want to be analyzing template definitions (as opposed to instantiations)? It seems like we'd be doing a bunch of guess work. It also seems like a performance issue if we analyze template code that is never used.

(2) I'd prefer to keep the test cases in one file, for faster processing by 'make test'. These can easily go into misc-ps-region-store.cpp. Each test case just needs a comment indicating the PR.

Ted

Would be VERY good if rietveld (or even gerrit) could be used (as main
tool) for reviewing Clang patches (as Chromium does). Much better than
attached patches or at bugzilla.

Hi Ted,

Hi Zhanyong,

Thanks for working on this. I admit that I haven't had a chance to investigate those PRs myself.

The changes look good, but I had two questions/comments.

(1) Do we even want to be analyzing template definitions (as opposed to instantiations)? It seems like we'd be doing a bunch of guess work. It also seems like a performance issue if we analyze template code that is never used.

I agree that there's little point analyzing template definitions. It
would be a quite invasive change to stop doing that though, so I'd
like to get this patch in before attempting that.

(2) I'd prefer to keep the test cases in one file, for faster processing by 'make test'. These can easily go into misc-ps-region-store.cpp. Each test case just needs a comment indicating the PR.

Good point. Done.

Please see the updated patch attached in this message. It's also
viewable at Issue 2716044: Fix Clang PR 8426, 8427, and 8433 - Code Review as patch set 2.
Thanks,

pr8426.patch (2.59 KB)

Hi Zhanyong,

I don't think it would quite invasive to stop analyzing template definitions, as it is the right thing to do. My concern is that your patch really only addresses specific symptoms, but not the general problem. If we don't intend to analyze template definitions, then we will likely just keep on hitting corner cases like these. The right thing is to have AnalysisConsumer not even construct GRExprEngine objects when faced with a template definition, and then in turn have GRExprEngine also do a similar sanity check.

Ted

How is such a change invasive? One can just check whether a FunctionDecl is a dependent context (FunctionDecl::isDependentContext()), and ignore it if it is.

  - Doug

Hi Ted and Doug,

pr8426.patch (2.11 KB)

Looks great! Thanks Zhanyong!

Thanks! Committed in r117853.