[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
JDevlieghere updated this revision to Diff 177598.
JDevlieghere added a comment.
- Rebase
- Copy over permissions
- Fix path when launching.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54617/new/
https://reviews.llvm.org/D54617
Files:
include/lldb/Host/FileSystem.h
include/lldb/Utility/FileCollector.h
include/lldb/Utility/Reproducer.h
lit/Reproducer/Inputs/FileCapture.in
lit/Reproducer/Inputs/FileReplay.in
lit/Reproducer/TestFileRepro.test
lit/Reproducer/TestGDBRemoteRepro.test
source/Host/common/FileSystem.cpp
source/Host/macosx/objcxx/Host.mm
source/Initialization/SystemInitializerCommon.cpp
source/Utility/CMakeLists.txt
source/Utility/FileCollector.cpp
source/Utility/Reproducer.cpp
Index: source/Utility/Reproducer.cpp
===
--- source/Utility/Reproducer.cpp
+++ source/Utility/Reproducer.cpp
@@ -223,3 +223,4 @@
void ProviderBase::anchor() {}
char ProviderBase::ID = 0;
+char FileProvider::ID = 0;
Index: source/Utility/FileCollector.cpp
===
--- /dev/null
+++ source/Utility/FileCollector.cpp
@@ -0,0 +1,148 @@
+//===-- FileCollector.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/FileCollector.h"
+
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+using namespace lldb_private;
+using namespace llvm;
+
+static bool IsCaseSensitivePath(StringRef path) {
+ SmallString<256> tmp_dest = path, upper_dest, real_dest;
+
+ // Remove component traversals, links, etc.
+ if (!sys::fs::real_path(path, tmp_dest))
+return true; // Current default value in vfs.yaml
+ path = tmp_dest;
+
+ // Change path to all upper case and ask for its real path, if the latter
+ // exists and is equal to path, it's not case sensitive. Default to case
+ // sensitive in the absence of real_path, since this is the YAMLVFSWriter
+ // default.
+ upper_dest = path.upper();
+ if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest))
+return false;
+ return true;
+}
+
+FileCollector::FileCollector(const FileSpec ) : m_root(directory) {
+ sys::fs::create_directories(m_root.GetPath(), true);
+}
+
+bool FileCollector::GetRealPath(StringRef src_path,
+SmallVectorImpl ) {
+ SmallString<256> real_path;
+ StringRef FileName = sys::path::filename(src_path);
+ std::string directory = sys::path::parent_path(src_path).str();
+ auto dir_with_symlink = m_symlink_map.find(directory);
+
+ // Use real_path to fix any symbolic link component present in a path.
+ // Computing the real path is expensive, cache the search through the
+ // parent path directory.
+ if (dir_with_symlink == m_symlink_map.end()) {
+if (!sys::fs::real_path(directory, real_path))
+ return false;
+m_symlink_map[directory] = real_path.str();
+ } else {
+real_path = dir_with_symlink->second;
+ }
+
+ sys::path::append(real_path, FileName);
+ result.swap(real_path);
+ return true;
+}
+
+void FileCollector::AddFile(const Twine ) {
+ std::lock_guard lock(m_mutex);
+ std::string file_str = file.str();
+ if (MarkAsSeen(file_str))
+AddFileImpl(file_str);
+}
+
+void FileCollector::AddFileImpl(StringRef src_path) {
+ std::string root = m_root.GetPath();
+
+ // We need an absolute src path to append to the root.
+ SmallString<256> absolute_src = src_path;
+ sys::fs::make_absolute(absolute_src);
+
+ // Canonicalize src to a native path to avoid mixed separator styles.
+ sys::path::native(absolute_src);
+
+ // 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);
+
+ // If a ".." component is present after a symlink component, remove_dots may
+ // lead to the wrong real destination path. Let the source be canonicalized
+ // like that but make sure we always use the real path for the destination.
+ SmallString<256> copy_from;
+ if (!GetRealPath(absolute_src, copy_from))
+copy_from = virtual_path;
+
+ SmallString<256> dst_path = StringRef(root);
+ sys::path::append(dst_path, sys::path::relative_path(copy_from));
+
+ // Always map a canonical src path to its real path into the YAML, by doing
+ // this we map different virtual src paths to the same entry in the VFS
+ // overlay, which is a way to emulate symlink inside the VFS; this is also
+ // needed for correctness, not doing that can lead to module
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
JDevlieghere updated this revision to Diff 176035.
JDevlieghere added a comment.
- Get the external path with modifying the VFS in LLVM.
- Integrate with the new integration logic.
- Add a test. (that doesn't work yet, still WIP)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54617/new/
https://reviews.llvm.org/D54617
Files:
include/lldb/Host/FileSystem.h
include/lldb/Utility/FileCollector.h
include/lldb/Utility/Reproducer.h
lit/Reproducer/Inputs/FileCapture.in
lit/Reproducer/Inputs/FileReplay.in
lit/Reproducer/TestFileRepro.test
lit/Reproducer/TestGDBRemoteRepro.test
source/Host/common/FileSystem.cpp
source/Initialization/SystemInitializerCommon.cpp
source/Utility/CMakeLists.txt
source/Utility/FileCollector.cpp
source/Utility/Reproducer.cpp
Index: source/Utility/Reproducer.cpp
===
--- source/Utility/Reproducer.cpp
+++ source/Utility/Reproducer.cpp
@@ -224,3 +224,4 @@
void ProviderBase::anchor() {}
char ProviderBase::ID = 0;
+char FileProvider::ID = 0;
Index: source/Utility/FileCollector.cpp
===
--- /dev/null
+++ source/Utility/FileCollector.cpp
@@ -0,0 +1,137 @@
+//===-- FileCollector.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/FileCollector.h"
+
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+using namespace lldb_private;
+using namespace llvm;
+
+static bool IsCaseSensitivePath(StringRef path) {
+ SmallString<256> tmp_dest = path, upper_dest, real_dest;
+
+ // Remove component traversals, links, etc.
+ if (!sys::fs::real_path(path, tmp_dest))
+return true; // Current default value in vfs.yaml
+ path = tmp_dest;
+
+ // Change path to all upper case and ask for its real path, if the latter
+ // exists and is equal to path, it's not case sensitive. Default to case
+ // sensitive in the absence of real_path, since this is the YAMLVFSWriter
+ // default.
+ upper_dest = path.upper();
+ if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest))
+return false;
+ return true;
+}
+
+FileCollector::FileCollector(const FileSpec ) : m_root(directory) {
+ sys::fs::create_directories(m_root.GetPath(), true);
+}
+
+bool FileCollector::GetRealPath(StringRef src_path,
+SmallVectorImpl ) {
+ SmallString<256> real_path;
+ StringRef FileName = sys::path::filename(src_path);
+ std::string directory = sys::path::parent_path(src_path).str();
+ auto dir_with_symlink = m_symlink_map.find(directory);
+
+ // Use real_path to fix any symbolic link component present in a path.
+ // Computing the real path is expensive, cache the search through the
+ // parent path directory.
+ if (dir_with_symlink == m_symlink_map.end()) {
+if (!sys::fs::real_path(directory, real_path))
+ return false;
+m_symlink_map[directory] = real_path.str();
+ } else {
+real_path = dir_with_symlink->second;
+ }
+
+ sys::path::append(real_path, FileName);
+ result.swap(real_path);
+ return true;
+}
+
+void FileCollector::AddFile(const Twine ) {
+ std::lock_guard lock(m_mutex);
+ std::string file_str = file.str();
+ if (MarkAsSeen(file_str))
+AddFileImpl(file_str);
+}
+
+void FileCollector::AddFileImpl(StringRef src_path) {
+ std::string root = m_root.GetPath();
+
+ // We need an absolute src path to append to the root.
+ SmallString<256> absolute_src = src_path;
+ sys::fs::make_absolute(absolute_src);
+
+ // Canonicalize src to a native path to avoid mixed separator styles.
+ sys::path::native(absolute_src);
+
+ // 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);
+
+ // If a ".." component is present after a symlink component, remove_dots may
+ // lead to the wrong real destination path. Let the source be canonicalized
+ // like that but make sure we always use the real path for the destination.
+ SmallString<256> copy_from;
+ if (!GetRealPath(absolute_src, copy_from))
+copy_from = virtual_path;
+
+ SmallString<256> dst_path = StringRef(root);
+ sys::path::append(dst_path, sys::path::relative_path(copy_from));
+
+ // Always map a canonical src path to its real path into the YAML, by doing
+ // this we map different virtual src paths to the same entry in the VFS
+ // overlay, which is a way to emulate symlink inside the VFS; this is also
+ //
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
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> 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 : 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
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
JDevlieghere added a comment. 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 :-) > Maybe you meant that by `Initialization of the FS needs to happen in the > driver.`? In any case, for me the initialization part is the interesting/hard > part of this problem. The details of how to collect the files are trivial > compared to that and possibly depend on the result of the decisions made > there. Yup, as per your suggestion in another review. I have an (unfinished) patch that splits off parsing of the command line arguments in the driver so we know the reproducer path before we initialize anything. That's pretty much the whole reason that we currently go through the debugger right now. 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
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
labath added a comment. 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 you meant that by `Initialization of the FS needs to happen in the driver.`? In any case, for me the initialization part is the interesting/hard part of this problem. The details of how to collect the files are trivial compared to that and possibly depend on the result of the decisions made there. 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
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
JDevlieghere added a comment. - Initialization of the FS needs to happen in the driver. - All file access has to go through the new FileSystem. - Needs tests. 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
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
JDevlieghere created this revision.
Herald added a subscriber: mgorny.
JDevlieghere added a subscriber: labath.
This patch is WIP and meant to showcase how the reproducer and VFS will
interact.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D54617
Files:
include/lldb/Host/FileSystem.h
include/lldb/Utility/FileCollector.h
include/lldb/Utility/Reproducer.h
source/Core/Debugger.cpp
source/Host/common/FileSystem.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Utility/CMakeLists.txt
source/Utility/FileCollector.cpp
source/Utility/Reproducer.cpp
Index: source/Utility/Reproducer.cpp
===
--- source/Utility/Reproducer.cpp
+++ source/Utility/Reproducer.cpp
@@ -187,7 +187,7 @@
return llvm::Error::success();
}
-llvm::Optional Loader::GetProviderInfo(StringRef name) {
+llvm::Optional Loader::GetInfo(StringRef name) {
assert(m_loaded);
auto it = m_provider_info.find(name);
Index: source/Utility/FileCollector.cpp
===
--- /dev/null
+++ source/Utility/FileCollector.cpp
@@ -0,0 +1,134 @@
+//===-- FileCollector.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/FileCollector.h"
+
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+using namespace lldb_private;
+using namespace llvm;
+
+static bool IsCaseSensitivePath(StringRef path) {
+ SmallString<256> tmp_dest = path, upper_dest, real_dest;
+
+ // Remove component traversals, links, etc.
+ if (!sys::fs::real_path(path, tmp_dest))
+return true; // Current default value in vfs.yaml
+ path = tmp_dest;
+
+ // Change path to all upper case and ask for its real path, if the latter
+ // exists and is equal to path, it's not case sensitive. Default to case
+ // sensitive in the absence of real_path, since this is the YAMLVFSWriter
+ // default.
+ for (auto : path)
+upper_dest.push_back(toUpper(C));
+ if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest))
+return false;
+ return true;
+}
+
+FileCollector::FileCollector(const FileSpec ) : m_root(directory) {
+ sys::fs::create_directories(m_root.GetPath(), true);
+}
+
+bool FileCollector::GetRealPath(StringRef src_path,
+SmallVectorImpl ) {
+ SmallString<256> real_path;
+ StringRef FileName = sys::path::filename(src_path);
+ std::string directory = sys::path::parent_path(src_path).str();
+ auto dir_with_symlink = m_symlink_map.find(directory);
+
+ // Use real_path to fix any symbolic link component present in a path.
+ // Computing the real path is expensive, cache the search through the
+ // parent path directory.
+ if (dir_with_symlink == m_symlink_map.end()) {
+if (!sys::fs::real_path(directory, real_path))
+ return false;
+m_symlink_map[directory] = real_path.str();
+ } else {
+real_path = dir_with_symlink->second;
+ }
+
+ sys::path::append(real_path, FileName);
+ result.swap(real_path);
+ return true;
+}
+
+void FileCollector::AddFile(const Twine ) {
+ std::lock_guard lock(m_mutex);
+ std::string file_str = file.str();
+ if (MarkAsSeen(file_str))
+AddFileImpl(file_str);
+}
+
+void FileCollector::AddFileImpl(StringRef src_path) {
+ std::string root = m_root.GetPath();
+
+ // We need an absolute src path to append to the root.
+ SmallString<256> absolute_src = src_path;
+ sys::fs::make_absolute(absolute_src);
+
+ // Canonicalize src to a native path to avoid mixed separator styles.
+ sys::path::native(absolute_src);
+
+ // 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);
+
+ // If a ".." component is present after a symlink component, remove_dots may
+ // lead to the wrong real destination path. Let the source be canonicalized
+ // like that but make sure we always use the real path for the destination.
+ SmallString<256> copy_from;
+ if (!GetRealPath(absolute_src, copy_from))
+copy_from = virtual_path;
+
+ SmallString<256> dst_path = StringRef(root);
+ sys::path::append(dst_path, sys::path::relative_path(copy_from));
+
+ // Always map a canonical src path to its real path into the YAML, by doing
+ // this we map different virtual src paths to the same entry in the VFS
+ // overlay, which is a way to emulate symlink inside the VFS; this is also
+ // needed for correctness, not doing that can lead to
