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

[Zachary Turner via lldb-commits]

zturner requested changes to this revision.
zturner added a comment.
This revision now requires changes to proceed.

I know you are just following the existing pattern, but this whole function appears broken in the presence of file systems that use backslash instead of forward slash.  Could you try to fix that?  I've provided some suggestions inline.

I also don't see any tests for this.  We've been pretty lax about this historically, but I want to start pushing back against CLs with no tests.  If you need help writing a test, let me know and we can figure something out.  For starters, what is the scenario you encountered this in?


================
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";
         }
 
----------------
Can you do all of this only if the target is not 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);
 
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D13201