1480089 - [Static-Analysis][Clang-Tidy] Tests should also be run in bulk mode

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Andi [:andi]]

In order to make sure we don't regress we should also run our tests in bulk mode, all tests passed to clang-tidy in one execution pipe.

Comment 1

[Andi [:andi]]

Attachment: WIP WIP - Bug 1480089 - pass all of the test files to to our static-analysis pipeline.

Review commit: https://reviewboard.mozilla.org/r/260776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260776/

Comment 2

[Andi [:andi]]

Unfortunately I've discovered an issue that is present in clang-tidy 5.0 till 6.0.1, running:

clang-tidy  -checks=-*,bugprone-suspicious-memset-usage,clang-analyzer-cplusplus.NewDelete,clang-analyzer-cplusplus.NewDeleteLeaks,clang-analyzer-deadcode.DeadStores,clang-analyzer-security.FloatLoopCounter,clang-analyzer-security.insecureAPI.getpw,clang-analyzer-security.insecureAPI.mkstemp,clang-analyzer-security.insecureAPI.mktemp,clang-analyzer-security.insecureAPI.UncheckedReturn,clang-analyzer-security.insecureAPI.vfork,clang-analyzer-unix.Malloc,clang-analyzer-unix.cstring.BadSizeArg,clang-analyzer-unix.cstring.NullArg,misc-argument-comment,misc-assert-side-effect,misc-bool-pointer-implicit-conversion,misc-forward-declaration-namespace,misc-macro-repeated-side-effects,misc-string-constructor,misc-string-integer-assignment,misc-suspicious-missing-comma,misc-suspicious-semicolon,misc-swapped-arguments,misc-unused-alias-decls,misc-unused-raii,misc-unused-using-decls,modernize-avoid-bind,modernize-loop-convert,modernize-raw-string-literal,modernize-shrink-to-fit,modernize-use-bool-literals,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nullptr,performance-faster-string-find,performance-for-range-copy,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,readability-container-size-empty,readability-else-after-return,readability-misleading-indentation,readability-redundant-control-flow,readability-redundant-string-cstr,readability-redundant-string-init,readability-uniqueptr-delete-release -extra-arg=-DMOZ_CLANG_PLUGIN -p=/var/folders/21/5g_w1s552c3c60dtq5_74djw0000gn/T/ccSK4Ktr /Users/abpostelnicu/Projects/mozilla/mozilla-unified/tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp

we get:

clang-analyzer-unix.cstring.NullArg.cpp:7:10: warning: Null pointer argument in call to string length function [clang-analyzer-cplusplus.NewDeleteLeaks]
  return strlen(s); // warning
         ^
clang-analyzer-unix.cstring.NullArg.cpp:12:3: note: 's' initialized to a null pointer value
  const char* s = nullptr;
  ^
clang-analyzer-unix.cstring.NullArg.cpp:13:20: note: Passing null pointer value via 1st parameter 's'
  return my_strlen(s);
                   ^
clang-analyzer-unix.cstring.NullArg.cpp:13:10: note: Calling 'my_strlen'
  return my_strlen(s);
         ^
clang-analyzer-unix.cstring.NullArg.cpp:7:10: note: Null pointer argument in call to string length function
  return strlen(s); // warning
         ^
Suppressed 3 warnings (2 in non-user code, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
abpostelnicu@Mozilla-MacBook-Pro:~/Desktop$ code /Users/abpostelnicu/Projects/mozilla/mozilla-unified/tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp

The category of the deffect is wrong, it should be: clang-analyzer-unix.cstring.NullArg

This issue has been fixed in 7.0, but i didn't look to see what patch has fixed it.

Jan do you think it's possible to skip 6.0 and move directly to 7.0, even though 7.0 I don't think it's released yet?

Comment 3

[Andi [:andi]]

Comment on attachment 8996697 [details]
WIP WIP - Bug 1480089 - pass all of the test files to to our static-analysis pipeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260776/diff/1-2/

Comment 4

[Jan Keromnes [:janx]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)
> Unfortunately I've discovered an issue that is present in clang-tidy 5.0
> till 6.0.1

I was able to reproduce this on Ubuntu 16.04 with clang-tidy 6.0.1.

The bug happens because both checks "clang-analyzer-cplusplus.NewDeleteLeaks" and "clang-analyzer-unix.cstring.NullArg" seem to find the same defect, and for some reason when both checks are active, "NewDeleteLeaks" has the priority to report it:

1) Running with "NullArg" only:

    $ clang-tidy-6.0  -checks=-*,clang-analyzer-unix.cstring.NullArg tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp
    [...]
    /home/user/firefox/tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp:7:10: warning: Null pointer argument in call to string length function [clang-analyzer-unix.cstring.NullArg]

