[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider

[Pavel Labath via Phabricator via lldb-commits]

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