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