Closed Bug 1478644 Opened Last year Closed 7 months ago

[Clang-Tidy] Synchronize run-clang-tidy output with the actual files that are being checked

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In order to perform static-analysis on Gecko we use the clang-tidy infrastructure, for this we leverage a tool run-clang-tidy, that spawns multiple process in order for the analysis process to be faster. Unfortunately run-clang-tidy doesn't uses a write synchronization when it comes to dump output in the stdout of stderr, because of this many time we see that the outputs is interlaced with results from different files, thus making it less readable to human and also to machine. 

In order to fix this issue upstream we've opened this issue: /Users/abpostelnicu/Projects/llvm/llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py
If that change gets accepted most likely it will land in 7.0 and we are currently a little bit behind with clang-tidy, at 5.0.1, thus we need an in-tree patch for our tool-chain build.
Assignee: nobody → bpostelnicu
Blocks: 1472975
Blocks: 1480089
No longer blocks: 1472975
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #0)
> In order to fix this issue upstream we've opened this issue: /Users/abpostelnicu/Projects/llvm/llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py

Oops, it looks like you pasted the wrong buffer. Could you please add the correct link for the upstream issue in the "See also" field of this bug?

I tried to find it myself, but unsuccessfully: https://bugs.llvm.org/buglist.cgi?quicksearch=run-clang-tidy
Flags: needinfo?(bpostelnicu)
Ups, here it goes: https://reviews.llvm.org/D49851
Flags: needinfo?(bpostelnicu)
Nevermind, I found it: https://reviews.llvm.org/D49851 (by googling `"run-clang-tidy.py" "with lock"`)
Comment on attachment 8995186 [details]
Bug 1478644 - [Clang-Tidy] Synchronize run-clang-tidy output with the actual files that are being checked.

https://reviewboard.mozilla.org/r/259678/#review267746

Thank you for back-porting this patch! I don't see any issue with it, but is it possible to wait for https://reviews.llvm.org/D49851 to be merged before we back-port it into our tree? I'm worried that it might change (e.g. if the reviewers favour a different approach), and also it doesn't have a test to guarantee that the patch definitely fixes the bug without regressing existing functionality.
Attachment #8995186 - Flags: review?(janx)
I see your point, but the tests that are in place in clang-tools-extra already cover this patch, and without it we cannot have bug 1480089
Since we don't know how long it will take until upstream will be reviewed and even so it will be integrated into trunk, we should have this in our code base. More than this, the precedence already happened when we use different patches that affect our clang-tidy build, take for example the patch for macos64, that eliminates the need of code sign.
Flags: needinfo?(janx)
Since Jan is in PTO and this revision has been accepted by llvm https://reviews.llvm.org/D49851 maybe someone else can review it.
Flags: needinfo?(janx)
Attachment #8995186 - Flags: review?(nfroyd)
Comment on attachment 8995186 [details]
Bug 1478644 - [Clang-Tidy] Synchronize run-clang-tidy output with the actual files that are being checked.

https://reviewboard.mozilla.org/r/259678/#review269334

Stealing the review. If it landed upstream, we should indeed cherry-pick it.
Attachment #8995186 - Flags: review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc15796e6fa7
[Clang-Tidy] Synchronize run-clang-tidy output with the actual files that are being checked. r=sylvestre
Comment on attachment 8995186 [details]
Bug 1478644 - [Clang-Tidy] Synchronize run-clang-tidy output with the actual files that are being checked.

Canceling review, since Sylvestre took care of this.
Attachment #8995186 - Flags: review?(nfroyd)

This should be closed since now it's part of clang 8 and we are moving to that version.

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.