[clang-tidy] run-clang-tidy add synchronisation to the output

[Abpostelnicu]

Summary

The goal of this patch is to add synchronisation of the output of the tool with the actual files that are verified. Without it the output of clang-tidy will get mixed with the clang-tidy command that is also passed to stdout.

[JonasToth]

I would like to have this patch in, because i currently have a 1MB output file, and the missing synchronization really destroys cleaning with grep and sed :(

clang-tidy/tool/run-clang-tidy.py

167 ↗

(On Diff #157473)

Please do the comparison != 0 to be more clear about the expected success return value.

171 ↗

(On Diff #157473)

Please add a comparison you are looking for (> 0 i guess)

[JonasToth]

I forgot: Did you measure how much of a time penalty this brings? Just out of curiosity, correct script is more important then fast script :)

[Abpostelnicu]

Regarding the time penalty it depends very much on how many files you have. But I would choose anytime to have this in the detriment of having "obfuscated" output.

[JonasToth]

Lets say running it over whole LLVM for one check? I will apply this path locally, because i have to analyze llvm right now anyway.

For me this looks good, but @alexfh or @aaron.ballman should accept it.

[JonasToth]

I do run it over llvm/lib, there is no visible effect. from my experience maybe 1-5% of the lines are interleafed without the patch, too.

[Abpostelnicu]

Did you notice any significant speed degradation?

[JonasToth]

No nothing. I think it is barely noticable in general. From prior long
runs i notices the scrambled messages, but it was only a small fraction.
So most of the time the threads dont seem to interfere, which makes the
lock kinda low-cost.

Am 07.08.2018 um 19:17 schrieb Andi via Phabricator:

Abpostelnicu added a comment.

Did you notice any significant speed degradation?

https://reviews.llvm.org/D49851

[JonasToth]

@alexfh, @aaron.ballman or @hokein could you take a final look on this one?

[alexfh]

LG

[R2RT]

Hey, all, as for me it seems that this commit introduced a few problems with Python3 (at least 3.7)

sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
TypeError: can only concatenate str (not "bytes") to str

Then

    if err > 0:
TypeError: '>' not supported between instances of 'bytes' and 'int'

Then

sys.stderr.write(err + '\n')
TypeError: can't concat str to bytes

After fixes:

with lock:
  sys.stdout.write(' '.join(invocation) + '\n' + str(output) + '\n')
  if err:
    sys.stderr.write(str(err) + '\n')
queue.task_done()

How about using logging module to get synchronization working out of hand?

[Abpostelnicu]

Strangely I haven't had those kind of issues but since you propose those modifications I would do one more modification, let's output everything to stdout and not stderr by doing:

if err:
  sys.stdout.write(str(err) + '\n')

[JonasToth]

In D49851#1202764, @Abpostelnicu wrote:

Strangely I haven't had those kind of issues but since you propose those modifications I would do one more modification, let's output everything to stdout and not stderr by doing:

if err:
  sys.stdout.write(str(err) + '\n')

You can make this a new revision, fixing the byte and str issues would be more important now.

The byte and str thingie is since the whole python3 releases or did it change?

[Abpostelnicu]

In D49851#1203676, @JonasToth wrote:

In D49851#1202764, @Abpostelnicu wrote:

Strangely I haven't had those kind of issues but since you propose those modifications I would do one more modification, let's output everything to stdout and not stderr by doing:

if err:
  sys.stdout.write(str(err) + '\n')

You can make this a new revision, fixing the byte and str issues would be more important now.

The byte and str thingie is since the whole python3 releases or did it change?

I'm not sure this is the fix for this, I think we should specify the encoding type when building the string from the byte like:

if err:
  sys.stdout.write(str(err, 'utf-8') + '\n')

And of course this must be done only on python 3+ since on python 2 the str ctor doesn't accept the encoding parameter.

[JonasToth]

Please dont work in this revision but create a new one (as this one is
already committed)!

Am 24.08.2018 um 17:34 schrieb Andi via Phabricator:

Abpostelnicu updated this revision to Diff 162387.

https://reviews.llvm.org/D49851

Files:

clang-tidy/tool/run-clang-tidy.py

Index: clang-tidy/tool/run-clang-tidy.py

• clang-tidy/tool/run-clang-tidy.py

+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,10 +166,18 @@

output, err = proc.communicate()
if proc.returncode != 0:
  failed_files.append(name)

+
+ if is_py2:
+ output_string = output
+ err_string = err
+ else:
+ output_string = str(output, 'utf-8')
+ err_string = str(err, 'utf-8')
+

with lock:

• sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
• if err > 0:
• sys.stderr.write(err + '\n')

+ sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+ if err:
+ sys.stderr.write(err_string + '\n')

queue.task_done()

clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py

def apply_fixes(args, tmpdir):
   if args.format:
     invocation.append('-format')
   if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
     name = queue.get()
     invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
                                      tmpdir, build_path, args.header_filter,
                                      args.extra_arg, args.extra_arg_before,
                                      args.quiet, args.config)
-    sys.stdout.write(' '.join(invocation) + '\n')
-    return_code = subprocess.call(invocation)
-    if return_code != 0:
+
+    proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    output, err = proc.communicate()
+    if proc.returncode != 0:
       failed_files.append(name)
+    with lock:
+      sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+      if err > 0:
+        sys.stderr.write(err + '\n')
     queue.task_done()
 
 
 def main():
   parser = argparse.ArgumentParser(description='Runs clang-tidy over all files '
                                    'in a compilation database. Requires '
                                    'clang-tidy and clang-apply-replacements in '
                                    '$PATH.')
def main():
   file_name_re = re.compile('|'.join(args.files))
 
   return_code = 0
   try:
     # Spin up a bunch of tidy-launching threads.
     task_queue = queue.Queue(max_task)
     # List of files with a non-zero return code.
     failed_files = []
+    lock = threading.Lock()
     for _ in range(max_task):
       t = threading.Thread(target=run_tidy,
-                           args=(args, tmpdir, build_path, task_queue, failed_files))
+                           args=(args, tmpdir, build_path, task_queue, lock, failed_files))
       t.daemon = True
       t.start()
 
     # Fill the queue with files.
     for name in files:
       if file_name_re.search(name):
         task_queue.put(name)