[Lldb-commits] [PATCH] D78801: [LLDB] Add class WasmProcess for WebAssembly debugging

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

Hi @paolosev, **many thanks for such a great patch which makes it possible to 
debug WebAssembly applications.** It is really really useful especially in 
non-browser environments.

We have enabled source debugging feature in **WebAssembly Micro Runtime 
** based on your patch 
(thanks @vwzm228 for the great work to make this happen!), 
and we have put the link of this patch to the ATTRIBUTIONS 
,
 and the acknowledgements in the document 
.

Please let me know if you have any concern or suggestion about this :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78801

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78801: [LLDB] Add class WasmProcess for WebAssembly debugging

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D78801#3279811 , @paolosev wrote:

> Thanks @xujuntwt95329! I am very happy that this was useful for WebAssembly 
> Micro Runtime!

I believe we are making the world of WebAssembly better!

BTW, I find an issue when trying to debug multi-thread wasm app:
The `qWasmLocal` package doesn't contain the thread id, which means it can only 
get locals of the current thread. If we have thread A, B and C, and they 
stopped at the same time, then LLDB will send three `qWasmLocal` package, and 
the wasm runtime will give same reply.

I think we should add thread id into `qWasmLocal` so that wasm runtime will 
know which thread to process, Am I right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78801

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78801: [LLDB] Add class WasmProcess for WebAssembly debugging

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D78801#3281504 , @paolosev wrote:

