[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

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112439#3102752 , @xujuntwt95329 
wrote:

> 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！

You're very welcome. Also these comments are all just small nits and not 
urgent, so feel free to address them whenever you have time.


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] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Fixed in 58dd658583eec9af24ca1262e1bce9f884d65487 


(Also left some nits in the test because I anyway looked over the code.)




Comment at: lldb/unittests/Target/FindFileTest.cpp:27
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}

I think this can just be a `std::string`. Right now it's relying on the caller 
to preserve the C-string storage, but that really isn't obvious to the whoever 
might update the test (and then has to debug a use-after-free).



Comment at: lldb/unittests/Target/FindFileTest.cpp:28
+  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)

No one is calling this IIUC?



Comment at: lldb/unittests/Target/FindFileTest.cpp:29
+  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) {}

I think both of those parameters can be `StringRefs` (FileSpec anyway converts 
it back to a StringRef).



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

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;
};
```



Comment at: lldb/unittests/Target/FindFileTest.cpp:58
+
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==

Couldn't `remapped` just be initialized to the FindFile value? Then you don't 
have to do the whole dance with converting to bool and avoiding the compiler 
warnings for assignments that look like comparisons.



Comment at: lldb/unittests/Target/FindFileTest.cpp:59
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+ConstString(match.remapped).GetStringRef());

`EXPECT_EQ` (StringRef is supported in the gtest macros)


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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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).


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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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.


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#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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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()


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

[Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe19ae352ce2: normalize file path when searching the source 
map (authored by xujuntwt95329, committed by Walter Erquinigo 
wall...@fb.com).

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 ,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto  : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto  : 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 _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(

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.

In D112439#3098307 , @xujuntwt95329 
wrote:

> 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?

Yes, that sounds right!

> Please let me know if I misunderstand the workflow.
>
> Thanks a lot!




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 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] 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 ,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto  : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto  : 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 _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;
 
   

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



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

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?



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());
   }

xujuntwt95329 wrote:
> 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`
IMO, the best solution would be change this too:
```
void PathMappingList::Append(StringRef path,
 StringRef replacement, bool notify) {
```


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 ,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto  : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto  : 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 _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;
 
   

[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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere 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());
   }

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.


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.

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 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 ,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto  : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto  : 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 _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;
 
 

[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 ,
+ llvm::ArrayRef matches,
+ llvm::ArrayRef fails) {
+  for (const auto  : fails) {
+SCOPED_TRACE(fail.GetCString());
+EXPECT_FALSE(map.FindFile(fail));
+  }
+
+  for (const auto  : 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 _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 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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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


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

[walter erquinigo via Phabricator via lldb-commits]

wallace accepted this revision.
wallace added a comment.
This revision is now accepted and ready to land.

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


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 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 _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 _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