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