[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

[Pavel Labath via Phabricator via lldb-commits]

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