Breakpad: auto-detect path style of file entries

[labath]

Summary

This adds support for auto-detection of path style to SymbolFileBreakpad
(similar to how r351328 did the same for DWARF). We guess each file
entry separately, as we have no idea which file came from which compile
units (and different compile units can have different path styles). The
breakpad generates should have already converted the paths to absolute
ones, so this guess should be reasonable accurate, but as always with
these kinds of things, it is hard to give guarantees about anything.

In an attempt to bring some unity to the path guessing logic, I move the
guessing logic from inside SymbolFileDWARF into the FileSpec class and
have both symbol files use it to implent their desired behavior.

[JDevlieghere]

LGTM

[amccarth]

include/lldb/Utility/FileSpec.h

250

Given that this implementation is limited to absolute paths ... should it be in such a general purpose class?

If so, maybe this restriction be made more obvious by naming the parameter absolute_path. Then people who just look at the signature without reading the comments, or who get a prompt in their IDE are more likely to note the limitation.

source/Utility/FileSpec.cpp

375

The code this replaces was returning Style::native rather than Style::None. I'm not sure if that will have any implications.

[labath]

include/lldb/Utility/FileSpec.h

250

Do you have an idea where to place it? Theoretically I could put it into the base SymbolFile class, since all existing users are SymbolFiles, but I am not sure if that makes things any better. Renaming the arg to absolute_path sounds good.

source/Utility/FileSpec.cpp

375

It terms of functionality, it doesn't, since all users then do getValueOr(Style::native). But I am not sure what's the impact on readability -- it seemed better to have a more explicit value for "I don't know" in such a general API, but since all users just revert to native when they can't guess (and I'm not sure if there's anything better they could do), maybe going through Optional is overkill. I'd be happy to change it if you think that's better.

[labath]

Rename arg to absolute_path.

[amccarth]

I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.

[labath]

In D57895#1390641, @amccarth wrote:

I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.

Thanks. BTW, I've added the auto-detection code to dwarf only a couple of weeks ago, so I think we're more likely to get latent surprises from that then from this move. :)

include/lldb/Utility/FileSpec.h

public:
   ///     @li -1 if \a lhs is less than \a rhs
   ///     @li 0 if \a lhs is equal to \a rhs
   ///     @li 1 if \a lhs is greater than \a rhs
   //------------------------------------------------------------------
   static int Compare(const FileSpec &lhs, const FileSpec &rhs, bool full);
 
   static bool Equal(const FileSpec &a, const FileSpec &b, bool full);
 
+  /// Attempt to guess path style for a given path string. It returns a style,
+  /// if it was able to make a reasonable guess, or None if it wasn't. The guess
+  /// will be correct if the input path was a valid absolute path on the system
+  /// which produced it. On other paths the result of this function is
+  /// unreliable (e.g. "c:\foo.txt" is a valid relative posix path).
+  static llvm::Optional<Style> GuessPathStyle(llvm::StringRef absolute_path);
+
   //------------------------------------------------------------------
   /// Case sensitivity of path.
   ///
   /// @return
   ///     \b true if the file path is case sensitive (POSIX), false
   ///   if case insensitive (Windows).
   //------------------------------------------------------------------
   bool IsCaseSensitive() const { return m_style != Style::windows; }

lit/SymbolFile/Breakpad/Inputs/line-table-mixed-path-styles.syms

+MODULE Linux x86_64 761550E08086333960A9074A9CE2895C0 a.out
+INFO CODE_ID E05015768680393360A9074A9CE2895C
+FILE 0 /tmp/a.c
+FILE 1 c:\tmp\b.c
+FUNC b0 10 0 func
+b0 1 1 0
+b1 1 1 1

lit/SymbolFile/Breakpad/line-table-discontinuous-file-ids.test

 # Test that we handle files which has gaps in the FILE record IDs.
 
-# XFAIL: system-windows
-
 # RUN: yaml2obj %S/Inputs/basic-elf.yaml > %T/line-table-discontinuous-file-ids.out
 # RUN: %lldb %T/line-table-discontinuous-file-ids.out \
 # RUN:   -o "target symbols add -s line-table-discontinuous-file-ids.out %S/Inputs/line-table-discontinuous-file-ids.syms" \
 # RUN:   -s %s -o exit | FileCheck %s
 
 image dump line-table a.c
 # CHECK-LABEL: Line table for /tmp/a.c
 # CHECK-NEXT: 0x00000000004000b0: /tmp/a.c:1
 # CHECK-NEXT: 0x00000000004000b1: /tmp/a.c:2
 # CHECK-NEXT: 0x00000000004000b2: /tmp/c.c:2
 # CHECK-NEXT: 0x00000000004000b3: 

