Please update the tc job to clang-tidy version 7.0

RESOLVED FIXED in Firefox 63

Status

defect
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: sylvestre, Assigned: janx, Mentored)

Tracking

Trunk
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The final tag is available 
See bug 1462498 (not landed yet)
Assignee: nobody → janx
Mentor: sledru
bug 1462498 landed. we should update it now.

Moving from clang tidy 5.0  to 6.0 will require some renaming in tools/clang-tidy/config.yaml and the tests
Should be done here: https://dxr.mozilla.org/mozilla-central/source/build/build-clang/clang-tidy-linux64.json

Note: "clang-tidy-cxx14.patch" might no longer be needed (check if upstreamed)
Comment on attachment 8996751 [details]
Bug 1466427 - Enable two new clang-tidy 6.0 checks.

https://reviewboard.mozilla.org/r/260798/#review267974

::: tools/clang-tidy/config.yaml
(Diff revision 1)
>      publish: !!bool yes
>    - name: performance-for-range-copy
>      publish: !!bool yes
> -  # Only available from clang tidy 6.0. We are currently using 5.0
> -  # - name: performance-implicit-conversion-in-loop
> +  - name: performance-implicit-conversion-in-loop
> +    publish: !!bool yes
> -  #   publish: !!bool yes

please also generate tests for this. For this please use:
https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/clang-tidy/performance-implicit-conversion-in-loop.cpp

::: tools/clang-tidy/config.yaml:133
(Diff revision 1)
>    - name: readability-redundant-string-init
>      publish: !!bool yes
>    - name: readability-uniqueptr-delete-release
>      publish: !!bool yes
> -# Only available from clang tidy 6.0. We are currently using 5.0
> -# - name: readability-static-accessed-through-instance
> +  - name: readability-static-accessed-through-instance
> +    publish: !!bool yes

For this as a test use: 
https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/clang-tidy/readability-static-accessed-through-instance.cpp
Attachment #8996751 - Flags: review?(bpostelnicu)
Comment on attachment 8996750 [details]
Bug 1466427 - Upgrade clang-tidy from version 5.0 to 6.0 in Taskcluster.

https://reviewboard.mozilla.org/r/260796/#review267976

::: tools/clang-tidy/config.yaml:14
(Diff revision 1)
>    - win64
>    - win32
>  clang_checkers:
>    - name: -*
>      publish: !!bool no
> +  - name: bugprone-argument-comment

Please generate the tests for this, besides this everything looks good!
Also please do a try build to see that the artifcats itself build.

::: tools/clang-tidy/config.yaml
(Diff revision 1)
>      publish: !!bool yes
>    - name: clang-analyzer-unix.cstring.BadSizeArg
>      publish: !!bool yes
>    - name: clang-analyzer-unix.cstring.NullArg
>      publish: !!bool yes
> -  - name: misc-argument-comment

Please also remove the old tests, cpp and json in order to avoid confusion.
Attachment #8996750 - Flags: review?(bpostelnicu)
Thanks Andi for the reviews and helpful pointers! I'll update the tests accordingly.
I am totally failing to trigger "static-anlaysis-autotest-{linux64,win64}/debug" a.k.a. "Sa" on Try:

- With "-b d -p linux64,win64 -u st-autotest -t none": https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b96ed1b964b115addb6f0c3bf303ab1c9b1e985

- With "-b d -p linux64,win64 -u Sa -t none": https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b8c310cbb1e398447e0ca7b57aa3c515c0e6901

It seems to always want to run some SpiderMonkey build, and not the tests I want. Andi, could you please help?
Flags: needinfo?(bpostelnicu)
For me the easiest way to do it is to use: 

>>./mach try fuzzy  

And select the two targets: static-anlaysis-autotest-{linux64,win64}

Let's see if try is happy:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfbf86d58a69651226ed4f2c79a1a52cc00fd9e1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3b1658454b49c00e1ad3a5f1312f0040e33f001
Flags: needinfo?(bpostelnicu)
Comment on attachment 8996750 [details]
Bug 1466427 - Upgrade clang-tidy from version 5.0 to 6.0 in Taskcluster.

https://reviewboard.mozilla.org/r/260796/#review268076

