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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits