This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Parallelize clang-tidy-diff.py
ClosedPublic

Authored by zinovy.nis on Feb 3 2019, 11:29 AM.

Diff Detail

Event Timeline

zinovy.nis created this revision.Feb 3 2019, 11:29 AM

Fixed minor typos.

Just a question.. If clang tidy is running with -fix in parallel, what stops each clang-tidy invocation altering a common header at the same time?

Just a question.. If clang tidy is running with -fix in parallel, what stops each clang-tidy invocation altering a common header at the same time?

You are right. May be it worth disabling -fix for j != 1.

You are right. May be it worth disabling -fix for j != 1.

I only say this because I think I might have seen it happen when I was running run-clang-tidy.py over a large code base with a fairly aggressive check/fixit, but frankly I was too new to LLVM to know it wasn't something I might have done wrong.

I saw my '[[nodiscard]]' checker splicing multiple [[nodiscard]]s onto the same line, I suspect there is a time of check/time of use issue for the locations, sometimes the positions where wrong by the time the Fixit came around. I guess because i'd used that script without specifiy a -j1 it just used the default which is -j0 which I guess goes as parallel as it can.

Its only when you raised this review for the diff check that I wondered if that could be the cause.

zinovy.nis added a comment.EditedFeb 11 2019, 3:25 AM

I only say this because I think I might have seen it happen when I was running run-clang-tidy.py over a large code base with a fairly aggressive check/fixit, but frankly I was too new to LLVM to know it wasn't something I might have done wrong.

A bit strange as run-clang-tidy.py explicitly collects fixes in separate files and after all merges them into a single fix in a thread/process-safe manner. Not like my current patch.

-j and -export-fixes were made mutually exclusive.

Just a question.. If clang tidy is running with -fix in parallel, what stops each clang-tidy invocation altering a common header at the same time?

clang-apply-replacements does it. The fixes are collected and then deduplicated and applied. If there are conflicts between the fixes at this stage they will be signaled.

JonasToth added inline comments.Feb 14 2019, 12:14 AM
clang-tidy/tool/clang-tidy-diff.py
51

if timeout is not None is more readable.

58

the command could be utf-8, too not?
If for example file-names use special characters they should be decoded as well. This is probably not done properly in the run-clang-tidy.py script, but can be adressed separatly.

76

why daemonize? Who kills that thread in the end? I think it's not the best choice.

131

I would prefer going the clang-apply-replacements route here as well.
It feels weird if there is an inconsistency between these parallel runners.

zinovy.nis marked an inline comment as done.Feb 14 2019, 1:41 AM
zinovy.nis added inline comments.
clang-tidy/tool/clang-tidy-diff.py
76

The script can be interrupted with keyabord or other signal, so it need not wait for any worker thread.

alexfh added inline comments.Feb 15 2019, 6:22 AM
clang-tidy/tool/clang-tidy-diff.py
98

The "clang-tidy runs are independent" assumption is unfortunately not valid for our internal compilation database integration, so making -j0 the default will break it. Let's make it 1.

zinovy.nis marked an inline comment as done.Feb 15 2019, 6:39 AM
zinovy.nis added inline comments.
clang-tidy/tool/clang-tidy-diff.py
98

Do you mean you run it with -fix or like this?

alexfh added inline comments.Feb 15 2019, 3:16 PM
clang-tidy/tool/clang-tidy-diff.py
98

Nope, our CompilationDatabase implementation doesn't support concurrent use (regardless of whether it's the same process or different ones).

  • -j is 1 by default;
  • fixed minor remarks;
zinovy.nis marked 4 inline comments as done.Feb 19 2019, 11:44 AM

A few more comments.

clang-tidy/tool/clang-tidy-diff.py
58

nit: let's wrap this to 80 characters.

133

What's wrong with -export-fixes and -j? Different clang-tidy processes could export the fixes to different files (e.g. the -export-fixes= value could be used as a prefix of the filename with a sequential or random suffix. WDYT?

179

"start_workers" would be a clearer name, IMO.

zinovy.nis marked an inline comment as done.Feb 22 2019, 12:17 AM
zinovy.nis added inline comments.
clang-tidy/tool/clang-tidy-diff.py
133

I can copy/paste (shame on me!) a code from run-clang-tidy.py that does just what we need.

zinovy.nis retitled this revision from [clang-tidy] Parallelise clang-tidy-diff.py to [clang-tidy] Parallelize clang-tidy-diff.py .Mar 6 2019, 9:51 AM
  • honest '--export-fixes': multiple yamls are merged into the resulting one.
  • minor cosmetic fixes.

One more gentle ping.

alexfh accepted this revision.Mar 19 2019, 9:15 AM

LG with a nit

clang-tidy/tool/clang-tidy-diff.py
88

nit: Spaces around operators, please

This revision is now accepted and ready to land.Mar 19 2019, 9:15 AM
This revision was automatically updated to reflect the committed changes.
derek added a subscriber: derek.Dec 27 2019, 1:28 PM

@zinovy.nis @alexfh @JonasToth @MyDeveloperDay

Hi folks, please correct me if I'm wrong but it appears that an effect of this change is that this script will no longer exit non-zero if clang-tidy discovers any errors, which was the previous functionality with sys.exit(subprocess.call(' '.join(command), shell=True)). As a result, when we upgraded a project to LLVM 9 our CI began having false-positives, as we relied on the exit code of this script to indicate success/failure. Would it be possible to restore that functionality? I'd be happy to provide a patch.

@zinovy.nis @alexfh @JonasToth @MyDeveloperDay

I'd be happy to provide a patch.

It would be nice, thanks!

@zinovy.nis @alexfh @JonasToth @MyDeveloperDay

Hi folks, please correct me if I'm wrong but it appears that an effect of this change is that this script will no longer exit non-zero if clang-tidy discovers any errors, which was the previous functionality with sys.exit(subprocess.call(' '.join(command), shell=True)). As a result, when we upgraded a project to LLVM 9 our CI began having false-positives, as we relied on the exit code of this script to indicate success/failure. Would it be possible to restore that functionality? I'd be happy to provide a patch.

Yeah, that would be good to restore. run-clang-tidy.py is parallelized and does fail properly (95% sure about that xD), so in doubt you can check that script as well.