Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
This revision now requires changes to proceed.


Comment at: include/lldb/Host/FileSpec.h:516
@@ -514,1 +515,3 @@
+FileSpec
+GetSymbolicLinkTarget () const;
 

rename to ResolveSymbolicLink?


Comment at: source/Host/common/FileSpec.cpp:818
@@ +817,3 @@
+{
+return FileSpec();
+}

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


Comment at: source/Host/common/HostInfoBase.cpp:310-312
@@ +309,5 @@
+
+if (lldb_file_spec.IsSymbolicLink()) {
+lldb_file_spec = lldb_file_spec.GetSymbolicLinkTarget();
+}
+

No need to check if we change the behavior of ResolveSymbolicLink, this code 
will just be:
```
lldb_file_spec = lldb_file_spec.ResolveSymbolicLink();
```


http://reviews.llvm.org/D12984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Zachary Turner via lldb-commits]

zturner added a comment.

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



Comment at: include/lldb/Host/FileSystem.h:43
@@ -42,1 +42,3 @@
+
+static Error Realpath(const FileSpec , FileSpec );
 

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.


http://reviews.llvm.org/D12984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Zachary Turner via lldb-commits]

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

On Fri, Sep 18, 2015 at 3:08 PM Sean Callanan  wrote:

> spyffe added a subscriber: spyffe.
> spyffe added a comment.
>
> Sure.  On it.
>
> Sean
>
>
> http://reviews.llvm.org/D12984
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Sean Callanan via lldb-commits]

spyffe added a reviewer: zturner.
spyffe removed a subscriber: zturner.
spyffe updated this revision to Diff 35143.
spyffe added a comment.

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


http://reviews.llvm.org/D12984

Files:
  include/lldb/Host/FileSpec.h
  include/lldb/Host/FileSystem.h
  source/Host/common/FileSpec.cpp
  source/Host/common/HostInfoBase.cpp
  source/Host/posix/FileSystem.cpp
  source/Host/windows/FileSystem.cpp

Index: source/Host/windows/FileSystem.cpp
===
--- source/Host/windows/FileSystem.cpp
+++ source/Host/windows/FileSystem.cpp
@@ -199,6 +199,12 @@
 return error;
 }
 
+Error
+FileSystem::Realpath(const FileSpec , FileSpec )
+{
+return Error("Realpath() isn't implemented on Windows");
+}
+
 bool
 FileSystem::IsLocal(const FileSpec )
 {
Index: source/Host/posix/FileSystem.cpp
===
--- source/Host/posix/FileSystem.cpp
+++ source/Host/posix/FileSystem.cpp
@@ -226,6 +226,28 @@
 return error;
 }
 