lit/SymbolFile/Breakpad/line-table-edgecases.test

 # Test handling of breakpad files with some unusual or erroneous constructs. The
 # input contains a LINE record which does not belong to any function as well as
 # a FUNC record without any LINE records.
 
-# XFAIL: system-windows
-
 # RUN: yaml2obj %S/Inputs/basic-elf.yaml > %T/line-table-edgecases.out
 # RUN: %lldb %T/line-table-edgecases.out \
 # RUN:   -o "target symbols add -s line-table-edgecases.out %S/Inputs/line-table-edgecases.syms" \
 # RUN:   -s %s -o exit | FileCheck %s
 
 # Test that line table for func2 was parsed properly:
 image dump line-table a.c
 # CHECK-LABEL: Line table for /tmp/a.c

lit/SymbolFile/Breakpad/line-table-missing-file.test

 # Test that we do something reasonable if a LINE record references a
 # non-existing FILE record.
 # Right now, "something reasonable" means creating a line entry with an empty
 # file.
 
-# XFAIL: system-windows
-
 # RUN: yaml2obj %S/Inputs/basic-elf.yaml > %T/line-table-missing-file.out
 # RUN: %lldb %T/line-table-missing-file.out \
 # RUN:   -o "target symbols add -s line-table-missing-file.out %S/Inputs/line-table-missing-file.syms" \
 # RUN:   -s %s -o exit | FileCheck %s
 
 image dump line-table a.c
 # CHECK-LABEL: Line table for /tmp/a.c
 # CHECK-NEXT: 0x00000000004000b0: /tmp/a.c:1
 # CHECK-NEXT: 0x00000000004000b1: /tmp/a.c:2
 # CHECK-NEXT: 0x00000000004000b2: :2
 # CHECK-NEXT: 0x00000000004000b3: 
 # CHECK-EMPTY: 

lit/SymbolFile/Breakpad/line-table-mixed-path-styles.test

+# RUN: yaml2obj %S/Inputs/basic-elf.yaml > %T/line-table-mixed-path-styles.out
+# RUN: %lldb %T/line-table-mixed-path-styles.out \
+# RUN:   -o "target symbols add -s line-table-mixed-path-styles.out %S/Inputs/line-table-mixed-path-styles.syms" \
+# RUN:   -s %s -o exit | FileCheck %s
+
+image dump line-table a.c
+# CHECK-LABEL: Line table for /tmp/a.c
+# CHECK-NEXT: 0x00000000004000b0: /tmp/a.c:1
+# CHECK-NEXT: 0x00000000004000b1: c:\tmp\b.c:1
+# CHECK-NEXT: 0x00000000004000b2:
+# CHECK-EMPTY:

lit/SymbolFile/Breakpad/line-table.test

-# XFAIL: system-windows
-
 # RUN: yaml2obj %S/Inputs/basic-elf.yaml > %T/line-table.out
 # RUN: %lldb %T/line-table.out -o "target symbols add -s line-table.out %S/Inputs/line-table.syms" \
 # RUN:   -s %s -o exit | FileCheck %s
 
 # We create a compile unit for each function. The compile unit name is the first
 # line table entry in that function.
 # This symbol file contains a single function in the "compile unit" a.c. This
 # function has two line table sequences.

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

