1324239 - Refactor the clang plugin

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

This patch mostly just splits up clang-plugin.cpp into separate files for
different classes or helpers.

Comment 1

[(no longer active)]

Attachment: Refactor the clang plugin

Comment 2

[(no longer active)]

Sorry for the enormous patch.  I think the easiest way to review it may be applying it to a local tree and inspecting things...  I only moved code around and the only edit changes are so that this would compile.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

I love it, great work :)

Comment 4

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

Comment on attachment 8819609 [details] [diff] [review]
Refactor the clang plugin

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

::: build/clang-plugin/DiagnosticsMatcher.h
@@ +54,5 @@
> +  SprintfLiteralChecker SprintfLiteral;
> +  OverrideBaseCallChecker OverrideBaseCall;
> +  OverrideBaseCallUsageChecker OverrideBaseCallUsage;
> +  NonParamInsideFunctionDeclChecker NonParamInsideFunctionDecl;
> +  MatchFinder AstMatcher;

I'm not the biggest fan of this design (you have to mention the analysis in so many different files to register it), but I've peeked ahead at the clang-tidy setup and I like how it turned out after adding clang-tidy support, so I'm OK with this.

::: build/clang-plugin/MozCheckAction.cpp
@@ +7,5 @@
> +class MozCheckAction : public PluginASTAction {
> +public:
> +  ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI,
> +                                   StringRef FileName) override {
> +#if CLANG_VERSION_FULL >= 306

Do we still support Clang 305? If we don't, can we remove the other now-dead branch of this code?

Follow-up of course.

::: build/clang-plugin/MozChecker.cpp
@@ +22,5 @@
> +
> +  return false;
> +}
> +
> +void MozChecker::handleUnusedExprResult(const Stmt *Statement) {

Was going to comment that the passes in this file should also be refactored into other files, but then I remembered the follow-ups. Awesome :).

::: build/clang-plugin/SprintfLiteralChecker.cpp
@@ +65,5 @@
> +      // Match calls like: const int y = 100; snprintf(x, y, ...);
> +      Literal = Result.Nodes.getNodeAs<IntegerLiteral>("constant");
> +    }
> +
> +    if (Type->getSize().ule(Literal->getValue())) {

I'm not sure if this is related to this patch (it probably isn't), but I tried to build this patch with my version of clang with assertions enabled and the error: "bool llvm::APInt::ult(const llvm::APInt&) const: Assertion `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"' failed." occurred. I imagine that this is caused by the operands to this method not always being the same bitwidth. I'd like to fix this in a followup.

::: build/clang-plugin/plugin.h
@@ +18,5 @@
> +#include "llvm/ADT/DenseMap.h"
> +#include "llvm/Support/FileSystem.h"
> +#include "llvm/Support/Path.h"
> +#include <memory>
> +#include <iterator>

I'm fairly confident that we don't need all of these headers in all of the files which we import plugin.h into. I imagine that some of the files (like "clang/ASTMatchers/ASTMatchers.h") would be important to always have imported, however, for example, I imagine that "clang/Sema/Sema.h" probably isn't needed everywhere and probably imports a huge amount of stuff which we don't need. Right now I imagine it won't make any differences (as we're unifying all of the .cpp files into a single unified build) but once we overflow into multiple compilation units, having to parse etc. less clang frontend header files will probably be a good thing.

Comment 5

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #4)
> ::: build/clang-plugin/MozCheckAction.cpp
> @@ +7,5 @@
> > +class MozCheckAction : public PluginASTAction {
> > +public:
> > +  ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI,
> > +                                   StringRef FileName) override {
> > +#if CLANG_VERSION_FULL >= 306
> 
> Do we still support Clang 305? If we don't, can we remove the other now-dead
> branch of this code?
> 
> Follow-up of course.

Bug 1324328 already removes this branch.  :-)

> ::: build/clang-plugin/SprintfLiteralChecker.cpp
> @@ +65,5 @@
> > +      // Match calls like: const int y = 100; snprintf(x, y, ...);
> > +      Literal = Result.Nodes.getNodeAs<IntegerLiteral>("constant");
> > +    }
> > +
> > +    if (Type->getSize().ule(Literal->getValue())) {
> 
> I'm not sure if this is related to this patch (it probably isn't), but I
> tried to build this patch with my version of clang with assertions enabled
> and the error: "bool llvm::APInt::ult(const llvm::APInt&) const: Assertion
> `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"'
> failed." occurred. I imagine that this is caused by the operands to this
> method not always being the same bitwidth. I'd like to fix this in a
> followup.

No we have had this assertion failure for ages.  If you can fix it, that would be awesome!  I'd love to turn our debug builders to use clangs with assertions turned on to make sure we don't regress once we fix this one (and perhaps others that have crept in).

> ::: build/clang-plugin/plugin.h
> @@ +18,5 @@
> > +#include "llvm/ADT/DenseMap.h"
> > +#include "llvm/Support/FileSystem.h"
> > +#include "llvm/Support/Path.h"
> > +#include <memory>
> > +#include <iterator>
> 
> I'm fairly confident that we don't need all of these headers in all of the
> files which we import plugin.h into. I imagine that some of the files (like
> "clang/ASTMatchers/ASTMatchers.h") would be important to always have
> imported, however, for example, I imagine that "clang/Sema/Sema.h" probably
> isn't needed everywhere and probably imports a huge amount of stuff which we
> don't need. Right now I imagine it won't make any differences (as we're
> unifying all of the .cpp files into a single unified build) but once we
> overflow into multiple compilation units, having to parse etc. less clang
> frontend header files will probably be a good thing.

I will give it a shot to remove a few of these...  But note that my goal in having plugin.h is to make it so that unified *and* non-unified builds will always work.  The reason is that when building with clang-tidy, we will be in non-unified mode, and I don't want to hurt our build times by making our builds also build in non-unified mode, so I decided to have one header that has all of the #includes and avoid #including things in multiple source files to simplify maintaining both build variants (especially because the non-unified build issues aren't detected in our local workflows as we probably won't build clang-tidy to hack on the plugin.)  So this was a very intentional trade-off.

Does this change your opinion?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfb48730883
Refactor the clang plugin; r=mystor

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/ddfb48730883

Comment 8

[(no longer active)]

needinfo for comment 5.

Comment 9

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

(In reply to :Ehsan Akhgari from comment #5)
> Bug 1324328 already removes this branch.  :-)

You can tell I didn't read all of the patches before reviewing :P

> No we have had this assertion failure for ages.  If you can fix it, that
> would be awesome!  I'd love to turn our debug builders to use clangs with
> assertions turned on to make sure we don't regress once we fix this one (and
> perhaps others that have crept in).

I would love that as well. I've filed bug 1326289 to fix it.

> I will give it a shot to remove a few of these...  But note that my goal in
> having plugin.h is to make it so that unified *and* non-unified builds will
> always work.  The reason is that when building with clang-tidy, we will be
> in non-unified mode, and I don't want to hurt our build times by making our
> builds also build in non-unified mode, so I decided to have one header that
> has all of the #includes and avoid #including things in multiple source
> files to simplify maintaining both build variants (especially because the
> non-unified build issues aren't detected in our local workflows as we
> probably won't build clang-tidy to hack on the plugin.)  So this was a very
> intentional trade-off.
> 
> Does this change your opinion?

My only holdback is that this probably significantly hurts the clang-tidy build times but, reliable builds which work both with unified builds and without it seem like they're probably worth it in the end ^.^. You've convinced me.