2) Running with "NewDeleteLeaks" only:

    $ clang-tidy-6.0  -checks=-*,clang-analyzer-cplusplus.NewDeleteLeaks tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp
    [...]
    /home/user/firefox/tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp:7:10: warning: Null pointer argument in call to string length function [clang-analyzer-cplusplus.NewDeleteLeaks]

3) Running with both "NullArg" and "NewDeleteLeaks" (check order doesn't matter):

    $ clang-tidy-6.0  -checks=-*,clang-analyzer-unix.cstring.NullArg,clang-analyzer-cplusplus.NewDeleteLeaks tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp
    [...]
    /home/user/firefox/tools/clang-tidy/test/clang-analyzer-unix.cstring.NullArg.cpp:7:10: warning: Null pointer argument in call to string length function [clang-analyzer-cplusplus.NewDeleteLeaks]

> This issue has been fixed in 7.0, but i didn't look to see what patch has
> fixed it.

Could you please find the patch? It would be good to back-port the fix into our tree.

> Jan do you think it's possible to skip 6.0 and move directly to 7.0, even
> though 7.0 I don't think it's released yet?

7.0 is indeed not yet released: https://llvm.org/svn/llvm-project/llvm/tags/

I'm not sure whether we can do such a thing. Maybe it's better for now to back-port the fix? (Note that bug 1466427 is upgrading 5.0.1 to 6.0.1, but that can happen independently of back-porting this fix.)

Comment 5

[Jan Keromnes [:janx]]

> WIP WIP - Bug 1480089 - pass all of the test files to to our static-analysis pipeline.

Nits:
- Remove one "WIP"
- Start commit message with a capital letter
- Improve commit message clarity, e.g. like so: "Run individual static-analysis tests all at once in order to cover analysis of multiple files." (or simply "Test './mach static-analysis check' on multiple files.")

Comment 6

[Jan Keromnes [:janx]]

(In reply to Jan Keromnes [:janx] from comment #4)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)
> > Jan do you think it's possible to skip 6.0 and move directly to 7.0, even
> > though 7.0 I don't think it's released yet?
> 
> 7.0 is indeed not yet released: https://llvm.org/svn/llvm-project/llvm/tags/
> 
> I'm not sure whether we can do such a thing. Maybe it's better for now to
> back-port the fix? (Note that bug 1466427 is upgrading 5.0.1 to 6.0.1, but
> that can happen independently of back-porting this fix.)

Actually, maybe we could use this: https://llvm.org/svn/llvm-project/llvm/branches/release_70/ ?

The file structure looks similar to what we use currently: https://llvm.org/svn/llvm-project/llvm/tags/RELEASE_501/final/

However, using a "branch" instead of a "tag" probably has major implications like "the code might change every now and then", which would probably mess up our hashes and cached binaries.

Comment 7

[Andi [:andi]]

Since clang 7 rc1 has been released we should try with that version, and for this there is a tag: https://llvm.org/svn/llvm-project/llvm/tags/RELEASE_700/rc1/

Comment 8

[Andi [:andi]]

One more thing that i want to mention, in release_70 we have also this checker: https://github.com/llvm-mirror/clang/blob/release_70/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp that would be nice to activate it for our analysis.

Comment 9

[Andi [:andi]]

Since we've updated to clang 7.0 rc2 i've finished this patch, let's see how it works on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0ea0ff137af4046d6e6da75a67da3a6865884f5

Comment 10

[Andi [:andi]]

Windows passed, let's see linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b24d212fd85a9ac467ab684d41cd03caa812239

Comment 11

[Andi [:andi]]

Attachment: Bug 1480089 - pass all of the test files to to our static-analysis pipeline. r=janx

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 9005158 [details]
Bug 1480089 - pass all of the test files to to our static-analysis pipeline. r=janx

Jan Keromnes [:janx] has approved the revision.

Comment 13

[Andi [:andi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17fc2af9fcbb4c94a05e7aca55a6df2d1611a801
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5bfb3a9a2e9da74b532f95ec8bc3ee76edea75f

Comment 14

[Andi [:andi]]

Everything works just fine, pushing it with lando!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7082bd230c3c
pass all of the test files to to our static-analysis pipeline. r=janx

Comment 16

[Sylvestre Ledru [:Sylvestre]]

bravo, much nicer!

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7082bd230c3c