> Hi @xujuntwt95329,
> Honestly I wasn't thinking to support multithreading with this initial patch 
> so I am not surprised that it doesn't work. You are right, we should add 
> thread information to the messages.
> Actually I am a little surprised that the patch still works after all this 
> time (more than one year), and that there have not been small changes that 
> caused merge errors.
> I am afraid that manually patching LLDB might not be a sustainable solution. 
> :(
>
> @labath, @jingham 
> Given that the debugging feature is being supported also by WebAssembly Micro 
> Runtime  now, maybe 
> we could work together to finalize an implementation of this patch (or also 
> some other different solution) that will be less intrusive and more 
> acceptable to be merged in LLDB?

Well, your patch works well with **WebAssembly Micro Runtime (WAMR)** source 
debugging feature in most cases, I have debugged several wasm applications 
written in C, C++, Rust and Go, it is really useful !
I'll try to add thread id to `qWasmLocal` message and check if it works.

> I am afraid that manually patching LLDB might not be a sustainable solution. 
> :(

I totally agree with you, currently we ask the users to download llvm-project, 
apply the patch, and build the customized lldb, it takes a long time. It will 
be great if this can be merged into upstream !!!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78801

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 created this revision.
xujuntwt95329 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The key stored in the source map is a normalized path string with host's
path style, so it is also necessary to normalize the file path during
searching the map


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112439

Files:
  lldb/source/Target/PathMappingList.cpp


Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -218,7 +218,12 @@
 }
 
 llvm::Optional PathMappingList::FindFile(const FileSpec &orig_spec) 
const {
-  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+  // We must normalize the orig_spec again using the host's path style,
+  // otherwise there will be mismatch between the host and remote platform
+  // if they use different path styles.
+  if (auto remapped = RemapPath(
+  NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+  /*only_if_exists=*/true))
 return remapped;
 
   return {};


Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -218,7 +218,12 @@
 }
 
 llvm::Optional PathMappingList::FindFile(const FileSpec &orig_spec) const {
-  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+  // We must normalize the orig_spec again using the host's path style,
+  // otherwise there will be mismatch between the host and remote platform
+  // if they use different path styles.
+  if (auto remapped = RemapPath(
+  NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+  /*only_if_exists=*/true))
 return remapped;
 
   return {};
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D112439#3084948 , @wallace wrote:

> Is it possible to add a test for that? In any case, this makes sense to me



In D112439#3085304 , @clayborg wrote:

> Adding a test would be great for this. Please add one.

Thanks a lot for your comments, I'll add a test and update the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 382235.
xujuntwt95329 added a comment.
Herald added a subscriber: mgorny.

add unittest


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

Files:
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/FindFileTest.cpp

Index: lldb/unittests/Target/FindFileTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -0,0 +1,97 @@
+//===-- FindFileTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+using namespace llvm::sys::fs;
+using namespace lldb_private;
+
+namespace {
+struct Matches {
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  : original(o, style), remapped(r) {}
+};
+
+class FindFileTest : public testing::Test {
+public:
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();
+  }
+  void TearDown() override {
+HostInfo::Terminate();
+FileSystem::Terminate();
+  }
+};
+} // namespace
+
+static void TestFileFindings(const PathMappingList &map,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto &fail : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto &match : matches) {
+SCOPED_TRACE(match.original.GetPath() + " -> " + match.remapped);
+llvm::Optional remapped;
+
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+ConstString(match.remapped).GetStringRef());
+  }
+}
+
+TEST_F(FindFileTest, FindFileTests) {
+  const auto *Info = testing::UnitTest::GetInstance()->current_test_info();
+  llvm::SmallString<128> DirName, FileName;
+  int fd;
+
+  ASSERT_NO_ERROR(createUniqueDirectory(Info->name(), DirName));
+
+  sys::path::append(FileName, Twine(DirName), Twine("test"));
+  ASSERT_NO_ERROR(openFile(FileName, fd, CD_CreateAlways, FA_Read, OF_None));
+
+  llvm::FileRemover dir_remover(DirName);
+  llvm::FileRemover file_remover(FileName);
+  PathMappingList map;
+
+  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
+  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+
+  Matches matches[] = {
+  {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
+  {"/old/test", llvm::sys::path::Style::posix, FileName.c_str()},
+  {R"(C:\foo)", llvm::sys::path::Style::windows, DirName.c_str()},
+  {R"(C:\foo\test)", llvm::sys::path::Style::windows, FileName.c_str()}};
+
+  ArrayRef fails{
+  // path not mapped
+  FileSpec("/foo", llvm::sys::path::Style::posix),
+  FileSpec("/new", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\new)", llvm::sys::path::Style::windows),
+  // path mapped, but file not exist
+  FileSpec("/test1", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\foo\test2)", llvm::sys::path::Style::windows)};
+
+  TestFileFindings(map, matches, fails);
+}
Index: lldb/unittests/Target/CMakeLists.txt
===
--- lldb/unittests/Target/CMakeLists.txt
+++ lldb/unittests/Target/CMakeLists.txt
@@ -7,6 +7,7 @@
   PathMappingListTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
+  FindFileTest.cpp
 
   LINK_LIBS
   lldbCore
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -218,7 +218,12 @@
 }
 
 llvm::Optional PathMappingList::FindFile(const FileSpec &orig_spec) const {
-  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+  // We must normalize the orig_spec again using the host's path style,
+  // otherwise there will be mismatch between the host and remote platform
+  // if they use different path styles.
+  if (auto remapped = RemapPath(
+  NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+   

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 382237.
xujuntwt95329 added a comment.

minor fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

Files:
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/FindFileTest.cpp

Index: lldb/unittests/Target/FindFileTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -0,0 +1,97 @@
+//===-- FindFileTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+using namespace llvm::sys::fs;
+using namespace lldb_private;
+
+namespace {
+struct Matches {
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  : original(o, style), remapped(r) {}
+};
+
+class FindFileTest : public testing::Test {
+public:
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();
+  }
+  void TearDown() override {
+HostInfo::Terminate();
+FileSystem::Terminate();
+  }
+};
+} // namespace
+
+static void TestFileFindings(const PathMappingList &map,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto &fail : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto &match : matches) {
+SCOPED_TRACE(match.original.GetPath() + " -> " + match.remapped);
+llvm::Optional remapped;
+
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+ConstString(match.remapped).GetStringRef());
+  }
+}
+
+TEST_F(FindFileTest, FindFileTests) {
+  const auto *Info = testing::UnitTest::GetInstance()->current_test_info();
+  llvm::SmallString<128> DirName, FileName;
+  int fd;
+
+  ASSERT_NO_ERROR(createUniqueDirectory(Info->name(), DirName));
+
+  sys::path::append(FileName, Twine(DirName), Twine("test"));
+  ASSERT_NO_ERROR(openFile(FileName, fd, CD_CreateAlways, FA_Read, OF_None));
+
+  llvm::FileRemover dir_remover(DirName);
+  llvm::FileRemover file_remover(FileName);
+  PathMappingList map;
+
+  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
+  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+
+  Matches matches[] = {
+  {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
+  {"/old/test", llvm::sys::path::Style::posix, FileName.c_str()},
+  {R"(C:\foo)", llvm::sys::path::Style::windows, DirName.c_str()},
+  {R"(C:\foo\test)", llvm::sys::path::Style::windows, FileName.c_str()}};
+
+  ArrayRef fails{
+  // path not mapped
+  FileSpec("/foo", llvm::sys::path::Style::posix),
+  FileSpec("/new", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\new)", llvm::sys::path::Style::windows),
+  // path mapped, but file not exist
+  FileSpec("/old/test1", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\foo\test2)", llvm::sys::path::Style::windows)};
+
+  TestFileFindings(map, matches, fails);
+}
Index: lldb/unittests/Target/CMakeLists.txt
===
--- lldb/unittests/Target/CMakeLists.txt
+++ lldb/unittests/Target/CMakeLists.txt
@@ -7,6 +7,7 @@
   PathMappingListTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
+  FindFileTest.cpp
 
   LINK_LIBS
   lldbCore
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -218,7 +218,12 @@
 }
 
 llvm::Optional PathMappingList::FindFile(const FileSpec &orig_spec) const {
-  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+  // We must normalize the orig_spec again using the host's path style,
+  // otherwise there will be mismatch between the host and remote platform
+  // if they use different path styles.
+  if (auto remapped = RemapPath(
+  NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+  /*only_if_exists=*/true))
 

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

I've added the unit test. 
The "FindFile" method requires the FileSystem, so I added a new file rather 
then put these code into exist PathMappingListTest.cpp

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added inline comments.



Comment at: lldb/source/Target/PathMappingList.cpp:33-37
   ConstString NormalizePath(ConstString path) {
 // If we use "path" to construct a FileSpec, it will normalize the path for
 // us. We then grab the string and turn it back into a ConstString.
 return ConstString(FileSpec(path.GetStringRef()).GetPath());
   }

JDevlieghere wrote:
> Can this function take a `StringRef` and return a `std::string` instead? The 
> amount of roundtrips between `StringRef`s, `ConstString`s and `std::string`s 
> is getting a bit out of hand.
I agree with you. 
However, if we change the signature of this function, then we need to do all 
these conversion from the caller side, seems things are not going better.

For example

```
void PathMappingList::Append(ConstString path,
 ConstString replacement, bool notify) {
  ++m_mod_id;
  m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
  if (notify && m_callback)
m_callback(*this, m_callback_baton);
}
```

We need to convert `path` to StringRef, or we can change the type of parameter 
`path`, but this will require more modification from the caller of `Append`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 383083.
xujuntwt95329 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

Files:
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/FindFileTest.cpp

Index: lldb/unittests/Target/FindFileTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -0,0 +1,97 @@
+//===-- FindFileTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+using namespace llvm::sys::fs;
+using namespace lldb_private;
+
+namespace {
+struct Matches {
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  : original(o, style), remapped(r) {}
+};
+
+class FindFileTest : public testing::Test {
+public:
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();
+  }
+  void TearDown() override {
+HostInfo::Terminate();
+FileSystem::Terminate();
+  }
+};
+} // namespace
+
+static void TestFileFindings(const PathMappingList &map,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto &fail : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto &match : matches) {
+SCOPED_TRACE(match.original.GetPath() + " -> " + match.remapped);
+llvm::Optional remapped;
+
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+ConstString(match.remapped).GetStringRef());
+  }
+}
+
+TEST_F(FindFileTest, FindFileTests) {
+  const auto *Info = testing::UnitTest::GetInstance()->current_test_info();
+  llvm::SmallString<128> DirName, FileName;
+  int fd;
+
+  ASSERT_NO_ERROR(createUniqueDirectory(Info->name(), DirName));
+
+  sys::path::append(FileName, Twine(DirName), Twine("test"));
+  ASSERT_NO_ERROR(openFile(FileName, fd, CD_CreateAlways, FA_Read, OF_None));
+
+  llvm::FileRemover dir_remover(DirName);
+  llvm::FileRemover file_remover(FileName);
+  PathMappingList map;
+
+  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
+  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+
+  Matches matches[] = {
+  {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
+  {"/old/test", llvm::sys::path::Style::posix, FileName.c_str()},
+  {R"(C:\foo)", llvm::sys::path::Style::windows, DirName.c_str()},
+  {R"(C:\foo\test)", llvm::sys::path::Style::windows, FileName.c_str()}};
+
+  ArrayRef fails{
+  // path not mapped
+  FileSpec("/foo", llvm::sys::path::Style::posix),
+  FileSpec("/new", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\new)", llvm::sys::path::Style::windows),
+  // path mapped, but file not exist
+  FileSpec("/old/test1", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\foo\test2)", llvm::sys::path::Style::windows)};
+
+  TestFileFindings(map, matches, fails);
+}
Index: lldb/unittests/Target/CMakeLists.txt
===
--- lldb/unittests/Target/CMakeLists.txt
+++ lldb/unittests/Target/CMakeLists.txt
@@ -7,6 +7,7 @@
   PathMappingListTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
+  FindFileTest.cpp
 
   LINK_LIBS
   lldbCore
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -218,7 +218,12 @@
 }
 
 llvm::Optional PathMappingList::FindFile(const FileSpec &orig_spec) const {
-  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+  // We must normalize the orig_spec again using the host's path style,
+  // otherwise there will be mismatch between the host and remote platform
+  // if they use different path styles.
+  if (auto remapped = RemapPath(
+  NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+  /*only_if_exists=*/true))
 ret

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 383553.
xujuntwt95329 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

Files:
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/FindFileTest.cpp

Index: lldb/unittests/Target/FindFileTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -0,0 +1,97 @@
+//===-- FindFileTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+using namespace llvm::sys::fs;
+using namespace lldb_private;
+
+namespace {
+struct Matches {
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  : original(o, style), remapped(r) {}
+};
+
+class FindFileTest : public testing::Test {
+public:
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();
+  }
+  void TearDown() override {
+HostInfo::Terminate();
+FileSystem::Terminate();
+  }
+};
+} // namespace
+
+static void TestFileFindings(const PathMappingList &map,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto &fail : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto &match : matches) {
+SCOPED_TRACE(match.original.GetPath() + " -> " + match.remapped);
+llvm::Optional remapped;
+
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+ConstString(match.remapped).GetStringRef());
+  }
+}
+
+TEST_F(FindFileTest, FindFileTests) {
+  const auto *Info = testing::UnitTest::GetInstance()->current_test_info();
+  llvm::SmallString<128> DirName, FileName;
+  int fd;
+
+  ASSERT_NO_ERROR(createUniqueDirectory(Info->name(), DirName));
+
+  sys::path::append(FileName, Twine(DirName), Twine("test"));
+  ASSERT_NO_ERROR(openFile(FileName, fd, CD_CreateAlways, FA_Read, OF_None));
+
+  llvm::FileRemover dir_remover(DirName);
+  llvm::FileRemover file_remover(FileName);
+  PathMappingList map;
+
+  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
+  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+
+  Matches matches[] = {
+  {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
+  {"/old/test", llvm::sys::path::Style::posix, FileName.c_str()},
+  {R"(C:\foo)", llvm::sys::path::Style::windows, DirName.c_str()},
+  {R"(C:\foo\test)", llvm::sys::path::Style::windows, FileName.c_str()}};
+
+  ArrayRef fails{
+  // path not mapped
+  FileSpec("/foo", llvm::sys::path::Style::posix),
+  FileSpec("/new", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\new)", llvm::sys::path::Style::windows),
+  // path mapped, but file not exist
+  FileSpec("/old/test1", llvm::sys::path::Style::posix),
+  FileSpec(R"(C:\foo\test2)", llvm::sys::path::Style::windows)};
+
+  TestFileFindings(map, matches, fails);
+}
Index: lldb/unittests/Target/CMakeLists.txt
===
--- lldb/unittests/Target/CMakeLists.txt
+++ lldb/unittests/Target/CMakeLists.txt
@@ -7,6 +7,7 @@
   PathMappingListTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
+  FindFileTest.cpp
 
   LINK_LIBS
   lldbCore
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -218,7 +218,12 @@
 }
 
 llvm::Optional PathMappingList::FindFile(const FileSpec &orig_spec) const {
-  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+  // We must normalize the orig_spec again using the host's path style,
+  // otherwise there will be mismatch between the host and remote platform
+  // if they use different path styles.
+  if (auto remapped = RemapPath(
+  NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+  /*only_if_exists=*/true))
 ret

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 created this revision.
xujuntwt95329 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The amount of roundtrips between StringRefs, ConstStrings and std::strings is 
getting a bit out of hand, this patch avoid the unnecessary roundtrips.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string and turn it back into a ConstString.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, 

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 383559.
xujuntwt95329 added a comment.

address dblaikie's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
+  uint32_t index, bool notify) {
   if (index >= m_pairs.size())
 return false;
   ++m_mod_id;
@@ -221,2

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 marked 2 inline comments as done.
xujuntwt95329 added a comment.

@dblaikie Thanks for your detailed comments, I've uploaded the new patch-set


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 marked 2 inline comments as done.
xujuntwt95329 added a comment.

Hi, @aprantl

I've submitted a NFC patch here:
https://reviews.llvm.org/D112863

Seems that patch can't build by CI because it is based on this patch. In my 
understanding we need to merge this patch firstly and rebase that NFC patch to 
let CI work, right?

Please let me know if I misunderstand the workflow.

Thanks a lot!




Comment at: lldb/source/Target/PathMappingList.cpp:33
   // nomalized path pairs to ensure things match up.
   ConstString NormalizePath(ConstString path) {
 // If we use "path" to construct a FileSpec, it will normalize the path for

aprantl wrote:
> Why does this take a ConstString argument if it immediately calls 
> GetStringRef on it?
> Could you change this to take a StringRef here or in a follow-up NFC commit?
I've submitted a separate NFC patch：
https://reviews.llvm.org/D112863


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D112863#3098706 , @wallace wrote:

> @xujuntwt95329 do you have push permissions or do you need help to land this 
> diff?

I don't have push permissions, could you please help me to land this?

Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 383965.
xujuntwt95329 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
+  uint32_t index, bool notify) {
   if (index >= m_pairs.size())
 return false;
   ++m_mod_id;
@@ -221,20 +219,19 @@
   // We

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

@wallace rebased

But I guess you should apply this patch firstly?
https://reviews.llvm.org/D112439


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D112439#3102506 , @teemperor wrote:

> In D112439#3098307 , @xujuntwt95329 
> wrote:
>
>> Seems that patch can't build by CI because it is based on this patch. In my 
>> understanding we need to merge this patch firstly and rebase that NFC patch 
>> to let CI work, right?
>
> You can set parent/child revisions in Phabricator that should handle this 
> situation (not sure if the premerge-checks are respecting that, but that's 
> how Phab usually manages unmerged dependencies).
>
> But to be clear, the premerge checks on Phabricator are *not* building or 
> testing LLDB at all. See the CI log:
>
>   INFOprojects: {'lldb'}
>   INFOall excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 
> 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 
> 'openmp', 'lldb'}
>   INFOeffective projects list set()

Thanks a lot for the detailed information!

BTW, I see there are some test failure reported by CI, but I can't reproduce 
them in my local environment (X86_64 Ubuntu and Windows), is there any docker 
images to simulate the CI environment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D112439#3102548 , @teemperor wrote:

> In D112439#3102533 , @xujuntwt95329 
> wrote:
>
>> In D112439#3102506 , @teemperor 
>> wrote:
>>
>>> In D112439#3098307 , 
>>> @xujuntwt95329 wrote:
>>>
 Seems that patch can't build by CI because it is based on this patch. In 
 my understanding we need to merge this patch firstly and rebase that NFC 
 patch to let CI work, right?
>>>
>>> You can set parent/child revisions in Phabricator that should handle this 
>>> situation (not sure if the premerge-checks are respecting that, but that's 
>>> how Phab usually manages unmerged dependencies).
>>>
>>> But to be clear, the premerge checks on Phabricator are *not* building or 
>>> testing LLDB at all. See the CI log:
>>>
>>>   INFOprojects: {'lldb'}
>>>   INFOall excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 
>>> 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 
>>> 'openmp', 'lldb'}
>>>   INFOeffective projects list set()
>>
>> Thanks a lot for the detailed information!
>>
>> BTW, I see there are some test failure reported by CI, but I can't reproduce 
>> them in my local environment (X86_64 Ubuntu and Windows), is there any 
>> docker images to simulate the CI environment?
>
> Can you try a Release + Assert build? I'm looking at the failures at the 
> moment too and it seems to only fail in non-Debug builds.

