This is an archive of the discontinued LLVM Phabricator instance.

normalize file path when searching the source map
ClosedPublic

Authored by xujuntwt95329 on Oct 25 2021, 4:55 AM.

Details

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

Diff Detail

Event Timeline

xujuntwt95329 requested review of this revision.Oct 25 2021, 4:55 AM
xujuntwt95329 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 4:55 AM
wallace accepted this revision.Oct 25 2021, 10:16 AM

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

This revision is now accepted and ready to land.Oct 25 2021, 10:16 AM

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

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

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.

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 added inline comments.Oct 26 2021, 9:42 AM
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 added inline comments.Oct 26 2021, 7:56 PM
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

aprantl added inline comments.Oct 29 2021, 11:32 AM
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 marked 2 inline comments as done.Oct 29 2021, 11:23 PM

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 accepted this revision.Nov 1 2021, 3:59 PM

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!

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

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?

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.

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 added a comment.EditedNov 2 2021, 4:27 AM

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.

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

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)

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

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 marked 7 inline comments as done.Nov 2 2021, 6:23 AM

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

Thanks