[Lldb-commits] [PATCH] D72823: [Reproducers] Add a tool to transparently capture and replay lldb sessions

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 4 inline comments as done.
JDevlieghere added a comment.

In D72823#1824152 <https://reviews.llvm.org/D72823#1824152>, @labath wrote:

> Considered making this a python script?


I think that makes sense



================
Comment at: lldb/test/Shell/lit.cfg.py:42-46
 if 'LLDB_CAPTURE_REPRODUCER' in os.environ:
   config.environment['LLDB_CAPTURE_REPRODUCER'] = os.environ[
       'LLDB_CAPTURE_REPRODUCER']
+if 'LLDB_REPRO_CAPTURE' in os.environ:
+  config.environment['LLDB_REPRO_CAPTURE'] = os.environ['LLDB_REPRO_CAPTURE']
----------------
labath wrote:
> The difference between these two is super-unclear. Any chance to unify them?
Not really, one is for the tool, the other is to enable reproducer capture by default. They're totally orthogonal, but I agree it's confusing. 


================
Comment at: lldb/tools/lldb-repro/lldb-repro.cpp:23
+
+unsigned ComputerHash(StringRef args, StringRef cwd) {
+  const unsigned hash = djbHash(cwd);
----------------
labath wrote:
> Is the `r` in the name intentional?
Nope, this is a typo


================
Comment at: lldb/tools/lldb-repro/lldb-repro.cpp:63
+    args.push_back(repro_dir.c_str());
+    args.push_back("--reproducer-auto-generate");
+
----------------
labath wrote:
> I am wondering if this shouldn't even be the default behavior. If I already passed `--capture` to lldb then it does not seem unreasonable to always write it out when lldb exits (we already do it after a crash, right?)
My ultimate goal is to have reproducers enabled by default. The capture flag is just away to make that behavior opt-in for now. You might use it to get a reproducer on a crash, but you don't necessary want to keep all reproducers around otherwise. 


================
Comment at: lldb/tools/lldb-repro/lldb-repro.h.cmake:12
+
+#cmakedefine LLDB_TEST_EXECUTABLE "${LLDB_TEST_EXECUTABLE}"
+
----------------
labath wrote:
> labath wrote:
> > Are you sure this will work fine with multi-config generators? You might be better off just relying on the fact that the lldb executable will sit right next to this binary...
> Actually how, is this thing going to be invoked exactly? Couldn't the path to lldb be passed simply as argv[1]?
It just needs patching up like lldb-dotest and lit. Assuming you mean `argv[0]`, it think we could make that work if I replace "%lldb" with a path to lldb-repro.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D72823