creduce script for clang crashes

[akhuang]

Summary

Add a script that calls C-Reduce on an input file and given the clang crash script, which is used to generate an interestingness test for C-Reduce.

[george.burgess.iv]

Thanks for this! Functionally, this looks good. My comments are mostly just simplicity/readability nitpicks.

clang/utils/creduce-clang-crash.py

36

nit: this isn't necessary; with doesn't introduce its own scope

37

nit , 'r' is implied; please remove

38

Hrm. Looks like there are cases where these crash reproducers are multiple lines, though the actually meaningful one (to us) is always the last one? If so, it may be cleaner to say cmd = f.readlines()[-1] here

43

nit: can replace with subprocess.check_output unless we explicitly want to ignore the return value (in which case, we should probably still call wait() anyway?)

49

please pipes.quote(llvm_not) and pipes.quote(cmd)

54

Why are we escaping the graves and single-quotes?

Also, nit: when possible with regex strings, please use raw strings. Doing so makes it way easier to reason about what Python's going to transform in the string literal before the regex engine gets to see it.

58

nit: please pipes.quote instead of adding manual quotes

62

nit: please simplify to del matches[:-5]

63

nit: pipes.quote please

76

nit: probably better to concatenate these; otherwise, I think we'll end up with a lot of spaces between these sentences? e.g.

help="The path to the llvm-not executable. "
     "Required if [...]")

108

I hate being *that* person, this technically isn't portable Python. I don't honestly know if we care about not-CPython being able to run our code, but the fix is to simply use with open(...) as f: instead of this one-liner.

(Specifically, CPython uses refcounts, so testfile will always be closed promptly, unless CPython decides someday to make files cyclic with themselves. GC'ed python implementations might take a while to flush this file and clean it up, which would be bad...)

113

Does creduce try and jump out into its own pgid? If not, I think this try/except block can be replaced with sys.exit(subprocess.call([creduce, testfile, file_to_reduce]))

120

nit: this is implied; please remove

[akhuang]

Addressed readability comments

[lebedev.ri]

The new diffs should not be relative to the previously uploaded diff, but to the git master/svn trunk.

[akhuang]

clang/utils/creduce-clang-crash.py

43

check_output() raises an error when the return code is nonzero, which it is in this case. I think communicate() calls wait(), though.

49

cmd is already quoted, since it's read out of another file

[joerg]

For it to be really useful for the majority of bugs, it would be nice to figure out automatically how to get the preprocessing step done and filter out the # lines afterwards. That part alone significantly cuts down the creduce time.

[akhuang]

fixed diff with style edits

[rnk]

@gbiv already got all my shell quoting comments.

I think we should do one more round of fixes, we can commit that for you, and then move on to the next steps.

clang/utils/creduce-clang-crash.py

109

We could try validating that the interestingness test passes here. If it doesn't, that's a bug in this script, I suppose.

CReduce already does this for the user, but it's not clear with our usage model how to do this.

110

Let's add a TODO (or FIXME, that's more LLVM-y) here to add a step that runs the full pre-processor with -E and -P here. As we've discussed, it often doesn't work, but when it does, it avoids all those issues with #defines, comments, etc, breaking up topformflat. This doesn't have to be in the first version, of course.

I see @joerg added a comment about this as well.

[george.burgess.iv]

I think we should do one more round of fixes, we can commit that for you, and then move on to the next steps

+1. This looks good to me with outstanding nits fixed

and filter out the # lines afterwards.

AIUI, the very-recently-merged https://github.com/csmith-project/creduce/pull/183 and https://github.com/csmith-project/creduce/pull/188 should hopefully handle the majority of those?

clang/utils/creduce-clang-crash.py

1

nit: please specify a python version here, since the world is steadily making python == python3 (if pipes.quote is working, I assume you'll want #!/usr/bin/env python2)

[rnk]

clang/utils/creduce-clang-crash.py

1

Please don't do that, there is no python2.exe or python3.exe on Windows. #!/usr/bin/env python is the simplest thing that could work.

[akhuang]

add interestingness test sanity check;
revive ctrl-c hack

[thakis]

(A few python style comments; feel free to ignore, and feel free to land no matter what you do with the comments.)

clang/utils/creduce-clang-crash.py

1

There's no python2 on mac either. #!/usr/bin/env python is the correct first line for executable python scripts.

29

There's distutils.spawn.find_executable() which does this for you.

68

It's a somewhat common pattern to put a module docstring at the top of the file instead of a comment which explains usage, and then say description=__doc__ here; see llvm/utils/gn/build/symlink_or_copy.py for an example.

90

It's a bit more common to say return 1 here and do sys.exit(main()) at the bottom.

[akhuang]

style edits

[george.burgess.iv]

I think that addresses all of the concerns people have put forward; given rnk's comment about one more round of fixes, this LGTM. Will check this in for you shortly.

Thanks again!

clang/utils/creduce-clang-crash.py

1

TIL. Thank you both for the counterexamples :)

[arichardson]

In case anyone is interested, for the CHERI fork of LLVM/Clang I added a similar script that contains additional features such as inferring the crash message (so that you get the minimal reproducer for the issue that you are trying to reduce and not some obscure parser crash for invalid input), dealing with infinite loops, generating a test case with a mostly sensible RUN: line.
Furthermore, it will attempt to use bugpoint if it is a backend rather than a frontend crash since that is much much faster than creduce.
It should allows you to run $LLVM_BUILDIR/bin/creduce_crash_testcase.py /tmp/clang-reproducers.sh and then give a sensible minimal test case back

