Resolve DW_AT_comp_dir path if it contains a symlink

[ovyalov]

Summary

Value of DW_AT_comp_dir entry can be enforced by PWD environment variable and set with symbolic link.
If DW_AT_comp_dir is a symlink use FileSystem::Readlink to get full path.

[chaoren]

LGTM.

[jingham]

Why do you need to do this? Between the comp dir and the file name you should have an absolute path. If the debug information is not resolved to an absolute path, that's a compiler bug and should be fixed there.

The debugger does not require that the source tree still exists, so you can't base any logic on this ACTUALLY existing to avoid a potentially large number of pointless stat's. And in general, unless there's some clear advantage, I'd rather we not touch the file system for source files until somebody actually wants to find a particular source file.

[ovyalov]

In D11357#208345, @jingham wrote:

Why do you need to do this? Between the comp dir and the file name you should have an absolute path. If the debug information is not resolved to an absolute path, that's a compiler bug and should be fixed there.

Binary can be built on remote machine (in case of distributed build) and in this case DW_AT_comp_dir entry contains useless and invalid path for a host which initiated a build.
Problem similar to the issue described here - http://marc.info/?l=cfe-dev&m=131916863602489. GCC puts /proc/self/cwd as a value of DW_AT_comp_dir so host can deduce it locally to own path.

The debugger does not require that the source tree still exists, so you can't base any logic on this ACTUALLY existing to avoid a potentially large number of pointless stat's. And in general, unless there's some clear advantage, I'd rather we not touch the file system for source files until somebody actually wants to find a particular source file.

I agree that numeric stat calls may pose a significant IO overhead - as a workaround, we can introduce a new property which enables option symbolic link resolution.

[clayborg]

Maybe we can look for these magic values and only resolve the symlink if they match? Is it currently only "/proc/self/cwd" that we would need to look for? stat-ing a ton of file can be very expensive as Jim said and we should strive to avoid it if at all possible.

[ovyalov]

Added static array of possible symbolic link values initialized with "/proc/self/cwd".

[clayborg]

Looks good.

[ovyalov]

Files:

/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Users:

ovyalov (Author)

http://reviews.llvm.org/rL242757

[jingham]

As a followup, can you make this non-exported static array of names that are pretty much only interesting on Linux into something queried from the platform? That way we could provide a way for the user to augment the list to add a new directory thats relevant, or even turn the stat behavior off by clearing the array, etc...

[jingham]

There should also be a test for this if you can manage to make one, even if it only runs on Linux, or this is likely to break.

[emaste]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

153

This is Linux-specific, it shouldn't be done unconditionally in here

[emaste]

In D11357#212775, @jingham wrote:

As a followup, can you make this non-exported static array of names that are pretty much only interesting on Linux into something queried from the platform? That way we could provide a way for the user to augment the list to add a new directory thats relevant, or even turn the stat behavior off by clearing the array, etc...

Oh, I didn't notice this when I wrote my note. Agreed.

[ovyalov]

Thanks for suggestions - will address them shortly.

[clayborg]

So a few rules that we must follow:

• lldb_private::Module and the contained lldb_private::ObjectFile and lldb_private::SymbolVendor/lldb_private::SymbolFile are never associated with a Target or Process unless they are loaded from memory and there is no backing file. This would be the typical way to get a platform, but that options isn't available. Why? We put all modules into a global cache that means multiple processes can all use the same module for libc.so. So there can be no ties to a target/process in any fix we do.
• Since we have no target/process we must rely on static function calls in the platforms or settings to fix this. One option is to make a setting, something like "plugin.symbol-file.dwarf.symlink-paths" which could be an array that could contain these items. This would allow people to add new ones, but this doesn't limit the checks to linux only. The setting could be a dictionary where the key is a triple (like "*-*-linux") and the value is a string which is the path to convert.
• Another option is to find the platform by using the triple from the symbol file and then asking the platform for the list to use.

I think I like the last option, but this options doesn't guarantee that we get the Platform that you might have created for your process.

[jingham]

Thinking about this a little more, it seems like this should really be a host facility, not a platform one. After all, in order for this bit to do its job, a realpath or its equivalent must succeed on the local system. So it really is a host feature. That makes figuring out where to put it a lot easier, but I also think it reflects better what is going on here.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

 #include "lldb/Core/Section.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Core/Timer.h"
 #include "lldb/Core/Value.h"
 
 #include "lldb/Expression/ClangModulesDeclVendor.h"
 
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/ClangExternalASTSourceCallbacks.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
if (
         '\\' == path_from_dwarf[2])
     {
         return path_from_dwarf;
     }
 
     return colon_pos + 1;
 }
 