Sure, I'll try the `release + assert` build. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D112439#3102600 , @teemperor wrote:

> In D112439#3102559 , @xujuntwt95329 
> wrote:
>
>> In D112439#3102548 , @teemperor 
>> wrote:
>>
>>> In D112439#3102533 , 
>>> @xujuntwt95329 wrote:
>>>
 In D112439#3102506 , @teemperor 
 wrote:

> In D112439#3098307 , 
> @xujuntwt95329 wrote:
>
>> Seems that patch can't build by CI because it is based on this patch. In 
>> my understanding we need to merge this patch firstly and rebase that NFC 
>> patch to let CI work, right?
>
> You can set parent/child revisions in Phabricator that should handle this 
> situation (not sure if the premerge-checks are respecting that, but 
> that's how Phab usually manages unmerged dependencies).
>
> But to be clear, the premerge checks on Phabricator are *not* building or 
> testing LLDB at all. See the CI log:
>
>   INFOprojects: {'lldb'}
>   INFOall excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 
> 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 
> 'openmp', 'lldb'}
>   INFOeffective projects list set()

 Thanks a lot for the detailed information!

 BTW, I see there are some test failure reported by CI, but I can't 
 reproduce them in my local environment (X86_64 Ubuntu and Windows), is 
 there any docker images to simulate the CI environment?