::: build/build-clang/clang-tidy-linux64.json:2
(Diff revision 2)
>  {
>      "llvm_revision": "320871",

This should be: 335538 and same for all targets.

::: build/build-clang/clang-tidy-macosx64.json:24
(Diff revision 2)
>      "as": "/builds/worker/workspace/build/src/clang/bin/clang",
>      "ar": "/builds/worker/workspace/build/src/cctools/bin/x86_64-apple-darwin11-ar",
>      "ranlib": "/builds/worker/workspace/build/src/cctools/bin/x86_64-apple-darwin11-ranlib",
>      "ld": "/builds/worker/workspace/build/src/clang/bin/clang",
>      "patches": [
>        "llvm-debug-frame-for-5.patch",

we no longer need this.
Attachment #8996750 - Flags: review?(bpostelnicu) → review+
Comment on attachment 8996750 [details]
Bug 1466427 - Upgrade clang-tidy from version 5.0 to 6.0 in Taskcluster.

https://reviewboard.mozilla.org/r/260796/#review268078
Attachment #8996750 - Flags: review+
Comment on attachment 8996750 [details]
Bug 1466427 - Upgrade clang-tidy from version 5.0 to 6.0 in Taskcluster.

https://reviewboard.mozilla.org/r/260796/#review268084

r+ with those nits and ofcourse try should be green.
Attachment #8996750 - Flags: review+
Comment on attachment 8996751 [details]
Bug 1466427 - Enable two new clang-tidy 6.0 checks.

https://reviewboard.mozilla.org/r/260798/#review268194

I htink these are OK, but let's try the autotest first before submiting.
Attachment #8996751 - Flags: review?(bpostelnicu) → review+
Thanks for the review! And also for pushing to try for me (I can't run `./mach try fuzzy` on my setup).

It seems to have failed with:

```
[task 2018-08-02T17:07:41.456Z] -- Linker detection: GNU ld [task 2018-08-02T17:07:41.469Z] -- Linker detection: GNU ld [task 2018-08-02T17:07:41.478Z] -- Builtin supported architectures: i386;x86_64 [task 2018-08-02T17:07:41.579Z] -- Looking for sys/resource.h [task 2018-08-02T17:07:41.630Z] -- Looking for sys/resource.h - found [task 2018-08-02T17:07:41.643Z] -- Clang version: 6.0.0 [task 2018-08-02T17:07:41.645Z] -- Performing Test CXX_SUPPORTS_NO_NESTED_ANON_TYPES_FLAG [task 2018-08-02T17:07:41.664Z] -- Performing Test CXX_SUPPORTS_NO_NESTED_ANON_TYPES_FLAG - Failed [task 2018-08-02T17:07:41.856Z] CMake Error at tools/clang/tools/extra/clang-tidy/tool/CMakeLists.txt:14 (target_link_libraries): [task 2018-08-02T17:07:41.856Z] The keyword signature for target_link_libraries has already been used with [task 2018-08-02T17:07:41.856Z] the target "clang-tidy". All uses of target_link_libraries with a target [task 2018-08-02T17:07:41.856Z] must be either all-keyword or all-plain. [task 2018-08-02T17:07:41.856Z] [task 2018-08-02T17:07:41.856Z] The uses of the keyword signature are here: [task 2018-08-02T17:07:41.856Z] [task 2018-08-02T17:07:41.856Z] * cmake/modules/LLVM-Config.cmake:105 (target_link_libraries) [task 2018-08-02T17:07:41.856Z] * cmake/modules/AddLLVM.cmake:771 (target_link_libraries) [task 2018-08-02T17:07:41.856Z] [task 2018-08-02T17:07:41.856Z] [task 2018-08-02T17:07:41.856Z] [task 2018-08-02T17:07:51.871Z] -- Configuring incomplete, errors occurred! [task 2018-08-02T17:07:51.871Z] See also "/builds/worker/workspace/moz-toolchain/build/stage1/build/CMakeFiles/CMakeOutput.log". [task 2018-08-02T17:07:51.871Z] See also "/builds/worker/workspace/moz-toolchain/build/stage1/build/CMakeFiles/CMakeError.log". [task 2018-08-02T17:07:52.131Z] Traceback (most recent call last): [task 2018-08-02T17:07:52.131Z] File "./build-clang.py", line 595, in <module> [task 2018-08-02T17:07:52.132Z] build_type, assertions, python_path, gcc_dir, libcxx_include_dir) [task 2018-08-02T17:07:52.132Z] File "./build-clang.py", line 236, in build_one_stage [task 2018-08-02T17:07:52.132Z] build_package(build_dir, cmake_args) [task 2018-08-02T17:07:52.132Z] File "./build-clang.py", line 66, in build_package [task 2018-08-02T17:07:52.132Z] run_in(package_build_dir, ["cmake"] + cmake_args) [task 2018-08-02T17:07:52.132Z] File "./build-clang.py", line 42, in run_in [task 2018-08-02T17:07:52.132Z] check_run(args) [task 2018-08-02T17:07:52.132Z] File "./build-clang.py", line 35, in check_run [task 2018-08-02T17:07:52.132Z] assert r == 0 [task 2018-08-02T17:07:52.132Z] AssertionError [fetches 2018-08-02T17:07:52.163Z] removing /builds/worker/workspace/build [fetches 2018-08-02T17:07:52.683Z] finished [taskcluster 2018-08-02 17:07:53.225Z] === Task Finished === [taskcluster 2018-08-02 17:07:54.114Z] Unsuccessful task run with exit code: 1 completed in 261.302 seconds 
```
Maybe this was indeed caused by the bad llvm_revision.

I've addressed all nits, and sent a new Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=012b5d1d7d263f9ba8774f0dba8fd2624fa1307a
I used revision 338713, because I saw it on this page: https://llvm.org/svn/llvm-project/llvm/tags/RELEASE_601/final/

However, Andi correctly identified that this revision is actually the latest LLVM revision, not the RELEASE_601 revision (as you can see on https://llvm.org/svn/llvm-project/llvm/tags/RELEASE_501/final/).

The correct revision for RELEASE_601 should be 335538, as can be seen in: https://dxr.mozilla.org/mozilla-central/source/build/build-clang/clang-6-linux64.json

Hopefully using the correct revision will fix this Try error, but to be honest I don't really understand it:

> CMake Error at tools/clang/tools/extra/clang-tidy/tool/CMakeLists.txt:14 (target_link_libraries):
>   The keyword signature for target_link_libraries has already been used with
>   the target "clang-tidy".  All uses of target_link_libraries with a target
>   must be either all-keyword or all-plain.
> 
>   The uses of the keyword signature are here:
> 
>    * cmake/modules/LLVM-Config.cmake:105 (target_link_libraries)
Comment on attachment 8996750 [details]
Bug 1466427 - Upgrade clang-tidy from version 5.0 to 6.0 in Taskcluster.

https://reviewboard.mozilla.org/r/260796/#review268492

More comments in the next post, explaining why the build crahshed.
Attachment #8996750 - Flags: review+
Since we've determined what's the correct revision let's see why the build crashes.
Looking at the build log we get this error:

>>-- Clang version: 6.0.1
>>CMake Error at tools/clang/tools/extra/clang-tidy/tool/CMakeLists.txt:14 (target_link_libraries):
>>  The keyword signature for target_link_libraries has already been used with
>>  the target "clang-tidy".  All uses of target_link_libraries with a target
>>  must be either all-keyword or all-plain.

This error suggests that there is something wrong with the elements from the target_link_libraries, since we import our mozilla checkers, please see [1], we also need to modify the dependencies from CMakeLists. Looking at the respective CMakeLists file, [3], we see that the syntax changed a little bit from 5.0.1, see [4], and that we also specify the access modifier as PRIVATE, but in our code when we do the line matching we specify target_link_libraries. So the fix in this case would be to match against PRIVATE.

I will provide an example late on.

[1] https://dxr.mozilla.org/mozilla-central/source/build/build-clang/build-clang.py#521
[2] https://dxr.mozilla.org/mozilla-central/source/build/clang-plugin/import_mozilla_checks.py#115
[3] https://github.com/llvm-mirror/clang-tools-extra/blob/release_60/clang-tidy/tool/CMakeLists.txt#L15
[4] https://github.com/llvm-mirror/clang-tools-extra/blob/release_50/clang-tidy/tool/CMakeLists.txt#L11
Discussing with Andi and Sylvestre, we decided to skip 6.0.1 altogether and upgrade to 7.0.0-rc1 directly.
Summary: Please update the tc job to clang-tidy version 6.0 → Please update the tc job to clang-tidy version 7.0
An update here: compiler-rt from 7.0rc1 change a lot, and it needs a newer macOS sdk, currently we are targeting 10.11, and because of this the cross-build fails. But we don't need compiler-rt building for macOS we I've removed this dependency in the latest patch, try jobs can be found here: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b12584aaeaf85da60cd7d43281009607b51c48&selectedJob=195267637
https://treeherder.mozilla.org/#/jobs?repo=try&revision=434b819bf501757ba154dd5d053a3ee506553791&selectedJob=195273898
https://treeherder.mozilla.org/#/jobs?repo=try&revision=582d1fe7ee9bc6630dc41c73e7e67d822ac7a4da
Attachment #8996750 - Attachment is obsolete: true
Your try links are so green! Great job.

I've reviewed your patch in Phabricator. Once it's approved and landed, I'll rebase my second patch "Enable two new clang-tidy 6.0 checks" on top of it.
As per glandium we should update to 7.0rc2.
Comment on attachment 9003143 [details]
Bug 1466427 - Migrate clang-tidy package from 5.0.1 to 7.0.0-rc2. r=janx,r=glandium

Jan Keromnes [:janx] has approved the revision.
Attachment #9003143 - Flags: review+
Attachment #9003143 - Attachment description: Bug 1466427 - Migrate clang-tidy package from 5.0.1 to 7.0rc1. r=janx → Bug 1466427 - Migrate clang-tidy package from 5.0.1 to 7.0rc2. r=janx
Comment on attachment 9003143 [details]
Bug 1466427 - Migrate clang-tidy package from 5.0.1 to 7.0.0-rc2. r=janx,r=glandium

Mike Hommey [:glandium] has approved the revision.
Attachment #9003143 - Flags: review+
Attachment #9003143 - Attachment description: Bug 1466427 - Migrate clang-tidy package from 5.0.1 to 7.0rc2. r=janx → Bug 1466427 - Migrate clang-tidy package from 5.0.1 to 7.0.0-rc2. r=janx,r=glandium
Andi, my patch enabling `performance-implicit-conversion-in-loop` and `readability-static-accessed-through-instance` rebases fine on top of yours.

Additionally, I've noticed that clang-tidy-7 now has the following additional checkers:

- clang-analyzer-security.insecureAPI.bcmp
- clang-analyzer-security.insecureAPI.bcopy
- clang-analyzer-security.insecureAPI.bzero
- performance-inefficient-algorithm
- performance-move-const-arg
- performance-move-constructor-init
- performance-noexcept-move-constructor

Can I enable them as well? (Reminder: We used to enable `clang-analyzer-security.*` and `performance-*` before.)
Flags: needinfo?(bpostelnicu)
Attachment #8996751 - Attachment is obsolete: true
(In reply to Jan Keromnes [:janx] from comment #43)
> Created attachment 9003773 [details]
> Bug 1466427 - Enable two new clang-tidy 7.0 checks. r=andi

Please create a new bug for that. This bug is about moving to v7 ;)
Flags: needinfo?(janx)
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c61d20194e11
Migrate clang-tidy package from 5.0.1 to 7.0.0-rc2. r=glandium,janx
https://hg.mozilla.org/mozilla-central/rev/c61d20194e11
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(bpostelnicu)
Well done Andi!

(In reply to Sylvestre Ledru [:sylvestre] from comment #44)
> > Bug 1466427 - Enable two new clang-tidy 7.0 checks. r=andi
> 
> Please create a new bug for that. This bug is about moving to v7 ;)

Agreed, will do.
Flags: needinfo?(janx)
Blocks: 1486410
Attachment #9003773 - Attachment is obsolete: true
Thank you Eliza for catching the regression and backing out this patch! And sorry for the extra work.

I've made two mistakes here:
- my Try push didn't run on Windows (I'll make sure it does next time)
- the commit message has the wrong bug number in it (it should be bug 1486410)
Flags: needinfo?(janx)
You need to log in before you can comment on or make changes to this bug.