akhuang marked 15 inline comments as done.
akhuang added inline comments.
================
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
----------------
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
================
Comment at: clang/utils/creduce-clang-crash.py:202
+ cmd = self.get_crash_cmd() + ['-E', '-P']
+ p = subprocess.Popen(self.get_crash_cmd() + ['-E', '-P'],
+ stdout=subprocess.PIPE,
----------------
george.burgess.iv wrote:
> was this intended to use `cmd`?
yep
================
Comment at: clang/utils/creduce-clang-crash.py:205
+ stderr=subprocess.STDOUT)
+ preprocessed, _ = p.communicate()
+
----------------
george.burgess.iv wrote:
> 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)
I think checking the exit code is a good idea
================
Comment at: clang/utils/creduce-clang-crash.py:222
+ def append_flags(x, y):
+ if len(x) > 0 and x[-1].startswith('-') and not y.startswith('-'):
+ x[-1] += ' ' + y
----------------
george.burgess.iv wrote:
> 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']`
>
>
I guess that was originally the intention, although now that I think of it it
makes more sense to group at most one argument.
================
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
----------------
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?
================
Comment at: clang/utils/creduce-clang-crash.py:279
+ "-debugger-tuning=",
+ "-gdwarf"])
+ new_args = self.try_remove_args(new_args,
----------------
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.
================
Comment at: clang/utils/creduce-clang-crash.py:362
+ r = Reduce(crash_script, file_to_reduce)
+ r.simplify_clang_args()
+ r.write_interestingness_test()
----------------
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-
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