>>>
>>> Can you try a Release + Assert build? I'm looking at the failures at the 
>>> moment too and it seems to only fail in non-Debug builds.
>>
>> Sure, I'll try the `release + assert` build. Thanks again!
>
> Actually the solution seems pretty straightforward. `ArrayRef 
> fails{ ...` is not actually extending the lifetime or copying the initializer 
> list you're passing. You have to use a std::vector or something similar to 
> actually store the FileSpecs (right now they are being destroyed before the 
> `TestFileFindings` call).
>
> I'll go ahead and push a fix that just uses a std::vector.

You are right, thanks a lot!

Interestingly, I can't reproduce this problem even with release + assert build 
in my local environment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

Thanks a lot for your comments, they are very useful and I learned a lot about 
C++ by talking with you.

I'll address these comments and submit a patch and request your review.

Much appreciated！




Comment at: lldb/unittests/Target/FindFileTest.cpp:36
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();

teemperor wrote:
> You can simplify this code by giving this test struct a member 
> `SubsystemRAII subsystems;`. It will automatically call 
> these functions for you in the right order on test setUp/tearDown. It also 
> automatically adds error handling for init errors to your test. So this whole 
> class can all be:
> 
> ```
> lang=c++
> struct FindFileTest : public testing::Test {
>   SubsystemRAII subsystems;
> };
> ```
> You can simplify this code by giving this test struct a member 
> `SubsystemRAII subsystems;`. It will automatically call 
> these functions for you in the right order on test setUp/tearDown. It also 
> automatically adds error handling for init errors to your test. So this whole 
> class can all be:
> 
> ```
> lang=c++
> struct FindFileTest : public testing::Test {
>   SubsystemRAII subsystems;
> };
> ```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D113011: [lldb] make the code prettier in FindFileTest.cpp

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 created this revision.
xujuntwt95329 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113011

Files:
  lldb/unittests/Target/FindFileTest.cpp


Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -6,6 +6,7 @@
 //
 
//===--===//
 
+#include "TestingSupport/SubsystemRAII.h"
 #include "TestingSupport/TestUtilities.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
@@ -24,22 +25,13 @@
 namespace {
 struct Matches {
   FileSpec original;
-  llvm::StringRef remapped;
-  Matches(const char *o, const char *r) : original(o), remapped(r) {}
-  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  std::string remapped;
+  Matches(llvm::StringRef o, llvm::sys::path::Style style, llvm::StringRef r)
   : original(o, style), remapped(r) {}
 };
 
 class FindFileTest : public testing::Test {
-public:
-  void SetUp() override {
-FileSystem::Initialize();
-HostInfo::Initialize();
-  }
-  void TearDown() override {
-HostInfo::Terminate();
-FileSystem::Terminate();
-  }
+  SubsystemRAII subsystems;
 };
 } // namespace
 
@@ -53,11 +45,11 @@
 
   for (const auto &match : matches) {
 SCOPED_TRACE(match.original.GetPath() + " -> " + match.remapped);
-llvm::Optional remapped;
+llvm::Optional remapped = map.FindFile(match.original);
 
-EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
-EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
-ConstString(match.remapped).GetStringRef());
+EXPECT_TRUE(remapped);
+EXPECT_EQ(FileSpec(remapped.getValue()).GetPath(),
+  ConstString(match.remapped).GetStringRef());
   }
 }
 


Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "TestingSupport/SubsystemRAII.h"
 #include "TestingSupport/TestUtilities.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
