Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
clayborg added a comment. PlatformPOSIX::LoadImage() and PlatformAndroid::LoadImage() should now become PlatformPOSIX::DoLoadImage() and PlatformAndroid::DoLoadImage() and the common code between them removed, add Platform::LoadImage() and move the common code that looks at both FileSpec arguments, does the remote path fixup and install, then call the virtual DoLoadImage(). It is ok to have a base implementation of Platform::DoLoadImage() that returns an error. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
tberghammer added a comment. Sounds reasonable. The only question is that currently only PlatformPOSIX supports LoadImage. Do we want to add the DoLoadImage function now, or do we want to wait until some other platform also want to implement image loading? http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
clayborg added a comment. Be sure to add nice HeaderDoc to the LoadImage() declaration in Platform.h so everyone knows what is expected. We still might want to split this up so that only Platform has LoadImage and we make everyone else just implement: virtual uint32_t DoLoadImage(lldb_private::Process* process, const FileSpec& image_spec, Error& error); These functions would _only_ load the native "image_spec", but the Platform::LoadImage() would take care of all of the common code that checks the two FileSpec parameters and installs the stuff if needed, then calls through to the DoLoadImage() in subclasses. How does that sound? http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
clayborg added a comment. I like that solution. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
tberghammer added a comment. I would like to exposed the ability to specify if we want the specified file to be copied over to the target or not both at the command line and on the SB API. To do this without duplicating the logic what do the file installation into CommandObjectProcess and SBProcess I think we have to keep the logic inside the Platform plugin and then we should change the signature of Platform::LoadImage. One possible solution what can cover all scenario is the following syntax LoadImage(Process* process, const FileSpec& local_file, const FileSpec& remote_file) with the following meaning: - If both file spec is specified then we copy over the file to the target - If only the remote file is specified then we open the specified file from the target - If only the local file is specified the we copy the file over to the current working directory and open it from there http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
clayborg added a comment. Maybe Platform::LoadImage() should keep its current arguments and the path that is specified should always be correct for the platform. Other code should do the copy over if it needs to and call Platform::LoadImage() with a path that makes sense for the platform. The we can add all the options we want to "process load" to do the installing to a certain location. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
tberghammer added a comment. What is the opinion about adding just a boolean flag to specify if the path provided is a local path or a remote path? In case it is a remote path (will be the default to keep current behavior) then we don't do any copying and if it is a local path then we copy the library over to the current working directory. On Linux it isn't matter where we install the shared library as we can give full path to dlopen and I think it should be true for all other system so I don't see any advantage of specifying the install directory. Note: I also plan to add a new method to the SB API where we can specify the same information. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
jingham added a subscriber: jingham. jingham added a comment. We use process load to load images that are already extant (and system libraries at that.) So it's got to be possible to load a library into a target process without trying to get the image over to the target. And given how slow some targets are, it would be better to have a mode which does no checking but goes straight to the dlopen. If you follow Greg's suggestion of providing an install spec, then an empty install spec could be the indication that you assume the binary is already present. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
clayborg added a comment. The --install-path option is optional and doesn't need to be specified, but if it is specified, we need all platforms, even the host platform, to install the shared library to this location prior to loading it. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. "process load" should probably be changed to have an options that allows us to specify where the shared library needs to be installed: (lldb) process load --install-path=/usr/lib ~/build/libfoo.so Then we should add a new argument to LoadImage: uint32_t Platform::LoadImage(lldb_private::Process* process, const FileSpec& local_image_spec, const FileSpec& install_image_spec, Error& error) This extra install_image_spec can be empty and if it is, we do what you did in the above patch, else we use the "install_image_spec" to copy the shared library to this location first, then load it from the install location. This means we might want to change the Platform::LoadImage() to do more stuff up in Platform.cpp (like install the image using the virtual platform functions to install the shared library), and then change all current Platform subclasses that override LoadImage() over to DoLoadImage() and have the platform subclasses just do the actual "dlopen()" call on a specified file. Right now we are duplicating some code between LoadImage methods. http://reviews.llvm.org/D15152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D15152: Change Platform::LoadImage to copy the file to the remote platform
tberghammer created this revision.
tberghammer added reviewers: labath, clayborg.
tberghammer added a subscriber: lldb-commits.
Herald added subscribers: danalbert, tberghammer, emaste.
Change Platform::LoadImage to copy the file to the remote platform
The new implementation mimic the behavior of "process launch" what also copy
the target executable from the host to the target.
http://reviews.llvm.org/D15152
Files:
packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
source/Plugins/Platform/Android/PlatformAndroid.cpp
source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
Index: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -903,6 +903,16 @@
char path[PATH_MAX];
image_spec.GetPath(path, sizeof(path));
+if (IsRemote() && IsConnected())
+{
+FileSpec remote_file = GetRemoteWorkingDirectory();
+remote_file.AppendPathComponent(image_spec.GetFilename().GetCString());
+error = Install(image_spec, remote_file);
+if (error.Fail())
+return LLDB_INVALID_IMAGE_TOKEN;
+remote_file.GetPath(path, sizeof(path));
+}
+
StreamString expr;
expr.Printf(R"(
struct __lldb_dlopen_result { void *image_ptr; const char
*error_str; } the_result;
Index: source/Plugins/Platform/Android/PlatformAndroid.cpp
===
--- source/Plugins/Platform/Android/PlatformAndroid.cpp
+++ source/Plugins/Platform/Android/PlatformAndroid.cpp
@@ -383,6 +383,16 @@
char path[PATH_MAX];
image_spec.GetPath(path, sizeof(path));
+if (IsRemote() && IsConnected())
+{
+FileSpec remote_file = GetRemoteWorkingDirectory();
+remote_file.AppendPathComponent(image_spec.GetFilename().GetCString());
+error = Install(image_spec, remote_file);
+if (error.Fail())
+return LLDB_INVALID_IMAGE_TOKEN;
+remote_file.GetPath(path, sizeof(path));
+}
+
StreamString expr;
expr.Printf(R"(
struct __lldb_dlopen_result { void *image_ptr; const char
*error_str; } the_result;
Index:
packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
===
--- packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
+++ packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
@@ -210,18 +210,13 @@
else:
dylibName = 'libloadunload_a.so'
-if lldb.remote_platform:
-dylibPath = os.path.join(shlib_dir, dylibName)
-else:
-dylibPath = dylibName
-
# Make sure that a_function does not exist at this point.
self.expect("image lookup -n a_function", "a_function should not exist
yet",
error=True, matching=False, patterns = ["1 match found"])
# Use lldb 'process load' to load the dylib.
-self.expect("process load %s" % dylibPath, "%s loaded correctly" %
dylibPath,
-patterns = ['Loading "%s".*ok' % dylibPath,
+self.expect("process load %s" % dylibName, "%s loaded correctly" %
dylibName,
+patterns = ['Loading "%s".*ok' % dylibName,
'Image [0-9]+ loaded'])
# Search for and match the "Image ([0-9]+) loaded" pattern.
Index: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -903,6 +903,16 @@
char path[PATH_MAX];
image_spec.GetPath(path, sizeof(path));
+if (IsRemote() && IsConnected())
+{
+FileSpec remote_file = GetRemoteWorkingDirectory();
+remote_file.AppendPathComponent(image_spec.GetFilename().GetCString());
+error = Install(image_spec, remote_file);
+if (error.Fail())
+return LLDB_INVALID_IMAGE_TOKEN;
+remote_file.GetPath(path, sizeof(path));
+}
+
StreamString expr;
expr.Printf(R"(
struct __lldb_dlopen_result { void *image_ptr; const char *error_str; } the_result;
Index: source/Plugins/Platform/Android/PlatformAndroid.cpp
===
--- source/Plugins/Platform/Android/PlatformAndroid.cpp
+++ source/Plugins/Platform/Android/PlatformAndroid.cpp
@@ -383,6 +383,16 @@
char path[PATH_MAX];
image_spec.GetPath(path, sizeof(path));
+if (IsRemote() && IsConnected())
+{
+FileSpec remote_file = GetRemoteWorkingDirectory();
+remote_file.AppendPathComponent(image_spec.GetFilename().GetCString());
+error = Install(image_spec, remote_file);
+if (error.Fail())
+