+// DW_AT_comp_dir can be overridden by setting compiler's PWD environment
+// variable - for example, set to "/proc/self/cwd" in order to cope with
+// distributed builds.
+static const char* comp_dir_symlinks[] = {"/proc/self/cwd"};
+
+static const char*
+resolveCompDir(const char* path_from_dwarf)
+{
+    if (!path_from_dwarf)
+        return nullptr;
+
+    // DWARF2/3 suggests the form hostname:pathname for compilation directory.
+    // Remove the host part if present.
+    const char* local_path = removeHostnameFromPathname(path_from_dwarf);
+    if (!local_path)
+        return nullptr;
+
+    bool is_symlink = false;
+    for (unsigned long i = 0; i < sizeof(comp_dir_symlinks)/sizeof(comp_dir_symlinks[0]) && !is_symlink; ++i)
+        is_symlink = !strcmp(comp_dir_symlinks[i], local_path);
+
+    if (!is_symlink)
+        return local_path;
+
+    const FileSpec local_path_spec(local_path, true);
+    if (!local_path_spec.IsSymbolicLink())
+        return local_path;
+
+    FileSpec resolved_local_path_spec;
+    const auto error = FileSystem::Readlink(local_path_spec, resolved_local_path_spec);
+    if (error.Success())
+        return resolved_local_path_spec.GetCString();
+
+    return nullptr;
+}
+
+
 #if defined(LLDB_CONFIGURATION_DEBUG) || defined(LLDB_CONFIGURATION_RELEASE)
 
 class DIEStack
 {
 public:
     
     void Push (DWARFCompileUnit *cu, const DWARFDebugInfoEntry *die)
     {
if (dwarf_cu)
                     {
                         FileSpec cu_file_spec{cu_die->GetName(this, dwarf_cu), false};
                         if (cu_file_spec)
                         {
                             // If we have a full path to the compile unit, we don't need to resolve
                             // the file.  This can be expensive e.g. when the source files are NFS mounted.
                             if (cu_file_spec.IsRelative())
                             {
-                                // DWARF2/3 suggests the form hostname:pathname for compilation directory.
-                                // Remove the host part if present.
                                 const char *cu_comp_dir{cu_die->GetAttributeValueAsString(this, dwarf_cu, DW_AT_comp_dir, nullptr)};
-                                cu_file_spec.PrependPathComponent(removeHostnameFromPathname(cu_comp_dir));
+                                cu_file_spec.PrependPathComponent(resolveCompDir(cu_comp_dir));
                             }
 
                             std::string remapped_file;
                             if (module_sp->RemapSourceFile(cu_file_spec.GetCString(), remapped_file))
                                 cu_file_spec.SetFile(remapped_file, false);
                         }
 
                         LanguageType cu_language = DWARFCompileUnit::LanguageTypeFromDWARF(cu_die->GetAttributeValueAsUnsigned(this, dwarf_cu, DW_AT_language, 0));
SymbolFileDWARF::ParseCompileUnitSupportFiles (const SymbolContext& sc, FileSpecList& support_files)
     assert (sc.comp_unit);
     DWARFCompileUnit* dwarf_cu = GetDWARFCompileUnit(sc.comp_unit);
     if (dwarf_cu)
     {
         const DWARFDebugInfoEntry * cu_die = dwarf_cu->GetCompileUnitDIEOnly();
 
         if (cu_die)
         {
-            const char * cu_comp_dir = cu_die->GetAttributeValueAsString(this, dwarf_cu, DW_AT_comp_dir, NULL);
-
-            // DWARF2/3 suggests the form hostname:pathname for compilation directory.
-            // Remove the host part if present.
-            cu_comp_dir = removeHostnameFromPathname(cu_comp_dir);
+            const char * cu_comp_dir = resolveCompDir(cu_die->GetAttributeValueAsString(this, dwarf_cu, DW_AT_comp_dir, nullptr));
 
             dw_offset_t stmt_list = cu_die->GetAttributeValueAsUnsigned(this, dwarf_cu, DW_AT_stmt_list, DW_INVALID_OFFSET);
 
             // All file indexes in DWARF are one based and a file of index zero is
             // supposed to be the compile unit itself.
             support_files.Append (*sc.comp_unit);
 
             return DWARFDebugLine::ParseSupportFiles(sc.comp_unit->GetModule(), get_debug_line_data(), cu_comp_dir, stmt_list, support_files);