Page MenuHomePhabricator

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

Authored by tberghammer on Dec 2 2015, 7:43 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer updated this revision to Diff 41624.Dec 2 2015, 7:43 AM
tberghammer retitled this revision from to Change Platform::LoadImage to copy the file to the remote platform.
tberghammer updated this object.
tberghammer added reviewers: labath, clayborg.
tberghammer added a subscriber: lldb-commits.
clayborg requested changes to this revision.Dec 2 2015, 10:14 AM
clayborg edited edge metadata.

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

This revision now requires changes to proceed.Dec 2 2015, 10:14 AM

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.

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.

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.

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.

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

I like that solution.

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?

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?

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 updated this revision to Diff 41877.Dec 4 2015, 7:28 AM
tberghammer retitled this revision from Change Platform::LoadImage to copy the file to the remote platform to Add a new option to Platform::LoadImage to install the image.
tberghammer updated this object.
tberghammer edited edge metadata.

Address review comments

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

clayborg requested changes to this revision.Dec 4 2015, 9:57 AM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.Dec 4 2015, 9:57 AM
tberghammer updated this revision to Diff 42055.Dec 7 2015, 4:34 AM
tberghammer edited edge metadata.

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.

clayborg accepted this revision.Dec 7 2015, 10:14 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Dec 7 2015, 10:14 AM
This revision was automatically updated to reflect the committed changes.