[Lldb-commits] [PATCH] D13201: Fix segmentation fault in lldb_private::Symbols::LocateExecutableSymbolFile()

[Vadim Macagon via lldb-commits]

enlight added a comment.

I've updated the summary with the scenario. I don't know how one would go about writing a test for this, mixed platform remote debugging is a bit finicky.


================
Comment at: source/Host/common/Symbols.cpp:238-250
@@ -237,15 +237,15 @@
 
         // Add /usr/lib/debug directory.
         debug_file_search_paths.AppendIfUnique (FileSpec("/usr/lib/debug", true));
 
         std::string uuid_str;
         const UUID &module_uuid = module_spec.GetUUID();
         if (module_uuid.IsValid())
         {
             // Some debug files are stored in the .build-id directory like this:
             //   /usr/lib/debug/.build-id/ff/e7fe727889ad82bb153de2ad065b2189693315.debug
             uuid_str = module_uuid.GetAsString("");
             uuid_str.insert (2, 1, '/');
             uuid_str = uuid_str + ".debug";
         }
 
----------------
zturner wrote:
> Can you do all of this only if the target is not Windows?
You mean if the **host** is not Windows? Skipping `/usr/lib/debug` on Windows makes sense, but it seems like the `uuid_str` stuff could still apply if the symbol files are copied to or shared with Windows.

================
Comment at: source/Host/common/Symbols.cpp:263-270
@@ -266,6 +262,10 @@
 
             files.push_back (dirname + "/" + symbol_filename);
             files.push_back (dirname + "/.debug/" + symbol_filename);
             files.push_back (dirname + "/.build-id/" + uuid_str);
-            files.push_back (dirname + module_directory + "/" + symbol_filename);
+
+            // Some debug files may stored in the module directory like this:
+            //   /usr/lib/debug/usr/lib/library.so.debug
+            if (!file_dir.IsEmpty())
+                files.push_back (dirname + file_dir.AsCString() + "/" + symbol_filename);
 
----------------
zturner wrote:
> Don't use literal slashes anywhere.  I know you're just copying the existing code, which was already broken, but it seems easy enough to fix all this by calling llvm::sys::path::append, which will do the right thing dependign on the Host platform.
> 
> Since this code is in Host, it's ok to do this, since we're ultimately going to pass this path directly through to the filesystem anyway, we already need to be able to assume that we're dealing with a path that has the appropriate separator for the platform anyway.
I'll submit a separate patch for the requested path separator changes. I'd prefer to do it after this patch though.


Repository:
  rL LLVM

http://reviews.llvm.org/D13201