normalize file path when searching the source map

[xujuntwt95329]

Summary

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

[wallace]

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

[clayborg]

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

[xujuntwt95329]

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.

[xujuntwt95329]

add unittest

[xujuntwt95329]

minor fix

[xujuntwt95329]

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!

[JDevlieghere]

lldb/source/Target/PathMappingList.cpp

33–37

Can this function take a StringRef and return a std::string instead? The amount of roundtrips between StringRefs, ConstStrings and std::strings is getting a bit out of hand.

[xujuntwt95329]

lldb/source/Target/PathMappingList.cpp

33–37

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

[xujuntwt95329]

rebase

[aprantl]

lldb/source/Target/PathMappingList.cpp

33

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?

33–37

IMO, the best solution would be change this too:

void PathMappingList::Append(StringRef path,
                             StringRef replacement, bool notify) {

[xujuntwt95329]

rebase

[xujuntwt95329]

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!

lldb/source/Target/PathMappingList.cpp

33

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

[aprantl]

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!

[teemperor]

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:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective projects list set()

[xujuntwt95329]

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:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective 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?

[teemperor]

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:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective 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.

[xujuntwt95329]

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:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective 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!

[teemperor]

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:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective 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<FileSpec> 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.

[xujuntwt95329]

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:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective 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<FileSpec> 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

[teemperor]

Fixed in 58dd658583eec9af24ca1262e1bce9f884d65487

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

lldb/unittests/Target/FindFileTest.cpp

28

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

29

No one is calling this IIUC?

30

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

37

You can simplify this code by giving this test struct a member SubsystemRAII<FileSystem, HostInfo> 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:

struct FindFileTest : public testing::Test {
  SubsystemRAII<FileSystem, HostInfo> subsystems;
};

59

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.

60

EXPECT_EQ (StringRef is supported in the gtest macros)

[xujuntwt95329]

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！

lldb/unittests/Target/FindFileTest.cpp

37

You can simplify this code by giving this test struct a member SubsystemRAII<FileSystem, HostInfo> 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:

struct FindFileTest : public testing::Test {
  SubsystemRAII<FileSystem, HostInfo> subsystems;
};

[teemperor]

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.

[xujuntwt95329]

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

Thanks

lldb/source/Target/PathMappingList.cpp

namespace {
   // things won't always work. We found a case where if we did:
   // (lldb) settings set target.source-map . /tmp
   // We would store a path pairs of "." and "/tmp" as raw strings. If the debug
   // info contains "./foo/bar.c", the path will get normalized to "foo/bar.c".
   // When PathMappingList::RemapPath() is called, it expects the path to start
   // 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());
   }
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
 
 PathMappingList::PathMappingList(ChangedCallback callback, void *callback_baton)
     : m_pairs(), m_callback(callback), m_callback_baton(callback_baton),
       m_mod_id(0) {}
 
for (const auto &it : m_pairs) {
     fixed.SetFile(orig_file, orig_style);
     AppendPathComponents(fixed, path_ref, orig_style);
     return true;
   }
   return false;
 }
 
 llvm::Optional<FileSpec> 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 {};
 }
 
 bool PathMappingList::Replace(ConstString path,
                               ConstString new_path, bool notify) {
   uint32_t idx = FindIndexForPath(path);

lldb/unittests/Target/CMakeLists.txt

 add_lldb_unittest(TargetTests
   ABITest.cpp
   DynamicRegisterInfoTest.cpp
   ExecutionContextTest.cpp
   MemoryRegionInfoTest.cpp
   ModuleCacheTest.cpp
   PathMappingListTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
+  FindFileTest.cpp
 
   LINK_LIBS
       lldbCore
       lldbHost
       lldbPluginObjectFileELF
       lldbPluginPlatformLinux
       lldbPluginSymbolFileSymtab
       lldbTarget
       lldbSymbol
       lldbUtility
       lldbUtilityHelpers
     LINK_COMPONENTS
       Support
   )
 
 add_unittest_inputs(TargetTests TestModule.so)

lldb/unittests/Target/FindFileTest.cpp

+//===-- 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 <utility>
+
+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> matches,
+                             llvm::ArrayRef<FileSpec> 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<FileSpec> 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<FileSpec> 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);
+}