+Error
+FileSystem::Realpath(const FileSpec , FileSpec )
+{
+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)
 {
Index: source/Host/common/HostInfoBase.cpp
===
--- source/Host/common/HostInfoBase.cpp
+++ source/Host/common/HostInfoBase.cpp
@@ -308,7 +308,7 @@
 Host::GetModuleFileSpecForHostAddress(reinterpret_cast(reinterpret_cast(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::Realpath(lldb_file_spec, lldb_file_spec);
 
 // Remove the filename so that this FileSpec only represents the directory.
 file_spec.GetDirectory() = lldb_file_spec.GetDirectory();
Index: source/Host/common/FileSpec.cpp
===
--- source/Host/common/FileSpec.cpp
+++ source/Host/common/FileSpec.cpp
@@ -811,32 +811,6 @@
 #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
 {
Index: include/lldb/Host/FileSystem.h
===
--- include/lldb/Host/FileSystem.h
+++ include/lldb/Host/FileSystem.h
@@ -39,6 +39,8 @@
 static Error Symlink(const FileSpec , const FileSpec );
 static Error Readlink(const FileSpec , FileSpec );
 static Error Unlink(const FileSpec _spec);
+
+static Error Realpath(const FileSpec , FileSpec );
 
 static bool CalculateMD5(const FileSpec _spec, uint64_t , uint64_t );
 static bool CalculateMD5(const FileSpec _spec,
Index: include/lldb/Host/FileSpec.h
===
--- include/lldb/Host/FileSpec.h
+++ include/lldb/Host/FileSpec.h
@@ -511,9 +511,6 @@
 
 bool
 IsSymbolicLink () const;
-
-FileSpec
-ResolveSymbolicLink () const;
 
 //--
 /// Get the memory cost of this object.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Zachary Turner via lldb-commits]

zturner added a comment.

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.


http://reviews.llvm.org/D12984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Sean Callanan via lldb-commits]

Sure.  On it.

Sean

> On Sep 18, 2015, at 2:44 PM, Zachary Turner  wrote:
> 
> zturner added a comment.
> 
> 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.
> 
> 
> http://reviews.llvm.org/D12984
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Sean Callanan via lldb-commits]

spyffe updated this revision to Diff 35144.
spyffe added a comment.

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


http://reviews.llvm.org/D12984

Files:
  include/lldb/Host/FileSpec.h
  include/lldb/Host/FileSystem.h
  source/Host/common/FileSpec.cpp
  source/Host/common/HostInfoBase.cpp
  source/Host/posix/FileSystem.cpp
  source/Host/windows/FileSystem.cpp

Index: source/Host/windows/FileSystem.cpp
===
--- source/Host/windows/FileSystem.cpp
+++ source/Host/windows/FileSystem.cpp
@@ -199,6 +199,12 @@
 return error;
 }
 
+Error
+FileSystem::ResolveSymbolicLink(const FileSpec , FileSpec )
+{
+return Error("ResolveSymbolicLink() isn't implemented on Windows");
+}
+
 bool
 FileSystem::IsLocal(const FileSpec )
 {
Index: source/Host/posix/FileSystem.cpp
===
--- source/Host/posix/FileSystem.cpp
+++ source/Host/posix/FileSystem.cpp
@@ -226,6 +226,28 @@
 return error;
 }
 
+Error
+FileSystem::ResolveSymbolicLink(const FileSpec , FileSpec )
+{
+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)
 {
Index: source/Host/common/HostInfoBase.cpp
===
--- source/Host/common/HostInfoBase.cpp
+++ source/Host/common/HostInfoBase.cpp
@@ -308,7 +308,7 @@
 Host::GetModuleFileSpecForHostAddress(reinterpret_cast(reinterpret_cast(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();
Index: source/Host/common/FileSpec.cpp
===
--- source/Host/common/FileSpec.cpp
+++ source/Host/common/FileSpec.cpp
@@ -811,32 +811,6 @@
 #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
 {
Index: include/lldb/Host/FileSystem.h
===
--- include/lldb/Host/FileSystem.h
+++ include/lldb/Host/FileSystem.h
@@ -39,6 +39,8 @@
 static Error Symlink(const FileSpec , const FileSpec );
 static Error Readlink(const FileSpec , FileSpec );
 static Error Unlink(const FileSpec _spec);
+
+static Error ResolveSymbolicLink(const FileSpec , FileSpec );
 
 static bool CalculateMD5(const FileSpec _spec, uint64_t , uint64_t );
 static bool CalculateMD5(const FileSpec _spec,
Index: include/lldb/Host/FileSpec.h
===
--- include/lldb/Host/FileSpec.h
+++ include/lldb/Host/FileSpec.h
@@ -511,9 +511,6 @@
 
 bool
 IsSymbolicLink () const;
-
-FileSpec
-ResolveSymbolicLink () const;
 
 //--
 /// Get the memory cost of this object.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12984: Resolve symlinks when looking for the LLDB shlib

[Zachary Turner via lldb-commits]

zturner accepted this revision.
zturner added a comment.

Thanks!


http://reviews.llvm.org/D12984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits