Closed Bug 1447817 Opened 6 years ago Closed 6 years ago

./mach static-analysis doesn't seem to match documentation or let --checks work

Categories

(Firefox Build System :: Mach Core, enhancement)

Product:
Firefox Build System
Component:
Mach Core
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: tjr, Assigned: andi)

Details

Attachments

(1 file)

Bug 1447817 - correlate mach static-analysis documentation with configuration file.
59 bytes, text/x-review-board-request
Sylvestre
: review+
Details 
./mach static-analysis check --help says

> --checks checks, -c checks
>                        Static analysis checks to enable. By default, this
>                        enables only custom Mozilla checks, but can be any
>                        clang-tidy checks syntax.


But when I run ./mach static-analysis check it says

> 0:01.43 Enabled checks:
> 0:01.43     clang-analyzer-core.CallAndMessage
> 0:01.43     clang-analyzer-core.DivideZero
> ...

And outputs lots of stuff other than mozilla-*.


If I try ./mach static-analysis check --checks mozilla-* it outputs

>  0:01.34 Enabled checks:
> 0:01.35     clang-analyzer-apiModeling.google.GTest
> 0:01.35     clang-analyzer-core.CallAndMessage
> ...

And _no_ mozilla checks. This is probably do to shell expansion. It would be neat if I could omit the final * and get the same behavior.

Anyway though, if I try ./mach static-analysis check --checks "mozilla-*" to bypass shell expansion I get

>  0:01.24 Enabled checks:
> 0:01.24     clang-analyzer-apiModeling.google.GTest
> 0:01.24     clang-analyzer-core.CallAndMessage
> ...

But _also_ the mozilla- tests.




So I think my issues are 

1) The documentation is wrong. All tests are run by default. Either the doc or behavior should change.
2) --checks doesn't work correctly
3) --checks with '*' as a wildcard is a footgun because of shell expansion, and it'd be great if the way it behaved was if it didn't match a check name exactly, it was used as a prefix filter even if you don't put a '*' on it.
Assignee: nobody → bpostelnicu
Thanks for bringing this up, indeed there is an inconsistency here. I'm gonna fix this asap. The problem with this is that the default checks that are used are taken from: https://dxr.mozilla.org/mozilla-central/source/tools/clang-tidy/config.yaml
An the documentation hasn't been updated. 

When you do this:
>>./mach static-analysis check --checks "mozilla-*"

You don't actually instruct clang-tidy to use only mozilla checkers, but to add them, thing that is already present since we use our own version of clang-tidy that has by default inserted our own checkers.
In order to use let's say only mozilla specific checker the syntax would be:
>>./mach static-analysis check --checks "-*, mozilla-*"

I admit looking at this right now, it's a bit confusing.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> When you do this:
> >>./mach static-analysis check --checks "mozilla-*"
> 
> You don't actually instruct clang-tidy to use only mozilla checkers, but to
> add them, thing that is already present since we use our own version of
> clang-tidy that has by default inserted our own checkers.
> In order to use let's say only mozilla specific checker the syntax would be:
> >>./mach static-analysis check --checks "-*, mozilla-*"
> 
> I admit looking at this right now, it's a bit confusing.

Hm. What I do that, only the mozilla checks are listed, but I still gets lots of output like:

> 0:04.36 /home/tom/Documents/moz/mozilla-unified-lto/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:398:3: error: C++ requires a type specifier for all declarations [clang-diagnostic-error]
> 0:04.36   static_assert(!IsArray<ValueT>::value,
> 0:04.36   ^
> 0:04.36 /home/tom/Documents/moz/mozilla-unified-lto/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:398:17: error: expected ')' [clang-diagnostic-error]
> 0:04.36   static_assert(!IsArray<ValueT>::value,
> 0:04.36                 ^
> 0:04.36 /home/tom/Documents/moz/mozilla-unified-lto/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:398:16: note: to match this '('
> 0:04.36   static_assert(!IsArray<ValueT>::value,
> 0:04.36                ^
> 0:04.36 /home/tom/Documents/moz/mozilla-unified-lto/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:398:17: error: expected parameter declarator [clang-diagnostic-error]
> 0:04.36   static_assert(!IsArray<ValueT>::value,
> 0:04.36                 ^

And I mean a *lot* of this output. I'd never find the actual static analysis output.
The problem that you encounter is related with the fact that somehow clang-tidy package is corrupted, I can't tell you why this is happening but you can fix this by:
>>rm -rf ~/.mozbuild/clang-tools/
Re-run the static-analysis tool and it will automatically re-download the needed tools.
It is working also without the quote. Example:
./mach static-analysis check --checks=-*,mozilla-nan-expr dom/presentation/

FYI, this is the clang-tidy syntax.
That's why the default behavior was not to add a prefix of '-*' thus removing all checkers. I'll update the documentation shortly and also the default checkers that are used to be in compliance with:
https://dxr.mozilla.org/mozilla-central/source/tools/clang-tidy/config.yaml
Comment on attachment 8962013 [details]
Bug 1447817 - correlate mach static-analysis documentation with configuration file.

https://reviewboard.mozilla.org/r/230852/#review236340

::: python/mozbuild/mozbuild/mach_commands.py:1620
(Diff revision 1)
>                            'the analysis is only performed on the files changed '
>                            'in the patch streamed through stdin.  This is called '
>                            'the diff mode.')
>      @CommandArgument('--checks', '-c', default='-*', metavar='checks',
>                       help='Static analysis checks to enable.  By default, this enables only '
> -                     'custom Mozilla checks, but can be any clang-tidy checks syntax.')
> +                     'checks that are published here: https://goo.gl/DPVkHt, but can be any '

please use https://mzl.la/2DRHeTh instead
Attachment #8962013 - Flags: review?(sledru)
Comment on attachment 8962013 [details]
Bug 1447817 - correlate mach static-analysis documentation with configuration file.

https://reviewboard.mozilla.org/r/230852/#review236342
Attachment #8962013 - Flags: review?(sledru) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afd7670ce90b
correlate mach static-analysis documentation with configuration file. r=sylvestre
https://hg.mozilla.org/mozilla-central/rev/afd7670ce90b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: