[Lldb-commits] [PATCH] D149096: [lldb] Speed up looking for dSYM next to executable

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:89-91
+  dsym_file.append(".dSYM");
+  llvm::sys::path::append(dsym_file, path_style, "Contents", "Resources",
+                          "DWARF", filename);
----------------
mib wrote:
> Why do you need the first call to `append` ?
The code suggestion you've given doesn't work for 2 reasons:
- `llvm::sys::path::append` only takes 4 Twine arguments, so this won't compile.
- `llvm::sys::path::append` appends things as if they were paths. So for `/bin/ls` you'll end up with `/bin/ls/.dSYM/Contents/Resources/DWARF` instead of `/bin/ls.dSYM/Contents/Resources/DWARF`.


================
Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:102
 
-    // See if we have "../CF.framework" - so we'll look for
-    // CF.framework.dSYM/Contents/Resources/DWARF/CF
-    // We need to drop the last suffix after '.' to match
-    // 'CF' in the DWARF subdir.
-    std::string binary_name(filename.AsCString());
-    auto last_dot = binary_name.find_last_of('.');
-    if (last_dot != std::string::npos) {
-      binary_name.erase(last_dot);
-      dsym_fspec = dsym_directory;
-      dsym_fspec.AppendPathComponent(binary_name);
-      if (FileSystem::Instance().Exists(dsym_fspec) &&
-          FileAtPathContainsArchAndUUID(dsym_fspec,
-                                        mod_spec.GetArchitecturePtr(),
-                                        mod_spec.GetUUIDPtr())) {
-        return true;
+  // See if we have ".../CF.framework" - so we'll look for
+  // CF.framework.dSYM/Contents/Resources/DWARF/CF
----------------
mib wrote:
> Is this extra dot on purpose ?
I wanted to distinguish between `$parent_dir/CF.framework` because what is the parent_dir in this case? I used 3 dots to indicate `$some_path/CF.framework`.


================
Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:105-108
+  if (filename.endswith(".framework")) {
+    const auto last_dot = dsym_file.find_last_of('.');
+    if (last_dot != llvm::StringRef::npos) {
+      dsym_file.truncate(last_dot - 1);
----------------
mib wrote:
> This part is a bit confusing ... may be a comment would help understand what we're trying to achieve here.
I'll add more to the comment.


================
Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:175-178
+    llvm::StringRef filename =
+        llvm::sys::path::filename(parent_path, path_style);
+    if (filename.find('.') == llvm::StringRef::npos)
+      continue;
----------------
mib wrote:
> Same here ... this could use a comment to explain what we're doing.
Will add.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149096/new/

https://reviews.llvm.org/D149096