I can try to clean that up and remove the CHERI-specific compiler flags and llvm-lit substitutions.

For reference that script can be found here: https://github.com/CTSRD-CHERI/llvm-project/blob/master/clang/utils/creduce_crash_testcase.py

clang/utils/creduce-clang-crash.py

 #!/usr/bin/env python
 # A tool that calls C-Reduce to create a minimal reproducer for clang crashes
 # Requires C-Reduce and not (part of LLVM utils) to be installed
 
 from argparse import ArgumentParser
 import os
 import re
 import stat
 import sys
 import subprocess
+import pipes
 
 def is_exe(fpath):
   return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 def which(program):
   """
   Return the full path to a program or return None.
   """
   fpath, fname = os.path.split(program)
   if fpath:
     if is_exe(program):
       return program
   else:
     for path in os.environ["PATH"].split(os.pathsep):
       exe_file = os.path.join(path, program)
       if is_exe(exe_file):
         return exe_file
   return None
 
 def create_test(build_script, llvm_not):
   """
   Create an interestingness test from the crash output.
   Return as a string.
   """
   # Get clang call from build script
-  cmd = None
-  with open(build_script, 'r') as f:
-    cmd = f.read()
-    cmd = re.sub("#!.*\n", "", cmd)
-    cmd = cmd.rstrip('\n\r')
+  # Assumes the call is the last line of the script
+  with open(build_script) as f:
+    cmd = f.readlines()[-1].rstrip('\n\r')
 
   # Get crash output
   p = subprocess.Popen(build_script,
                        stdout=subprocess.PIPE,
                        stderr=subprocess.STDOUT)
   crash_output, _ = p.communicate()
 
   output = ['#!/bin/bash']
-  output.append('%s --crash %s >& t.log || exit 1' % (llvm_not, cmd))
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
+                                                      cmd))
 
   # Add messages from crash output to the test
   # If there is an Assertion failure, use that; otherwise use the
   # last five stack trace functions
-  assertion_re = "Assertion \`([^\']+)\' failed"
+  assertion_re = r'Assertion `([^\']+)\' failed'
   assertion_match = re.search(assertion_re, crash_output)
   if assertion_match:
-    msg = assertion_match.group(1).replace('"', '\\"')
-    output.append('grep "%s" t.log || exit 1' % msg)
+    msg = assertion_match.group(1)
+    output.append('grep %s t.log || exit 1' % pipes.quote(msg))
   else:
-    stacktrace_re = "#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\("
+    stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
     matches = re.findall(stacktrace_re, crash_output)
-    del matches[:len(matches)-5]
-    output += ['grep "%s" t.log || exit 1' % msg for msg in matches]
+    del matches[:-5]
+    output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
 
   return output
 
 def main():
   parser = ArgumentParser(description='Runs C-Reduce on the input file')
   parser.add_argument('build_script', type=str, nargs=1,
                       help='Name of the script that generates the crash.')
   parser.add_argument('file_to_reduce', type=str, nargs=1,
                       help='Name of the file to be reduced.')
   parser.add_argument('-o', '--output', dest='output', type=str,
                       help='Name of the output file for the reduction. Optional.')
   parser.add_argument('--llvm-not', dest='llvm_not', type=str,
-                      help="""The path to the llvm-not executable.
-                      Required if 'not' is not in PATH environment.""");
+                      help="The path to the llvm-not executable. "
+                      "Required if 'not' is not in PATH environment.");
   parser.add_argument('--creduce', dest='creduce', type=str,
-                      help="""The path to the C-Reduce executable.
-                      Required if 'creduce' is not in PATH environment.""");
+                      help="The path to the C-Reduce executable. "
+                      "Required if 'creduce' is not in PATH environment.");
   args = parser.parse_args()
 
   build_script = os.path.abspath(args.build_script[0])
   file_to_reduce = os.path.abspath(args.file_to_reduce[0])
   llvm_not = which(args.llvm_not) if args.llvm_not else which('not')
   creduce = which(args.creduce) if args.creduce else which('creduce')
 
   if not os.path.isfile(build_script):
     print(("ERROR: input file '%s' does not exist") % build_script)
     sys.exit(1)
 
   if not os.path.isfile(file_to_reduce):
     print(("ERROR: input file '%s' does not exist") % file_to_reduce)
     sys.exit(1)
 
   if not llvm_not:
     parser.print_help()
     sys.exit(1)
 
   if not creduce:
     parser.print_help()
     sys.exit(1)
 
   # Write interestingness test to file
   test_contents = create_test(build_script, llvm_not)
   testname, _ = os.path.splitext(file_to_reduce)
   testfile = testname + '.test.sh'
-  open(testfile, 'w').write('\n'.join(test_contents))
+  with open(testfile, 'w') as f:
+    f.write('\n'.join(test_contents))
   os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
 
-  # Call C-Reduce
-  try:
-    p = subprocess.Popen([creduce, testfile, file_to_reduce])
-    p.communicate()
-
-  except KeyboardInterrupt:
-    print('\n\nctrl-c detected, killed creduce')
-    p.kill()
-
-  sys.exit(0)
+  sys.exit(subprocess.call([creduce, testfile, file_to_reduce]))
 
 if __name__ == '__main__':
   main()