for (llvm::StringRef line : lines(Record::File)) {
     auto record = FileRecord::parse(line);
     if (!record) {
       LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
       continue;
     }
 
     if (record->Number >= m_files->size())
       m_files->resize(record->Number + 1);
-    (*m_files)[record->Number] = FileSpec(record->Name);
+    FileSpec::Style style = FileSpec::GuessPathStyle(record->Name)
+                                .getValueOr(FileSpec::Style::native);
+    (*m_files)[record->Number] = FileSpec(record->Name, style);
   }
 }
 
 void SymbolFileBreakpad::ParseCUData() {
   if (m_cu_data)
     return;
 
   m_cu_data.emplace();

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

 }
 
 void DWARFUnit::ComputeCompDirAndGuessPathStyle() {
   m_comp_dir = FileSpec();
   const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly();
   if (!die)
     return;
 
-  auto guess = [](llvm::StringRef str) {
-    if (str.startswith("/"))
-      return FileSpec::Style::posix;
-    if (str.size() > 3 && llvm::isAlpha(str[0]) && str.substr(1, 2) == ":\\")
-      return FileSpec::Style::windows;
-    return FileSpec::Style::native;
-  };
   llvm::StringRef comp_dir = removeHostnameFromPathname(
       die->GetAttributeValueAsString(m_dwarf, this, DW_AT_comp_dir, NULL));
   if (!comp_dir.empty()) {
-    m_comp_dir = resolveCompDir(FileSpec(comp_dir, guess(comp_dir)));
+    FileSpec::Style comp_dir_style =
+        FileSpec::GuessPathStyle(comp_dir).getValueOr(FileSpec::Style::native);
+    m_comp_dir = resolveCompDir(FileSpec(comp_dir, comp_dir_style));
   } else {
     // Try to detect the style based on the DW_AT_name attribute, but just store
     // the detected style in the m_comp_dir field.
-    m_comp_dir = FileSpec("", guess(die->GetAttributeValueAsString(
-                                  m_dwarf, this, DW_AT_name, NULL)));
+    const char *name =
+        die->GetAttributeValueAsString(m_dwarf, this, DW_AT_name, NULL);
+    m_comp_dir = FileSpec(
+        "", FileSpec::GuessPathStyle(name).getValueOr(FileSpec::Style::native));
   }
 }
 
 SymbolFileDWARFDwo *DWARFUnit::GetDwoSymbolFile() const {
   return m_dwo_symbol_file.get();
 }
 
 dw_offset_t DWARFUnit::GetBaseObjOffset() const { return m_base_obj_offset; }

source/Utility/FileSpec.cpp

if (!filenames_equal)
     return false;
 
   if (!full && (a.GetDirectory().IsEmpty() || b.GetDirectory().IsEmpty()))
     return filenames_equal;
 
   return a == b;
 }
 
+llvm::Optional<FileSpec::Style> FileSpec::GuessPathStyle(llvm::StringRef absolute_path) {
+  if (absolute_path.startswith("/"))
+    return Style::posix;
+  if (absolute_path.startswith(R"(\\)"))
+    return Style::windows;
+  if (absolute_path.size() > 3 && llvm::isAlpha(absolute_path[0]) &&
+      absolute_path.substr(1, 2) == R"(:\)")
+    return Style::windows;
+  return llvm::None;
+}
+
 //------------------------------------------------------------------
 // Dump the object to the supplied stream. If the object contains a valid
 // directory name, it will be displayed followed by a directory delimiter, and
 // the filename.
 //------------------------------------------------------------------
 void FileSpec::Dump(Stream *s) const {
   if (s) {
     std::string path{GetPath(true)};

unittests/Utility/FileSpecTest.cpp

TEST(FileSpecTest, EqualDotsPosixRoot) {
 
   for (const auto &test : tests) {
     FileSpec one(test.first, FileSpec::Style::posix);
     FileSpec two(test.second, FileSpec::Style::posix);
     EXPECT_EQ(one, two);
   }
 }
 
+TEST(FileSpecTest, GuessPathStyle) {
+  EXPECT_EQ(FileSpec::Style::posix, FileSpec::GuessPathStyle("/foo/bar.txt"));
+  EXPECT_EQ(FileSpec::Style::posix, FileSpec::GuessPathStyle("//net/bar.txt"));
+  EXPECT_EQ(FileSpec::Style::windows,
+            FileSpec::GuessPathStyle(R"(C:\foo.txt)"));
+  EXPECT_EQ(FileSpec::Style::windows,
+            FileSpec::GuessPathStyle(R"(\\net\foo.txt)"));
+  EXPECT_EQ(llvm::None, FileSpec::GuessPathStyle("foo.txt"));
+  EXPECT_EQ(llvm::None, FileSpec::GuessPathStyle("foo/bar.txt"));
+}
+
 TEST(FileSpecTest, GetNormalizedPath) {
   std::pair<const char *, const char *> posix_tests[] = {
       {"/foo/.././bar", "/bar"},
       {"/foo/./../bar", "/bar"},
       {"/foo/../bar", "/bar"},
       {"/foo/./bar", "/foo/bar"},
       {"/foo/..", "/"},
       {"/foo/.", "/foo"},