1180549 - Fix a startup crash when using the clang-plugin with clang>3.5

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

The unique_ptr temporary object returned by getOtherConsumer() would be
destroyed, which would cause the vector to point to a deleted object!
With this fixed, the plugin can be built with more recent versions of
clang.

Comment 1

[(no longer active)]

Attachment: Fix a startup crash when using the clang-plugin with clang>3.5

Comment 2

[(no longer active)]

Botond, I think you may be interested in this.  This patch fixes the crash that I was seeing when using the plugin on recent versions of clang!  With this fixed, I hope to be able to upgrade the clang versions we use for static analysis on the infrastructure to the ToT clang.  \o/

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8629750 [details] [diff] [review]
Fix a startup crash when using the clang-plugin with clang>3.5

Review of attachment 8629750 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +1199,5 @@
>  class MozCheckAction : public PluginASTAction {
>  public:
>    ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI, StringRef fileName) override {
>  #if CLANG_VERSION_FULL >= 306
> +    std::unique_ptr<MozChecker> checker(llvm::make_unique<MozChecker>(CI));

Is there any particular reason for the addition of the llvm:: to make_unique? It's already in namespace, and seems unnecessary.

@@ -1206,2 @@
>      consumers.push_back(std::move(checker));
> -    consumers.push_back(checker->getOtherConsumer());

Isn't the problem that we were de-referencing checker after moving it into consumers here? The explanation in the commit message seems odd to me.

We could also fix this (I think) by just swapping the above two lines, but it's probably better to act the same as below.

Comment 4

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #3)
> ::: build/clang-plugin/clang-plugin.cpp
> @@ +1199,5 @@
> >  class MozCheckAction : public PluginASTAction {
> >  public:
> >    ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI, StringRef fileName) override {
> >  #if CLANG_VERSION_FULL >= 306
> > +    std::unique_ptr<MozChecker> checker(llvm::make_unique<MozChecker>(CI));
> 
> Is there any particular reason for the addition of the llvm:: to
> make_unique? It's already in namespace, and seems unnecessary.

Just copying what LLVM consumers do, and a bit of C++14 safety.  :-)

> @@ -1206,2 @@
> >      consumers.push_back(std::move(checker));
> > -    consumers.push_back(checker->getOtherConsumer());
> 
> Isn't the problem that we were de-referencing checker after moving it into
> consumers here? The explanation in the commit message seems odd to me.
> 
> We could also fix this (I think) by just swapping the above two lines, but
> it's probably better to act the same as below.

You're right!  I will fix the commit message.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/277e60059a10

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/277e60059a10