[Lldb-commits] [PATCH] D75877: [lldb/Reproducers] Fix replay for process attach workflows

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

A bunch of comments but nothing really major. Maybe it would be nice to put the code for yamlification of ProcessInfo into a separate patch?



================
Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:499-512
+        auto error_or_file = MemoryBuffer::getFile(*process_file);
+        if (auto err = error_or_file.getError()) {
+          SetError(result, errorCodeToError(err));
+          return false;
+        }
+
+        ProcessInstanceInfoList infos;
----------------
Maybe some kind of a utility function to convert a file to an object?
`template<typename T> Expected<T> readAsYaml(StringRef filename)` ?


================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:596
                              ProcessInstanceInfoList &process_infos) {
+  if (llvm::Optional<ProcessInstanceInfoList> infos =
+          repro::GetReplayProcessInstanceInfoList()) {
----------------
This means that every implementation of FindProcesses will need to introduce this bolierplate. We should put this into common code somehow. One way to do that would be to rename all the platform-specific implementations to something like DoFindProcesses, and then implement FindProcesses `source/Host/common/Host.cpp` to handle the delegation & reproducer logic.


================
Comment at: lldb/source/Utility/FileSpec.cpp:543
+                                                raw_ostream &Out) {
+  Out << Val.GetPath();
+}
----------------
There's more to FileSpecs than just the path -- they also hold the path syntax and the "case-sensitive" bit. Kind of not needed for your current goal, but maybe we should add those too while we're here?


================
Comment at: lldb/source/Utility/ProcessInfo.cpp:400-403
+llvm::StringRef llvm::yaml::MappingTraits<ProcessInstanceInfo>::validate(
+    IO &io, ProcessInstanceInfo &) {
+  return {};
+}
----------------
You don't actually have to provide these functions if they are not going to do anything.


================
Comment at: lldb/source/Utility/ProcessInfo.cpp:417-430
+  static std::unique_ptr<repro::MultiLoader<repro::ProcessInfoProvider>>
+      loader = repro::MultiLoader<repro::ProcessInfoProvider>::Create(
+          repro::Reproducer::Instance().GetLoader());
+
+  if (!loader)
+    return {};
+
----------------
random thought: Would any of this be simpler if this wasn't a "multi" provider but rather stored all of the responses as a sequence in a single file?


================
Comment at: lldb/source/Utility/ProcessInfo.cpp:436
+
+  if (auto err = yin.error())
+    return {};
----------------
what's the type of this?


================
Comment at: lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py:30
+
+        inferior = self.spawnSubprocess(
+            self.getBuildArtifact("somewhat_unique_name"))
----------------
You still need to do the `wait_for_file_on_target` dance here to ensure that `lldb_enable_attach` is executed before we actually attach. One example of that is in  `test/API/python_api/hello_world/main.c`.


================
Comment at: lldb/test/API/functionalities/reproducers/attach/main.cpp:10
+  while (true) {
+    std::this_thread::sleep_for(microseconds(1));
+  }
----------------
You probably copied this from some existing test, but I'd say this is putting unnecessary load on the system. For this use case even a 1-second sleep would be perfectly fine.


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

https://reviews.llvm.org/D75877