[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

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



================
Comment at: clang/utils/creduce-clang-crash.py:36
+  # Get clang call from build script
+  cmd = None
+  with open(build_script, 'r') as f:
----------------
nit: this isn't necessary; `with` doesn't introduce its own scope


================
Comment at: clang/utils/creduce-clang-crash.py:37
+  cmd = None
+  with open(build_script, 'r') as f:
+    cmd = f.read()
----------------
nit `, 'r'` is implied; please remove


================
Comment at: clang/utils/creduce-clang-crash.py:38
+  with open(build_script, 'r') as f:
+    cmd = f.read()
+    cmd = re.sub("#!.*\n", "", cmd)
----------------
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


================
Comment at: clang/utils/creduce-clang-crash.py:43
+  # Get crash output
+  p = subprocess.Popen(build_script,
+                       stdout=subprocess.PIPE,
----------------
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?)


================
Comment at: clang/utils/creduce-clang-crash.py:49
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (llvm_not, cmd))
+
----------------
please `pipes.quote(llvm_not)` and `pipes.quote(cmd)`


================
Comment at: clang/utils/creduce-clang-crash.py:54
+  # last five stack trace functions
+  assertion_re = "Assertion \`([^\']+)\' failed"
+  assertion_match = re.search(assertion_re, crash_output)
----------------
Why are we escaping the graves and single-quotes?

Also, nit: when possible with regex strings, please use [raw strings](https://stackoverflow.com/questions/2081640/what-exactly-do-u-and-r-string-flags-do-and-what-are-raw-string-literals). 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.


================
Comment at: clang/utils/creduce-clang-crash.py:58
+    msg = assertion_match.group(1).replace('"', '\\"')
+    output.append('grep "%s" t.log || exit 1' % msg)
+  else:
----------------
nit: please `pipes.quote` instead of adding manual quotes


================
Comment at: clang/utils/creduce-clang-crash.py:62
+    matches = re.findall(stacktrace_re, crash_output)
+    del matches[:len(matches)-5]
+    output += ['grep "%s" t.log || exit 1' % msg for msg in matches]
----------------
nit: please simplify to `del matches[:-5]`


================
Comment at: clang/utils/creduce-clang-crash.py:63
+    del matches[:len(matches)-5]
+    output += ['grep "%s" t.log || exit 1' % msg for msg in matches]
+
----------------
nit: `pipes.quote` please


================
Comment at: clang/utils/creduce-clang-crash.py:76
+  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.""");
----------------
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 [...]")
```


================
Comment at: clang/utils/creduce-clang-crash.py:108
+  testfile = testname + '.test.sh'
+  open(testfile, 'w').write('\n'.join(test_contents))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
----------------
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...)


================
Comment at: clang/utils/creduce-clang-crash.py:113
+  try:
+    p = subprocess.Popen([creduce, testfile, file_to_reduce])
+    p.communicate()
----------------
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]))`


================
Comment at: clang/utils/creduce-clang-crash.py:120
+
+  sys.exit(0)
+
----------------
nit: this is implied; please remove


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59118/new/

https://reviews.llvm.org/D59118