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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