@@ -24,22 +25,13 @@
 namespace {
 struct Matches {
   FileSpec original;
-  llvm::StringRef remapped;
-  Matches(const char *o, const char *r) : original(o), remapped(r) {}
-  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  std::string remapped;
+  Matches(llvm::StringRef o, llvm::sys::path::Style style, llvm::StringRef r)
   : original(o, style), remapped(r) {}
 };
 
 class FindFileTest : public testing::Test {
-public:
-  void SetUp() override {
-FileSystem::Initialize();
-HostInfo::Initialize();
-  }
-  void TearDown() override {
-HostInfo::Terminate();
-FileSystem::Terminate();
-  }
+  SubsystemRAII subsystems;
 };
 } // namespace
 
@@ -53,11 +45,11 @@
 
   for (const auto &match : matches) {
 SCOPED_TRACE(match.original.GetPath() + " -> " + match.remapped);
-llvm::Optional remapped;
+llvm::Optional remapped = map.FindFile(match.original);
 
-EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
-EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
-ConstString(match.remapped).GetStringRef());
+EXPECT_TRUE(remapped);
+EXPECT_EQ(FileSpec(remapped.getValue()).GetPath(),
+  ConstString(match.remapped).GetStringRef());
   }
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D113011: [lldb] make the code prettier in FindFileTest.cpp

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

Thanks to @teemperor's comments in https://reviews.llvm.org/D112439


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 marked 7 inline comments as done.
xujuntwt95329 added a comment.

I have submitted a new patch here: 
https://reviews.llvm.org/D113011

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits