Resolve symlinks when looking for the LLDB shlib

[spyffe]

Summary

When running the testsuite, the LLDB shlib is a symlink to its actual location, so finding shlib-relative resources can be problematic. This patch fixes that by resolving symlinks.

[clayborg]

include/lldb/Host/FileSpec.h

516

rename to ResolveSymbolicLink?

source/Host/common/FileSpec.cpp

818

rename to ResolveSymbolicLink() and return a copy of this object if it isn't a symbolic link

source/Host/common/HostInfoBase.cpp

310–312

No need to check if we change the behavior of ResolveSymbolicLink, this code will just be:

lldb_file_spec = lldb_file_spec.ResolveSymbolicLink();

[spyffe]

Updated to reflect Greg's comments.

[clayborg]

Looks good.

[zturner]

Can you move this to FileSystem.h? I don't think we should be adding more
things that hit the file system to FileSpec. That's exactly the reason
FileSystem.h exists, because many of the operations will be implemented
differently across platforms, so we should be using the Host layer.

[zturner]

Furthermore, FileSpec can refer to a remote path, so you can't even
guarantee that the OS you're on is the same OS as that which the path
refers to. So another reason why putting this in FileSpec doesn't make
sense in my opinion.

[spyffe]

Sure. On it.

Sean

[zturner]

Thanks! Feel free to leave the method in Host/windows/FileSystem.cpp
empty, i'll fill it out.

[spyffe]

At zturner's suggestion, moved the function to FileSystem and renamed it to be more consistent with what other FileSystem functions are called.

[zturner]

If you change the name back to ResolveSymbolicLink or GetSymbolicLinkTarget, then this looks fine.

include/lldb/Host/FileSystem.h

43

I'd rather not call it realpath, because even though realpath is the way it will be implemented on non-Windows platforms, realpath has some subtleties in its semantics. So anyone reading this function name will expect that it has realpath semantics, even though some of the semantics are only well-defined in the context of a posix-like file system. I actually like your original name ResolveSymbolicLink better, because the behavior is narrow enough that it's easy to implement everywhere, and we don't have to worry about these little edge cases.

[spyffe]

Restored the old name based on zturner's suggestion that Realpath() is too specific and has semantics that Windows wouldn't honor.

[zturner]

Thanks!

include/lldb/Host/FileSpec.h

 
     bool
     IsSymbolicLink () const;
-    
-    FileSpec
-    ResolveSymbolicLink () const;
 
     //------------------------------------------------------------------
     /// Get the memory cost of this object.

include/lldb/Host/FileSystem.h

     static Error Symlink(const FileSpec &src, const FileSpec &dst);
     static Error Readlink(const FileSpec &src, FileSpec &dst);
     static Error Unlink(const FileSpec &file_spec);
+    
+    static Error ResolveSymbolicLink(const FileSpec &src, FileSpec &dst);
 
     static bool CalculateMD5(const FileSpec &file_spec, uint64_t &low, uint64_t &high);
     static bool CalculateMD5(const FileSpec &file_spec,

source/Host/common/FileSpec.cpp

 #endif
 }
 
-FileSpec
-FileSpec::ResolveSymbolicLink () const {
-    if (!IsSymbolicLink())
-    {
-        return *this;
-    }
-        
-    char resolved_path[PATH_MAX];
-    if (!GetPath (resolved_path, sizeof (resolved_path)))
-    {
-        return *this;
-    }
-        
-#ifdef _WIN32
-    return *this; // TODO make this work on win32
-#else
-    char real_path[PATH_MAX + 1];
-    if (realpath(resolved_path, real_path) == nullptr)
-    {
-        return *this;
-    }
-    
-    return FileSpec(real_path, false);
-#endif
-}
-
 uint32_t
 FileSpec::GetPermissions () const
 {

source/Host/common/HostInfoBase.cpp

         Host::GetModuleFileSpecForHostAddress(reinterpret_cast<void *>(reinterpret_cast<intptr_t>(HostInfoBase::GetLLDBPath))));
     
     // This is necessary because when running the testsuite the shlib might be a symbolic link inside the Python resource dir.
-    lldb_file_spec = lldb_file_spec.ResolveSymbolicLink();
+    FileSystem::ResolveSymbolicLink(lldb_file_spec, lldb_file_spec);
     
     // Remove the filename so that this FileSpec only represents the directory.
     file_spec.GetDirectory() = lldb_file_spec.GetDirectory();

source/Host/posix/FileSystem.cpp

     return error;
 }
 
+Error
+FileSystem::ResolveSymbolicLink(const FileSpec &src, FileSpec &dst)
+{
+    char resolved_path[PATH_MAX];
+    if (!src.GetPath (resolved_path, sizeof (resolved_path)))
+    {
+        return Error("Couldn't get the canonical path for %s", src.GetCString());
+    }
+    
+    char real_path[PATH_MAX + 1];
+    if (realpath(resolved_path, real_path) == nullptr)
+    {
+        Error err;
+        err.SetErrorToErrno();
+        return err;
+    }
+    
+    dst = FileSpec(real_path, false);
+    
+    return Error();
+}
+
 #if defined(__NetBSD__)
 static bool IsLocal(const struct statvfs& info)
 {

source/Host/windows/FileSystem.cpp

     return error;
 }
 
+Error
+FileSystem::ResolveSymbolicLink(const FileSpec &src, FileSpec &dst)
+{
+    return Error("ResolveSymbolicLink() isn't implemented on Windows");
+}
+
 bool
 FileSystem::IsLocal(const FileSpec &spec)
 {