Page MenuHomePhabricator

Additions to creduce script
ClosedPublic

Authored by akhuang on Mar 22 2019, 3:43 PM.

Details

Summary

Some more additions to the script - mainly reducing the clang args after the creduce run by removing them one by one and seeing if the crash reproduces. Other things:

  • remove the --crash flag when "fatal error" occurs
  • fixed to read stack trace functions from the top
  • run creduce on a copy of the original file

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
akhuang updated this revision to Diff 192145.Mar 25 2019, 10:41 AM

Fix some typos, pass --tidy flag to creduce

akhuang updated this revision to Diff 192149.Mar 25 2019, 10:46 AM

reuploaded diff with full context

Thanks for this!

clang/utils/creduce-clang-crash.py
137 ↗(On Diff #192149)

Please expand a bit on why 5 was chosen (is there some deep reason behind it, or does it just seem like a sensible number?)

145 ↗(On Diff #192149)

nit: please prefer [x for x in matches if x and x.strip() not in filters][:5]. py3's filter returns a generator, whereas py2's returns a list.

193 ↗(On Diff #192149)

I think _ is an open file descriptor; please os.close it if we don't want to use it

199 ↗(On Diff #192149)

Same nit

202 ↗(On Diff #192149)

was this intended to use cmd?

205 ↗(On Diff #192149)

Do we want to check the exit code of this? Or do we assume that if clang crashes during preprocessing, we'll just see a different error during check_expected_output? (In the latter case, please add a comment)

222 ↗(On Diff #192149)

Is it intentional to group multiple consecutive non-dashed args? e.g. it seems that clang -ffoo bar baz will turn into ['clang', '-ffoo bar baz']

223 ↗(On Diff #192149)

Should we be shlex.quote'ing y here (and probably in the return x + [y] below)?

251 ↗(On Diff #192149)

IMO, if even extra_arg is in new_args, we should still move it near the end. Arg ordering matters in clang, generally with later args taking precedence over earlier ones. e.g. the -g$N args in https://godbolt.org/z/Mua8cs

279 ↗(On Diff #192149)

If we're replacing other args with their effective negation, does it also make sense to replace all debug-ish options with -g0?

296 ↗(On Diff #192149)

Might not want to have a space here; -DFOO=1 is valid (same for -I below)

306 ↗(On Diff #192149)

Probably want to do a similar thing for -Xclang (which, as far as this code is concerned, acts a lot like -mllvm)

362 ↗(On Diff #192149)

I'm unclear on why we do a partial simplification of clang args here, then a full reduction after creduce. Is there a disadvantage to instead doing:

r.write_interestingness_test()
r.clang_preprocess()
r.reduce_clang_args()
r.run_creduce()
r.reduce_clang_args()

?

The final reduce_clang_args being there to remove extra -D/-I/etc. args if preprocessing failed somehow, to remove -std=foo args if those aren't relevant after reduction, etc.

The advantage to this being that we no longer need to maintain a simplify function, and creduce runs with a relatively minimal set of args to start with.

In any case, can we please add comments explaining why we chose whatever order we end up going with, especially where subtleties make it counter to what someone might naively expect?

akhuang marked 15 inline comments as done.Mar 25 2019, 2:59 PM
akhuang added inline comments.
clang/utils/creduce-clang-crash.py
137 ↗(On Diff #192149)

There is no deep reason - it was an arbitrary smallish number to hopefully not only pick up common stack trace functions

202 ↗(On Diff #192149)

yep

205 ↗(On Diff #192149)

I think checking the exit code is a good idea

222 ↗(On Diff #192149)

I guess that was originally the intention, although now that I think of it it makes more sense to group at most one argument.

223 ↗(On Diff #192149)

It quotes everything right before writing to file - are there reasons to quote here instead?

279 ↗(On Diff #192149)

I guess -g0 is not a cc1 option, nor is -gdwarf? So this is essentially just removing -gcodeview. I'm actually not sure what the other flags do.

362 ↗(On Diff #192149)

Basically the disadvantage is that clang takes longer to run before the reduction, so it takes unnecessary time to iterate through all the arguments beforehand.
And yeah, the final reduce_clang_args is there to minimize the clang arguments needed to reproduce the crash on the reduced source file.

If that makes sense, I can add comments for this-

rnk added a comment.Mar 25 2019, 3:37 PM

I recently discovered NamedTemporaryFile, maybe that would help simplify up the various mkstemp usages.

akhuang updated this revision to Diff 192218.Mar 25 2019, 3:44 PM
akhuang marked an inline comment as done.

Style nits, added comments

george.burgess.iv marked an inline comment as done.Mar 25 2019, 4:15 PM
george.burgess.iv added inline comments.
clang/utils/creduce-clang-crash.py
303 ↗(On Diff #192218)

FWIW, opportunistically trying to add -fsyntax-only may help here: if the crash is in the frontend, it means that non-repros will stop before codegen, rather than trying to generate object files, or whatever they were trying to generate in the first place.

223 ↗(On Diff #192149)

We're shlex.spliting groups below, and I assume the intent is Reduce.ungroup_args(Reduce.group_args_by_dash(args)) == args.

If we don't want to quote here, we can also have ungroup_args and group_args_by_dash deal in lists of nonempty lists.

279 ↗(On Diff #192149)

Ah, I didn't realize this was dealing with cc1 args. My mistake.

I'm not immediately sure either, but grepping through the code, it looks like -debug-info-kind= may be the main interesting one to us here.

306 ↗(On Diff #192149)

(You can ignore this comment if we're dealing in cc1; -Xclang is just "pass this directly as a cc1 arg")

362 ↗(On Diff #192149)

Eh, I don't have a strong opinion here. My inclination is to prefer a simpler reduction script if the difference is len(args) clang invocations, since I assume creduce is going to invoke clang way more than len(args) times. I see a docstring detailing that simplify is only meant to speed things up now, so I'm content with the way things are.

akhuang marked 2 inline comments as done.Mar 25 2019, 5:47 PM
akhuang added inline comments.
clang/utils/creduce-clang-crash.py
223 ↗(On Diff #192149)

good point- I guess the whole grouping thing is unnecessarily complicated, so I got rid of it and it now removes the next arg in try_remove_arg_by_index

306 ↗(On Diff #192149)

ah, ok.

akhuang updated this revision to Diff 192230.Mar 25 2019, 5:48 PM

fix issue with grouping two command line args together

arichardson added inline comments.Mar 26 2019, 2:26 AM
clang/utils/creduce-clang-crash.py
303 ↗(On Diff #192218)

Yes that sounds like a good idea! I just do -emit-llvm to avoid assembly output but for parser/sema crashes -fsyntax-only would save some time.

Another one I found useful was -Werror=implicit-int to get more readable test cases from creduce: https://github.com/CTSRD-CHERI/llvm-project/blob/master/clang/utils/creduce_crash_testcase.py#L851

Without that flag lots of test cases look really weird due to the implicit int and various inferred semicolons.

145 ↗(On Diff #192149)

Stack traces also look different on macOS and it would be nice to handle that too.

Here's a sample I got from adding a llvm_unreachable at a random location:

My unreachable message...
UNREACHABLE executed at /Users/alex/cheri/llvm-project/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4468!
Stack dump:
0.	Program arguments: /Users/alex/cheri/llvm-project/cmake-build-debug/bin/opt -mtriple=cheri-unknown-freebsd -mcpu=cheri128 -mattr=+cheri128 -target-abi purecap -relocation-model pic -S -instcombine -simplifycfg /Users/alex/cheri/llvm-project/llvm/test/CodeGen/Mips/cheri/simplifycfg-ptrtoint.ll -o - 
1.	Running pass 'Function Pass Manager' on module '/Users/alex/cheri/llvm-project/llvm/test/CodeGen/Mips/cheri/simplifycfg-ptrtoint.ll'.
2.	Running pass 'Combine redundant instructions' on function '@cannot_fold_tag_unknown'
0  libLLVMSupport.dylib     0x0000000114515a9d llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 45
1  libLLVMSupport.dylib     0x00000001145153f1 llvm::sys::RunSignalHandlers() + 65
2  libLLVMSupport.dylib     0x0000000114515fbf SignalHandler(int) + 111
3  libsystem_platform.dylib 0x00007fff5b637b3d _sigtramp + 29
4  libsystem_platform.dylib 0x00007ffee20d0cf0 _sigtramp + 2259259856
5  libsystem_c.dylib        0x00007fff5b4f51c9 abort + 127
6  libLLVMSupport.dylib     0x000000011446bb12 llvm::llvm_unreachable_internal(char const*, char const*, unsigned int) + 162
7  libLLVMInstCombine.dylib 0x0000000112c345c8 llvm::InstCombiner::foldICmpUsingKnownBits(llvm::ICmpInst&) + 4136
8  libLLVMInstCombine.dylib 0x0000000112c34d19 llvm::InstCombiner::visitICmpInst(llvm::ICmpInst&) + 569
9  libLLVMInstCombine.dylib 0x0000000112bb9cf2 llvm::InstCombiner::run() + 1522
10 libLLVMInstCombine.dylib 0x0000000112bbb310 combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, bool, llvm::LoopInfo*) + 624
11 libLLVMInstCombine.dylib 0x0000000112bbb6d6 llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) + 214
12 libLLVMCore.dylib        0x0000000111c0bb4d llvm::FPPassManager::runOnFunction(llvm::Function&) + 317
13 libLLVMCore.dylib        0x0000000111c0be83 llvm::FPPassManager::runOnModule(llvm::Module&) + 99
14 libLLVMCore.dylib        0x0000000111c0c2c4 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) + 420
15 libLLVMCore.dylib        0x0000000111c0c036 llvm::legacy::PassManagerImpl::run(llvm::Module&) + 182
16 opt                      0x000000010db6657b main + 7163
17 libdyld.dylib            0x00007fff5b44ced9 start + 1
362 ↗(On Diff #192149)

I think it makes sense to remove some clang args before running creduce since removal of some flags can massively speed up reduction later (e.g. not emitting debug info or compiling at -O0, only doing -emit-llvm, etc) if they avoid expensive optimizations that don't cause the crash.

64 ↗(On Diff #192230)

Does this need to be Reduce(object): for python2?

123 ↗(On Diff #192230)

Some operating systems use a different assertion format (see my reduce script: https://github.com/CTSRD-CHERI/llvm-project/blob/master/clang/utils/creduce_crash_testcase.py#L662)

For MacOS/FreeBSD we need to also handle r"Assertion failed: \(.+\),". Over the past two years I have also had cases where the other message formats have been useful so I would quite like to see them added here as well.

205 ↗(On Diff #192230)

If we are writing the preprocessed output to that tempfile anyway, we could use stdout=tmpfile?

For python3 this would be simpler with subprocess.check_call() but I'm not sure python 2.7 has it.

310 ↗(On Diff #192230)

I would move this before the remove_arg_by_index call since all llvm args start with a - and try_remove_arg_by_index will cause lots of invalid command lines to be created otherwise.

akhuang marked 8 inline comments as done.Mar 26 2019, 1:56 PM
akhuang added inline comments.
clang/utils/creduce-clang-crash.py
303 ↗(On Diff #192218)

Sounds good-- I added -fsyntax-only, -emit-llvm and -Werror=implicit-int

145 ↗(On Diff #192149)

I changed the regex to ignore the # at the beginning of the line - I think that should cover the mac os stack trace

64 ↗(On Diff #192230)

I think it still works, but adding object makes sense.

123 ↗(On Diff #192230)

added the error messages from your script

205 ↗(On Diff #192230)

I think python2.7 has check_call - switched to using that

akhuang updated this revision to Diff 192350.Mar 26 2019, 1:57 PM
akhuang marked 2 inline comments as done.

added to error message regexes and command line flags

Only a few more nits on my side, and this LGTM. WDYT, arichardson?

clang/utils/creduce-clang-crash.py
137 ↗(On Diff #192149)

Sorry -- should've been clearer. I meant "in the comment in the code, please expand a bit [...]" :)

362 ↗(On Diff #192149)

Agreed. My question was more "why do we have special reduction code on both sides of this instead of just reduce_clang_args'ing on both sides of the run_creduce." It wasn't clear to me that simplify_clang_args was only intended to speed things up, but now it is. :)

198 ↗(On Diff #192350)

Did we want to use NamedTemporaryFile here as rnk suggested?

(If not, you can lift the os.closes to immediately after this line.)

206 ↗(On Diff #192350)

Similar question about NamedTemporaryFile.

Please note that you'll probably have to pass delete=False, since apparently delete=True sets O_TEMPORARY on Windows, which... might follow the file across renames? I'm unsure.

akhuang updated this revision to Diff 192499.Mar 27 2019, 12:52 PM
akhuang marked 3 inline comments as done.

change mkstemp to NamedTemporaryFile and add decode(utf-8) so it works on python3.5

clang/utils/creduce-clang-crash.py
198 ↗(On Diff #192350)

switched to using NamedTemporaryFile here -

206 ↗(On Diff #192350)

moved to NamedTemporaryFile with comment about delete=False

Only a few more nits on my side, and this LGTM. WDYT, arichardson?

LGTM with the minor tempfile changes.

clang/utils/creduce-clang-crash.py
201 ↗(On Diff #192499)

with tempfile.NamedTemporaryFile() as empty_file:? Definitely works with python 3 and 2.7 docs say This file-like object can be used in a with statement, just like a normal file..

210 ↗(On Diff #192499)

Use a with statement?

212 ↗(On Diff #192499)

Some crash messages might include the line numbers, do you think it makes sense to fall back to running with -E but without -P and also checking that? I do it in my script but I'm not sure preprocessing saves that much time since creduce will try to remove those statements early.

akhuang updated this revision to Diff 192669.Mar 28 2019, 9:59 AM
akhuang marked 3 inline comments as done.

Add preprocessing with clang -E only;
use with for opening files

clang/utils/creduce-clang-crash.py
212 ↗(On Diff #192499)

Makes sense-- in my experience preprocessing is still quite a bit faster than letting creduce remove all the statements.

arichardson accepted this revision.Mar 29 2019, 2:50 AM

LGTM once the tempfile is deleted.

clang/utils/creduce-clang-crash.py
208 ↗(On Diff #192669)

I believe we are currently not deleting this temporary file.
Can delete=False be removed since we are using shutil.copy() instead of a move?

This revision is now accepted and ready to land.Mar 29 2019, 2:50 AM
akhuang marked 2 inline comments as done.Mar 29 2019, 9:38 AM
akhuang added inline comments.
clang/utils/creduce-clang-crash.py
208 ↗(On Diff #192669)

Yeah, I think that should be fine.

akhuang updated this revision to Diff 192846.Mar 29 2019, 9:39 AM
akhuang marked an inline comment as done.

Tmpfile was not being removed

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2019, 10:49 AM

I've stumbled into an issue with the script:
It got a line: clang: /build/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2582: bool {anonymous}::IndVarSimplify::run(llvm::Loop*): Assertion L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' failed.`
And produced the following grep: grep 'L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' t.log || exit 1
But that doesn't work, escapes should be applied. it should be
grep 'L->isRecursivelyLCSSAForm(\*DT, \*LI) && \"LCSSA required to run indvars!\"' t.log || exit 1

I've stumbled into an issue with the script:
It got a line: clang: /build/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2582: bool {anonymous}::IndVarSimplify::run(llvm::Loop*): Assertion L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' failed.`
And produced the following grep: grep 'L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' t.log || exit 1
But that doesn't work, escapes should be applied. it should be
grep 'L->isRecursivelyLCSSAForm(\*DT, \*LI) && \"LCSSA required to run indvars!\"' t.log || exit 1

In my script I use grep -F " + shlex.quote(self.args.crash_message) which should escape both the message and ensure that the grep argument is not interpreted as a regex.

lebedev.ri added inline comments.Apr 24 2019, 11:50 AM
cfe/trunk/utils/creduce-clang-crash.py
185

I've stumbled into an issue with the script:
It got a line: clang: /build/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2582: bool {anonymous}::IndVarSimplify::run(llvm::Loop*): Assertion L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' failed.`
And produced the following grep: grep 'L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' t.log || exit 1
But that doesn't work, escapes should be applied. it should be
grep 'L->isRecursivelyLCSSAForm(\*DT, \*LI) && \"LCSSA required to run indvars!\"' t.log || exit 1

In my script I use grep -F " + shlex.quote(self.args.crash_message) which should escape both the message and ensure that the grep argument is not interpreted as a regex.

Great to hear!
It appears that the script is currently more primitive than that currently though.

akhuang marked an inline comment as done.Apr 25 2019, 11:05 AM
akhuang added inline comments.
cfe/trunk/utils/creduce-clang-crash.py
185

Thanks; I added a -F flag (r359216), and it seems like that fixes the issue.

lebedev.ri added inline comments.Apr 25 2019, 11:11 AM
cfe/trunk/utils/creduce-clang-crash.py
185

Thanks!
What about shlex? How do you know bash won't expand that as regex already?

akhuang marked an inline comment as done.Apr 25 2019, 2:37 PM
akhuang added inline comments.
cfe/trunk/utils/creduce-clang-crash.py
185

Sorry, not sure what you mean by bash expanding shlex as regex? It should be printing a quoted string to the bash script.

lebedev.ri added inline comments.Apr 25 2019, 11:29 PM
cfe/trunk/utils/creduce-clang-crash.py
185
$ ls konsole*
'konsole*'   konsole-ACNHfh.history   konsole-CVnmsn.history   konsole-HhEJRA.history
$ echo "konsole\*" | grep -F "konsole*"
$ echo "konsole\*" | grep -F "konsole\*"
konsole\*

The regex still needs to be escaped, regardless of -F.
Otherwise bash will expand it.