labath added a comment. I have a lot of comments, but I think they're mostly cosmetic. I think the general idea here is good.
================ Comment at: include/lldb/API/SBInitializerOptions.h:15 +#include "lldb/API/SBFileSpec.h" +#include "lldb/Initialization/SystemInitializer.h" + ---------------- You cannot include non-API headers from SB headers (cpp files are fine). `SystemInitializer::Options` needs to forward-declared. (I guess that means moving it out of the SystemInitializer class). ================ Comment at: include/lldb/API/SBInitializerOptions.h:17 + +#include <vector> + ---------------- not needed? ================ Comment at: include/lldb/API/SBInitializerOptions.h:32 + + mutable std::unique_ptr<lldb_private::SystemInitializer::Options> m_opaque_up; +}; ---------------- I think this will make the copy-constructor of this class deleted, which will not play well with python. I think you'll need to declare your own copy operations here (something like SBExpressionOptions, I guess). Also, why is this `mutable`? SBExpressionOptions seems to have that too, but it's not clear to me why would that be needed? ================ Comment at: include/lldb/Initialization/SystemInitializer.h:28 + + virtual void Initialize(Options options) = 0; virtual void Terminate() = 0; ---------------- `const Options &` ? (it contains a string so it's not trivial to copy). ================ Comment at: lit/Reproducer/TestGDBRemoteRepro.test:7 + +# RUN: %clang %S/Inputs/simple.c -g -o %t.out --target=x86_64-apple-macosx +# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture %T/reproducer -- %t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY ---------------- With `--target` this test will not run on other platforms. This sounds like it should work on platforms using gdb-remote (i.e., mac & linux). Maybe remove the --target and set an appropriate REQUIRES directive. ================ Comment at: lit/Reproducer/TestGDBRemoteRepro.test:8 +# RUN: %clang %S/Inputs/simple.c -g -o %t.out --target=x86_64-apple-macosx +# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture %T/reproducer -- %t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY +# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteReplay.in --replay %T/reproducer -- %t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY ---------------- `s/REPLAY/CAPTURE` ? ================ Comment at: scripts/interface/SBInitializerOptions.i:15 +public: + SBInitializerOptions (); + ---------------- Weird formatting. ================ Comment at: source/Initialization/SystemInitializerCommon.cpp:74 + + Reproducer::Initialize(mode, FileSpec(options.reproducer_path.c_str())); FileSystem::Initialize(); ---------------- remove `.c_str()` ================ Comment at: source/Utility/Reproducer.cpp:41-42 +Reproducer::Reproducer(ReproducerMode mode, llvm::Optional<FileSpec> root) { + // It's unfortunate that we have to do so much I/O here that can fail. The + // best we can do is not initialize the reproducer. + switch (mode) { ---------------- It should be possible to bubble this all the way up to the `SBDebugger::Initialize` call. Is there a reason to not do that? ================ Comment at: tools/driver/CMakeLists.txt:21 liblldb + lldbUtility ${host_lib} ---------------- This is wrong. The driver should only use the public API exposed via liblldb. Why did you need to do that? ================ Comment at: unittests/Utility/ReproducerTest.cpp:39-43 + llvm::Error SetCapture(bool b) { return Reproducer::SetCapture(b); } + + llvm::Error SetReplay(llvm::Optional<FileSpec> root) { + return Reproducer::SetReplay(root); + } ---------------- You should be able to change the visibility of these via `using` directives. (`using Reproducer::SetCapture`). Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55038/new/ https://reviews.llvm.org/D55038 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits