[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

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