[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

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



================
Comment at: clang/utils/creduce-clang-crash.py:137
+
+    # If no message was found, use the top five stack trace functions,
+    # ignoring some common functions
----------------
akhuang wrote:
> george.burgess.iv wrote:
> > 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?)
> There is no deep reason - it was an arbitrary smallish number to hopefully not only pick up common stack trace functions
Sorry -- should've been clearer. I meant "in the comment in the code, please expand a bit [...]" :)


================
Comment at: clang/utils/creduce-clang-crash.py:362
+  r = Reduce(crash_script, file_to_reduce)
+  r.simplify_clang_args()
+  r.write_interestingness_test()
----------------
arichardson wrote:
> george.burgess.iv wrote:
> > akhuang wrote:
> > > george.burgess.iv wrote:
> > > > 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?
> > > 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-
> > 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.
> 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.
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. :)


================
Comment at: clang/utils/creduce-clang-crash.py:198
+    # Instead of modifying the filename in the test file, just run the command
+    fd, empty_file = tempfile.mkstemp()
+    if self.check_expected_output(filename=empty_file):
----------------
Did we want to use `NamedTemporaryFile` here as rnk suggested?

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


================
Comment at: clang/utils/creduce-clang-crash.py:206
+    print("\nTrying to preprocess the source file...")
+    fd, tmpfile = tempfile.mkstemp()
+
----------------
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.


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

https://reviews.llvm.org/D59725