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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/test/Shell/helper/toolchain.py:13
+
+def _lldb_init(config):
+    return os.path.join(config.test_exec_root, 'Shell', 'lit-lldb-init')
----------------
Maybe `_get_lldb_init_path`? This way it looks like this function is doing some fancy initialization..


================
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']
----------------
aprantl wrote:
> JDevlieghere wrote:
> > 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. 
> How about adding this as a comment here?
Yes, that's better now, though, per the other comment, I think the env vars can be removed completely.


================
Comment at: lldb/tools/lldb-repro/lldb-repro.cpp:63
+    args.push_back(repro_dir.c_str());
+    args.push_back("--reproducer-auto-generate");
+
----------------
JDevlieghere wrote:
> 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. 
Ok, that makes sense. That said, I think we will always want a way to disable this -- this just looks too much like one of those "this shit is tracking everything I do" features a lot of people will want to stay away from.


================
Comment at: lldb/tools/lldb-repro/lldb-repro.h.cmake:12
+
+#cmakedefine LLDB_TEST_EXECUTABLE "${LLDB_TEST_EXECUTABLE}"
+
----------------
JDevlieghere wrote:
> 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.
No, I really meant argv[1]. :)

The idea was that `%lldb` would expand to `/src/path/to/lldb-repro.py /build/path/to/lldb.exe --whatever`. That way, you wouldn't need to rely on the "same directory" trick and could get rid of all the cmake code. In fact, we could even throw in a `--capture/--replay` argument to the command line, and ditch the environment variables too...


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

https://reviews.llvm.org/D72823