This is an archive of the discontinued LLVM Phabricator instance.

Implement better path matching in FileSpecList::FindCompatibleIndex(...).
ClosedPublic

Authored by clayborg on Jul 22 2022, 3:17 PM.

Details

Summary

Currently a FileSpecList::FindFileIndex(...) will only match the specified FileSpec if:

  • it has filename and directory and both match exactly
  • if has a filename only and any filename in the list matches

Because of this, we modify our breakpoint resolving so it can handle relative paths by doing some extra code that removes the directory from the FileSpec when searching if the path is relative.

This patch is intended to fix breakpoints so they work as users expect them to by adding the following features:

  • allow matches to relative paths in the file list to match as long as the relative path is at the end of the specified path at valid directory delimiters
  • allow matches to paths to match if the specified path is relative and shorter than the file paths in the list

This allows us to remove the extra logic from BreakpointResolverFileLine.cpp that added support for setting breakpoints with relative paths.

This means we can still set breakpoints with relative paths when the debug info contains full paths. We add the ability to set breakpoints with full paths when the debug info contains relative paths.

Debug info contains "./a/b/c/main.cpp", the following will set breakpoints successfully:

  • /build/a/b/c/main.cpp
  • a/b/c/main.cpp
  • b/c/main.cpp
  • c/main.cpp
  • main.cpp
  • ./c/main.cpp
  • ./a/b/c/main.cpp
  • ./b/c/main.cpp
  • ./main.cpp

This also ensures that we won't match partial directory names, if a relative path is in the list or is used for the match, things must match at the directory level.

The breakpoint resolving code will now use the new FileSpecList::FindCompatibleIndex(...) function to allow this fuzzy matching to work for breakpoints.

Diff Detail

Event Timeline

clayborg created this revision.Jul 22 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 3:17 PM
clayborg requested review of this revision.Jul 22 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald Transcript
labath added inline comments.Jul 25 2022, 5:29 AM
lldb/include/lldb/Core/FileSpecList.h
140–141

Is this ever being called with full=false? If not can we drop it?

lldb/include/lldb/Utility/FileSpec.h
219–224

Are you sure about this part? It is my impression that we're already storing the windows paths in the "normalized" form (using / as separator), and so there shouldn't be a need to do anything special (at least regarding directory separators) when working with windows paths.

lldb/source/Core/FileSpecList.cpp
93

use lower case?

121–141

This could be a lot less repetitive. Perhaps something like:

const bool comparison_case_sensitive = file_spec_case_sensitive || curr_file.IsCaseSensitive(); // I changed this from && to || as that's the logic used elsewhere
auto &is_suffix = [&](StringRef a, StringRef b) {
  return a.consume_back(b) && (a.empty() || a.endswith("/"));
};
if (is_suffix(curr_file_dir, file_spec_dir) || is_suffix(file_spec_dir, curr_file_dir))
  return idx;
lldb/unittests/Core/FileSpecListTest.cpp
20–22

It would be nice to have a quick windows test as well, to confirm the separator story.

clayborg added inline comments.Jul 25 2022, 4:31 PM
lldb/include/lldb/Core/FileSpecList.h
140–141

Probably yes!

lldb/include/lldb/Utility/FileSpec.h
219–224

I can check to make sure by adding some unit tests and if so, I can drop this

lldb/source/Core/FileSpecList.cpp
93

I will reword

121–141

Good idea

lldb/unittests/Core/FileSpecListTest.cpp
20–22

will do!

clayborg updated this revision to Diff 447523.Jul 25 2022, 4:54 PM
  • Added windows tests to make sure FileSpecList::FindCompatibleIndex() works on windows
  • Use lambdas to simplify code in FileSpecList::FindCompatibleIndex()
  • Remove the "full" argument from FindCompatibleIndex
  • Reword the comment that explains when we will use a filename match only.
clayborg marked 4 inline comments as done.Jul 25 2022, 4:55 PM

Marking things as done

labath added inline comments.Jul 27 2022, 5:24 AM
lldb/include/lldb/Utility/FileSpec.h
219–224

Cool. Can we also revert this part now that it's not necessary? (Mainly because it's misleading as there is more than one valid directory separator on windows -- that's why the old functions used the term '"preferred" separator').

lldb/source/Core/FileSpecList.cpp
119

Change && to ||, as that's how e.g. FileSpec::DirectoryEquals is implemented. (I actually think && makes more sense, but I think we should be consistent)

121–141

Could you also deduplicate along the case-insenstiveness branches?
(This is sort of where I was going with the original snippet, but then I forgot about it by the time I got to the end of it. I wanted to write something like

bool consumed = comparison_case_sensitive ? a.consume_back(b) : a.consume_back_insensitive(b);
return consumed && (a.empty() || a.endswith("/"));
clayborg added inline comments.Jul 27 2022, 1:44 PM
lldb/include/lldb/Utility/FileSpec.h
219–224

yes I can.

lldb/source/Core/FileSpecList.cpp
119

yep!

121–141

sure thing

clayborg updated this revision to Diff 448436.Jul 28 2022, 2:09 PM

Revert FileSpec::GetPathSeparator() changes that are no longer needed and respond to other imrpovement feedback.

clayborg marked 2 inline comments as done.Jul 28 2022, 2:10 PM
labath accepted this revision.Aug 1 2022, 4:50 AM
This revision is now accepted and ready to land.Aug 1 2022, 4:50 AM