Add a new option to Platform::LoadImage to install the image

[tberghammer]

Summary

Add a new option to Platform::LoadImage to install the image

This change introduce 3 different working mode for Platform::LoadImage
depending on the file specs specified.

• If only a remote file is specified then the remote file is loaded on the target (same behavior as before)
• If only a local file is specified then the local file is installed to the current working directory and then loaded from there.
• If both local and remote file is specified then the local file is installed to the specified location and then loaded from there.

The same options are exposed on the SB API with a new method LoadImage
method while the old signature presers its meaning.

On the command line the installation of the shared library can be specified
with the "--install" option of "process load".

[clayborg]

"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.

[clayborg]

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.

[jingham]

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.

[tberghammer]

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.

[clayborg]

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.

[tberghammer]

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

[clayborg]

I like that solution.

[clayborg]

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?

[tberghammer]

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?

[clayborg]

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.

[tberghammer]

Address review comments

Note: PlatformAndroid::DoLoadImage isn't necessary because of the cleanup done in rL254608

[clayborg]

Add header doc for the new LoadImage and make Platform::LoadImage virtual in case platforms want to override this and this is good to go.

include/lldb/API/SBProcess.h

301–307 ↗

(On Diff #41877)

Add correct header doc for this if we are going to add documentation to the header file.

include/lldb/Target/Platform.h

1023–1027 ↗

(On Diff #41877)

This can still be virtual just in case other platforms want to do something completely different that what is supported in the default implementation.

[tberghammer]

Update API documentation in SBProcess.h

include/lldb/API/SBProcess.h

301–307 ↗

(On Diff #41877)

Done

include/lldb/Target/Platform.h

1023–1027 ↗

(On Diff #41877)

Currently I don't see any valid reason to override the implementation of this function as it still have to respect the API specification. For now I would prefer to leave it non-virtual and we can change it in the future if we need to.

lldb/trunk/include/lldb/API/SBProcess.h

public:
     GetBroadcasterClass ();
 
     bool
     GetDescription (lldb::SBStream &description);
 
     uint32_t
     GetNumSupportedHardwareWatchpoints (lldb::SBError &error) const;
 
+    //------------------------------------------------------------------
+    /// Load a shared library into this process.
+    ///
+    /// @param[in] remote_image_spec
+    ///     The path for the shared library on the target what you want
+    ///     to load.
+    ///
+    /// @param[out] error
+    ///     An error object that gets filled in with any errors that
+    ///     might occur when trying to load the shared library.
+    ///
+    /// @return
+    ///     A token that represents the shared library that can be
+    ///     later used to unload the shared library. A value of
+    ///     LLDB_INVALID_IMAGE_TOKEN will be returned if the shared
+    ///     library can't be opened.
+    //------------------------------------------------------------------
+    uint32_t
+    LoadImage (lldb::SBFileSpec &remote_image_spec, lldb::SBError &error);
+
+    //------------------------------------------------------------------
+    /// Load a shared library into this process.
+    ///
+    /// @param[in] local_image_spec
+    ///     The file spec that points to the shared library that you
+    ///     want to load if the library is located on the host. The
+    ///     library will be copied over to the location specified by
+    ///     remote_image_spec or into the current working directory with
+    ///     the same filename if the remote_image_spec isn't specified.
+    ///
+    /// @param[in] remote_image_spec
+    ///     If local_image_spec is specified then the location where the
+    ///     library should be copied over from the host. If
+    ///     local_image_spec isn't specified, then the path for the
+    ///     shared library on the target what you want to load.
+    ///
+    /// @param[out] error
+    ///     An error object that gets filled in with any errors that
+    ///     might occur when trying to load the shared library.
+    ///
+    /// @return
+    ///     A token that represents the shared library that can be
+    ///     later used to unload the shared library. A value of
+    ///     LLDB_INVALID_IMAGE_TOKEN will be returned if the shared
+    ///     library can't be opened.
+    //------------------------------------------------------------------
     uint32_t
-    LoadImage (lldb::SBFileSpec &image_spec, lldb::SBError &error);
+    LoadImage (const lldb::SBFileSpec &local_image_spec,
+               const lldb::SBFileSpec &remote_image_spec,
+               lldb::SBError &error);
     
     lldb::SBError
     UnloadImage (uint32_t image_token);
     
     lldb::SBError
     SendEventData (const char *data);
     
     //------------------------------------------------------------------

lldb/trunk/include/lldb/Target/Platform.h

//                Error &error) = 0;
         ///
         /// Try and load a shared library into the current process. This
         /// call might fail in the dynamic loader plug-in says it isn't safe
         /// to try and load shared libraries at the moment.
         ///
         /// @param[in] process
         ///     The process to load the image.
         ///
-        /// @param[in] image_spec
-        ///     The image file spec that points to the shared library that
-        ///     you want to load.
+        /// @param[in] local_file
+        ///     The file spec that points to the shared library that you want
+        ///     to load if the library is located on the host. The library will
+        ///     be copied over to the location specified by remote_file or into
+        ///     the current working directory with the same filename if the
+        ///     remote_file isn't specified.
+        ///
+        /// @param[in] remote_file
+        ///     If local_file is specified then the location where the library
+        ///     should be copied over from the host. If local_file isn't
+        ///     specified, then the path for the shared library on the target
+        ///     what you want to load.
         ///
         /// @param[out] error
         ///     An error object that gets filled in with any errors that
         ///     might occur when trying to load the shared library.
         ///
         /// @return
         ///     A token that represents the shared library that can be
         ///     later used to unload the shared library. A value of
         ///     LLDB_INVALID_IMAGE_TOKEN will be returned if the shared
         ///     library can't be opened.
         //------------------------------------------------------------------
-        virtual uint32_t
+        uint32_t
         LoadImage (lldb_private::Process* process,
-                   const lldb_private::FileSpec& image_spec,
+                   const lldb_private::FileSpec& local_file,
+                   const lldb_private::FileSpec& remote_file,
+                   lldb_private::Error& error);
+
+        virtual uint32_t
+        DoLoadImage (lldb_private::Process* process,
+                     const lldb_private::FileSpec& remote_file,
                      lldb_private::Error& error);
 
         virtual Error
         UnloadImage (lldb_private::Process* process, uint32_t image_token);
 
     protected:
         bool m_is_host;
         // Set to true when we are able to actually set the OS version while
         // being connected. For remote platforms, we might set the version ahead

lldb/trunk/packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py

def test_lldb_process_load_and_unload_commands(self):
         else:
             shlib_dir = self.mydir
 
         if self.platformIsDarwin():
             dylibName = 'libloadunload_a.dylib'
         else:
             dylibName = 'libloadunload_a.so'
 
-        if lldb.remote_platform:
-            # Don't use os.path.join as we have to use the path separator for the target
-            dylibPath = 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 --install" % dylibName, "%s loaded correctly" % dylibName,
+            patterns = ['Loading "%s".*ok' % dylibName,
                         'Image [0-9]+ loaded'])
 
         # Search for and match the "Image ([0-9]+) loaded" pattern.
         output = self.res.GetOutput()
         pattern = re.compile("Image ([0-9]+) loaded")
         for l in output.split(os.linesep):
             #print("l:", l)
             match = pattern.search(l)

lldb/trunk/source/API/SBProcess.cpp

SBProcess::GetNumSupportedHardwareWatchpoints (lldb::SBError &sb_error) const
     else
     {
         sb_error.SetErrorString ("SBProcess is invalid");
     }
     return num;
 }
 
 uint32_t
-SBProcess::LoadImage (lldb::SBFileSpec &sb_image_spec, lldb::SBError &sb_error)
+SBProcess::LoadImage (lldb::SBFileSpec &sb_remote_image_spec, lldb::SBError &sb_error)
+{
+    return LoadImage(SBFileSpec(), sb_remote_image_spec, sb_error);
+}
+
+uint32_t
+SBProcess::LoadImage (const lldb::SBFileSpec &sb_local_image_spec,
+                      const lldb::SBFileSpec &sb_remote_image_spec,
+                      lldb::SBError &sb_error)
 {
     ProcessSP process_sp(GetSP());
     if (process_sp)
     {
         Process::StopLocker stop_locker;
         if (stop_locker.TryLock(&process_sp->GetRunLock()))
         {
             Mutex::Locker api_locker (process_sp->GetTarget().GetAPIMutex());
             PlatformSP platform_sp = process_sp->GetTarget().GetPlatform();
-            return platform_sp->LoadImage (process_sp.get(), *sb_image_spec, sb_error.ref());
+            return platform_sp->LoadImage (process_sp.get(),
+                                           *sb_local_image_spec,
+                                           *sb_remote_image_spec,
+                                           sb_error.ref());
         }
         else
         {
             Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
             if (log)
                 log->Printf ("SBProcess(%p)::LoadImage() => error: process is running",
                              static_cast<void*>(process_sp.get()));
             sb_error.SetErrorString("process is running");

lldb/trunk/source/Commands/CommandObjectProcess.cpp

 //-------------------------------------------------------------------------
 // CommandObjectProcessLoad
 //-------------------------------------------------------------------------
 #pragma mark CommandObjectProcessLoad
 
 class CommandObjectProcessLoad : public CommandObjectParsed
 {
 public:
+    class CommandOptions : public Options
+    {
+    public:
+        CommandOptions (CommandInterpreter &interpreter) :
+            Options(interpreter)
+        {
+            // Keep default values of all options in one place: OptionParsingStarting ()
+            OptionParsingStarting ();
+        }
+
+        ~CommandOptions () override = default;
+
+        Error
+        SetOptionValue (uint32_t option_idx, const char *option_arg) override
+        {
+            Error error;
+            const int short_option = m_getopt_table[option_idx].val;
+            switch (short_option)
+            {
+            case 'i':
+                do_install = true;
+                if (option_arg && option_arg[0])
+                    install_path.SetFile(option_arg, false);
+                break;
+            default:
+                error.SetErrorStringWithFormat("invalid short option character '%c'", short_option);
+                break;
+            }
+            return error;
+        }
+
+        void
+        OptionParsingStarting () override
+        {
+            do_install = false;
+            install_path.Clear();
+        }
+
+        const OptionDefinition*
+        GetDefinitions () override
+        {
+            return g_option_table;
+        }
+
+        // Options table: Required for subclasses of Options.
+        static OptionDefinition g_option_table[];
+
+        // Instance variables to hold the values for command options.
+        bool do_install;
+        FileSpec install_path;
+    };
 
     CommandObjectProcessLoad (CommandInterpreter &interpreter) :
         CommandObjectParsed (interpreter,
                              "process load",
                              "Load a shared library into the current process.",
                              "process load <filename> [<filename> ...]",
                              eCommandRequiresProcess       |
                              eCommandTryTargetAPILock      |
                              eCommandProcessMustBeLaunched |
-                             eCommandProcessMustBePaused   )
+                             eCommandProcessMustBePaused   ),
+        m_options (interpreter)
     {
     }
 
-    ~CommandObjectProcessLoad () override
+    ~CommandObjectProcessLoad () override = default;
+
+    Options *
+    GetOptions () override
     {
+        return &m_options;
     }
 
 protected:
     bool
     DoExecute (Args& command, CommandReturnObject &result) override
     {
         Process *process = m_exe_ctx.GetProcessPtr();
 
         const size_t argc = command.GetArgumentCount();
-        
         for (uint32_t i=0; i<argc; ++i)
         {
             Error error;
+            PlatformSP platform = process->GetTarget().GetPlatform();
             const char *image_path = command.GetArgumentAtIndex(i);
+            uint32_t image_token = LLDB_INVALID_IMAGE_TOKEN;
+
+            if (!m_options.do_install)
+            {
                 FileSpec image_spec (image_path, false);
-            PlatformSP platform = process->GetTarget().GetPlatform();
                 platform->ResolveRemotePath(image_spec, image_spec);
-            uint32_t image_token = platform->LoadImage(process, image_spec, error);
+                image_token = platform->LoadImage(process, FileSpec(), image_spec, error);
+            }
+            else if (m_options.install_path)
+            {
+                FileSpec image_spec (image_path, true);
+                platform->ResolveRemotePath(m_options.install_path, m_options.install_path);
+                image_token = platform->LoadImage(process, image_spec, m_options.install_path, error);
+            }
+            else
+            {
+                FileSpec image_spec (image_path, true);
+                image_token = platform->LoadImage(process, image_spec, FileSpec(), error);
+            }
+
             if (image_token != LLDB_INVALID_IMAGE_TOKEN)
             {
-                result.AppendMessageWithFormat ("Loading \"%s\"...ok\nImage %u loaded.\n", image_path, image_token);  
+                result.AppendMessageWithFormat ("Loading \"%s\"...ok\nImage %u loaded.\n", image_path, image_token);
                 result.SetStatus (eReturnStatusSuccessFinishResult);
             }
             else
             {
                 result.AppendErrorWithFormat ("failed to load '%s': %s", image_path, error.AsCString());
                 result.SetStatus (eReturnStatusFailed);
             }
         }
         return result.Succeeded();
     }
+    
+    CommandOptions m_options;
 };
 
+OptionDefinition
+CommandObjectProcessLoad::CommandOptions::g_option_table[] =
+{
+    { LLDB_OPT_SET_ALL, false, "install", 'i', OptionParser::eOptionalArgument, nullptr, nullptr, 0, eArgTypePath, "Install the shared library to the target. If specified without an argument then the library will installed in the current working directory."},
+    { 0,                false, nullptr,    0 , 0,                               nullptr, nullptr, 0, eArgTypeNone, nullptr }
+};
 
 //-------------------------------------------------------------------------
 // CommandObjectProcessUnload
 //-------------------------------------------------------------------------
 #pragma mark CommandObjectProcessUnload
 
 class CommandObjectProcessUnload : public CommandObjectParsed
 {

lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.h

public:
 
     lldb_private::Error
     ConnectRemote (lldb_private::Args& args) override;
 
     lldb_private::Error
     DisconnectRemote () override;
 
     uint32_t
-    LoadImage (lldb_private::Process* process,
-               const lldb_private::FileSpec& image_spec,
+    DoLoadImage (lldb_private::Process* process,
+                 const lldb_private::FileSpec& remote_file,
                  lldb_private::Error& error) override;
 
     lldb_private::Error
     UnloadImage (lldb_private::Process* process, uint32_t image_token) override;
 
 protected:
     std::unique_ptr<lldb_private::OptionGroupOptions> m_options;
     lldb::PlatformSP m_remote_platform_sp; // Allow multiple ways to connect to a remote POSIX-compliant OS
 

lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

UserExpression::Evaluate(exe_ctx,
                              result_valobj_sp,
                              expr_error);
     if (result_valobj_sp->GetError().Fail())
         return result_valobj_sp->GetError();
     return Error();
 }
 
 uint32_t
-PlatformPOSIX::LoadImage(lldb_private::Process* process, const FileSpec& image_spec, Error& error)
+PlatformPOSIX::DoLoadImage(lldb_private::Process* process,
+                           const lldb_private::FileSpec& remote_file,
+                           lldb_private::Error& error)
 {
     char path[PATH_MAX];
-    image_spec.GetPath(path, sizeof(path));
+    remote_file.GetPath(path, sizeof(path));
 
     StreamString expr;
     expr.Printf(R"(
                    struct __lldb_dlopen_result { void *image_ptr; const char *error_str; } the_result;
                    the_result.image_ptr = dlopen ("%s", 2);
                    if (the_result.image_ptr == (void *) 0x0)
                    {
                        the_result.error_str = dlerror();

lldb/trunk/source/Target/Platform.cpp

 Platform::GetUnixSignals()
 {
     if (IsHost())
         return Host::GetUnixSignals();
     return GetRemoteUnixSignals();
 }
 
 uint32_t
-Platform::LoadImage(lldb_private::Process* process, const FileSpec& image_spec, Error& error)
+Platform::LoadImage(lldb_private::Process* process,
+                    const lldb_private::FileSpec& local_file,
+                    const lldb_private::FileSpec& remote_file,
+                    lldb_private::Error& error)
+{
+    if (local_file && remote_file)
+    {
+        // Both local and remote file was specified. Install the local file to the given location.
+        if (IsRemote() || local_file != remote_file)
+        {
+            error = Install(local_file, remote_file);
+            if (error.Fail())
+                return LLDB_INVALID_IMAGE_TOKEN;
+        }
+        return DoLoadImage(process, remote_file, error);
+    }
+
+    if (local_file)
+    {
+        // Only local file was specified. Install it to the current working directory.
+        FileSpec target_file = GetWorkingDirectory();
+        target_file.AppendPathComponent(local_file.GetFilename().AsCString());
+        if (IsRemote() || local_file != target_file)
+        {
+            error = Install(local_file, target_file);
+            if (error.Fail())
+                return LLDB_INVALID_IMAGE_TOKEN;
+        }
+        return DoLoadImage(process, target_file, error);
+    }
+
+    if (remote_file)
+    {
+        // Only remote file was specified so we don't have to do any copying
+        return DoLoadImage(process, remote_file, error);
+    }
+
+    error.SetErrorString("Neither local nor remote file was specified");
+    return LLDB_INVALID_IMAGE_TOKEN;
+}
+
+uint32_t
+Platform::DoLoadImage (lldb_private::Process* process,
+                       const lldb_private::FileSpec& remote_file,
+                       lldb_private::Error& error)
 {
     error.SetErrorString("LoadImage is not supported on the current platform");
     return LLDB_INVALID_IMAGE_TOKEN;
 }
 
 Error
 Platform::UnloadImage(lldb_private::Process* process, uint32_t image_token)
 {
-    return Error("UnLoadImage is not supported on the current platform");
+    return Error("UnloadImage is not supported on the current platform");
 }