labath added a comment. In https://reviews.llvm.org/D54617#1302376, @JDevlieghere wrote:
> In https://reviews.llvm.org/D54617#1302366, @labath wrote: > > > I am confused by differing scopes of various objects that are interacting > > here. It seems you are implementing capture/replay as something that can be > > flicked on/off at any point during a debug session (`Debugger` lifetime). > > However, you are modifying global state (the filesystem singleton, at > > least), which is shared by all debug sessions. If multiple debug sessions > > try to enable capture/replay (or even access the filesystem concurrently, > > as none of this is synchronized in any way), things will break horribly. > > This seems like a bad starting point in implementing a feature, as I don't > > see any way to fix incrementally in the future. > > > Maybe I should've added a comment (I didn't want to litter the code with > design goals) but the goal of the commands is not really to enable/disable > the reproducer feature. I'm currently abusing the commands to do that, but > the long term goal is that you can enable/disable part of the reproducer. For > example, you could say only capture GDB packets or capture only files. It > makes testing a lot easier if you can isolate a single source of information. > Since there's currently only GDB packets I didn't split it up (yet). > > Also I piggy-backed off this to enable/disable the feature as a whole because > I needed to go through the debugger. See my reply below on why and how > that'll be fixed by the next patch :-) Ok, that makes sense, thanks for the explanation. I guess that means that filesystem capturing (due to it being global) will then be one of the things that cannot be enabled/disabled at runtime ? ================ Comment at: source/Host/common/FileSystem.cpp:71-74 + llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> buffer = + m_fs->getBufferForFile(mapping); + if (!buffer) + return; ---------------- Do you want to propagate this error up the stack? ================ Comment at: source/Utility/FileCollector.cpp:31-32 + // default. + for (auto &C : path) + upper_dest.push_back(toUpper(C)); + if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest)) ---------------- `upper_dest = path.upper();` ? ================ Comment at: source/Utility/FileCollector.cpp:82-87 + // Remove redundant leading "./" pieces and consecutive separators. + absolute_src = sys::path::remove_leading_dotslash(absolute_src); + + // Canonicalize the source path by removing "..", "." components. + SmallString<256> virtual_path = absolute_src; + sys::path::remove_dots(virtual_path, /*remove_dot_dot=*/true); ---------------- Is there anything which `remove_leading_dotslash` does, which wouldn't be done by the latter `remove_dots` call? Repository: rLLDB LLDB https://reviews.llvm.org/D54617 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
