george.burgess.iv marked an inline comment as done.
george.burgess.iv added inline comments.
================
Comment at: clang/utils/creduce-clang-crash.py:223
+ if len(x) > 0 and x[-1].startswith('-') and not y.startswith('-'):
+ x[-1] += ' ' + y
+ return x
----------------
akhuang wrote:
> george.burgess.iv wrote:
> > Should we be `shlex.quote`'ing y here (and probably in the `return x + [y]`
> > below)?
> It quotes everything right before writing to file - are there reasons to
> quote here instead?
We're `shlex.split`ing 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.
================
Comment at: clang/utils/creduce-clang-crash.py:279
+ "-debugger-tuning=",
+ "-gdwarf"])
+ new_args = self.try_remove_args(new_args,
----------------
akhuang wrote:
> george.burgess.iv wrote:
> > If we're replacing other args with their effective negation, does it also
> > make sense to replace all debug-ish options with `-g0`?
> 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.
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.
================
Comment at: clang/utils/creduce-clang-crash.py:306
+ # Remove other cases that aren't covered by the heuristic
+ new_args = self.try_remove_args(new_args, msg="Removed -mllvm",
+ opts_one_arg_startswith=["-mllvm"])
----------------
george.burgess.iv wrote:
> Probably want to do a similar thing for `-Xclang` (which, as far as this code
> is concerned, acts a lot like `-mllvm`)
(You can ignore this comment if we're dealing in cc1; `-Xclang` is just "pass
this directly as a cc1 arg")
================
Comment at: clang/utils/creduce-clang-crash.py:362
+ r = Reduce(crash_script, file_to_reduce)
+ r.simplify_clang_args()
+ r.write_interestingness_test()
----------------
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.
================
Comment at: clang/utils/creduce-clang-crash.py:303
+ opts_startswith=["-O"])
+ self.clang_args = new_args
+ verbose_print("Simplified command:", quote_cmd(self.get_crash_cmd()))
----------------
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.
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
