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