1503647 - Add support for building out of tree checkers in the clang plugin

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[(no longer active)]

It would be nice to be able to run some private checkers in our clang plugin which we don't publish the source code to, for security sensitive static analyses.

This basically amounts to adding a new build-time configuration flag like --with-clang-plugin-checks=/path/to/dir, which, once enabled, would add hooks inside the following places:

  * Add something to build/clang-plugin/moz.build to pick up /path/to/dir/moz.build.
  * Add something to build/clang-plugin/ChecksIncludes.inc to pick up /path/to/dir/ChecksIncludes.inc.
  * Add something to build/clang-plugin/Checks.inc to to pick up /path/to/dir/Checks.inc.

This would allow us to upload a new static checker in source form to tooltool and build it in infra when building clang-tidy.

Comment 1

[(no longer active)]

Actually, perhaps instead of the last step in comment 0, we should keep Checks.inc separate from the out of tree copy, and include the out of tree copy separately so that here <https://searchfox.org/mozilla-central/source/build/clang-plugin/MozillaTidyModule.cpp#20> we can give those checks a different prefix (e.g., mozilla-external-foo).

Comment 2

[Mike Hommey [:glandium]]

Why not just store your code in a patch form and apply that patch when building clang-tidy?

Comment 3

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #2)
> Why not just store your code in a patch form and apply that patch when
> building clang-tidy?

Because it would keep breaking as the underlying code changes.  Maintaining stuff that lives in tooltool is difficult since it wouldn't be obvious to update them as the in-tree code changes.

Comment 4

[Mike Hommey [:glandium]]

The only files where there could be conflicts are those you listed in comment 0, and if you add placeholder comments, patches that add stuff between the placeholders should apply to them with fuzzing without any problem. Even if there are problems, that woouldn't go unnoticed, since it would break the clang-tidy jobs.

Comment 5

[(no longer active)]

It's true, placeholder comments would make things better indeed.  Not 100% ideal but better.

Thinking more about this, if we could just have separate files that we wouldn't normally edit, that would really solve the problem.  We could keep those files mostly empty and only use them for applying the out-of-tree diffs.  Doing that for Checks.inc and ChecksIncludes.inc is trivial since those two are just headers that we #include, but what about moz.build?  Do we have a facility for including one moz.build file inside the same directory into another?

Comment 6

[Mike Hommey [:glandium]]

include()

Comment 7

[(no longer active)]

Great, thanks!  Then I think that would be a much simpler solution.

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Anyone working on this? I assume not so I was planning on picking this up this quarter, and trialing it with 1114683 and 1279569. Any objections/considerations, let me know, otherwise I'll go ahead.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Please go ahead! Don't hesitate to get in touch with Andi and Ehsan to gather input.

Comment 10

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I didn't make a lot of progress on this bug, so I'm removing myself from this for now. I (think I) managed to un-bitrot the clang-plugin for bug 1114683, but I hit a lot of challenges here, not least of which was my limited experience with writing/building c++. I had a lot more success with Semmle/LGTM, so I'm going to pursue that for now since we already have a private instance setup.