[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!



================
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
----------------
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?)


================
Comment at: clang/utils/creduce-clang-crash.py:145
+      matches = re.findall(stacktrace_re, crash_output)
+      result = filter(lambda x: x and x.strip() not in filters, matches)[:5]
+      for msg in result:
----------------
nit: please prefer `[x for x in matches if x and x.strip() not in filters][:5]`. py3's filter returns a generator, whereas py2's returns a list.


================
Comment at: clang/utils/creduce-clang-crash.py:193
+    # Instead of modifying the filename in the test file, just run the command
+    _, empty_file = tempfile.mkstemp()
+    if self.check_expected_output(filename=empty_file):
----------------
I think `_` is an open file descriptor; please `os.close` it if we don't want to use it


================
Comment at: clang/utils/creduce-clang-crash.py:199
+    print("\nTrying to preprocess the source file...")
+    _, tmpfile = tempfile.mkstemp()
+
----------------
Same nit


================
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,
----------------
was this intended to use `cmd`?


================
Comment at: clang/utils/creduce-clang-crash.py:205
+                         stderr=subprocess.STDOUT)
+    preprocessed, _ = p.communicate()
+
----------------
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)


================
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
----------------
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']`




================
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
----------------
Should we be `shlex.quote`'ing y here (and probably in the `return x + [y]` below)?


================
Comment at: clang/utils/creduce-clang-crash.py:251
+    new_args = self.filter_args(args, **kwargs)
+    if extra_arg and extra_arg not in new_args:
+      new_args.append(extra_arg)
----------------
IMO, if even `extra_arg` is in `new_args`, we should still move it near the end. Arg ordering matters in clang, generally with later args taking precedence over earlier ones. e.g. the -g$N args in https://godbolt.org/z/Mua8cs


================
Comment at: clang/utils/creduce-clang-crash.py:279
+                                                     "-debugger-tuning=",
+                                                     "-gdwarf"])
+    new_args = self.try_remove_args(new_args,
----------------
If we're replacing other args with their effective negation, does it also make sense to replace all debug-ish options with `-g0`?


================
Comment at: clang/utils/creduce-clang-crash.py:296
+    new_args = self.try_remove_args(new_args, msg="Removed -D options",
+                                    opts_startswith=["-D "])
+    new_args = self.try_remove_args(new_args, msg="Removed -I options",
----------------
Might not want to have a space here; `-DFOO=1` is valid (same for `-I` below)


================
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"])
----------------
Probably want to do a similar thing for `-Xclang` (which, as far as this code is concerned, acts a lot like `-mllvm`)


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


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

https://reviews.llvm.org/D59725