This is an archive of the discontinued LLVM Phabricator instance.

Fetch module specification from remote process also
ClosedPublic

Authored by tberghammer on Mar 23 2015, 8:27 AM.

Details

Summary

Fetch module specification from remote process also

Previously the remote module specification was fetched only from the
remote platform. With this CL if we have a remote process then we ask it
if it have any information from a given module. It is required because
on android the dynamic linker only reports the name of the SO file and
the platform can't always find it without a full path (the process can
do it based on /proc/<pid>/maps).

Diff Detail

Event Timeline

tberghammer retitled this revision from to Fetch module specification from remote process also.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added reviewers: ovyalov, clayborg.
tberghammer added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Mar 23 2015, 10:45 AM
clayborg edited edge metadata.

Fix the "std::string" arguments as noted in inline comments and this is good to go.

include/lldb/Host/common/NativeProcessProtocol.h
291

change this parameter to "const char *' or a "const llvm::StringRef &" unless there really is a need for a std::string.

include/lldb/Target/Process.h
3009

add a "module_spec.Clear();" before the return false.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
3983

change argument to "const char *" or "const llvm::StringRef &"

This revision now requires changes to proceed.Mar 23 2015, 10:45 AM
tberghammer edited edge metadata.

Address issues requested in review

BTW: Why do we prefer const char* and const StringRef& over const std::string&? (My problem with const StringRef& is that it isn't necessarily null terminated so it is expensive to convert it to a c-string while with a const char* we possibly have to check for nullptr and it is expensive to get the length of the string.)

ovyalov accepted this revision.Mar 23 2015, 12:15 PM
ovyalov edited edge metadata.

Looks good - please see my comment.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
3982

What do you think about returning Error if sth bad happens? No maps file, failed to open it...

clayborg accepted this revision.Mar 23 2015, 1:17 PM
clayborg edited edge metadata.

Its mainly an efficiency thing. If you have a std::string you might need to allocate data on the heap to store the string just to make the call. Since this argument is a path, it is more likely to have to allocate on the heap. Most std::string implementations store the first 20 some bytes in the std::string itself, but if it goes over, it will need to malloc a buffer for the string and make a copy. Most path arguments in the standard libraries and in functions already take "const char *" for paths, so it is just keeping the argument in the currency that most people want is good. I also don't like llvm::StringRef for the path because if you want to guarantee NULL terminate you will need to call my_str.str().c_str() which makes a temp std::string using the length, so I do think "const char *" is the best for the API.

This revision is now accepted and ready to land.Mar 23 2015, 1:17 PM
tberghammer edited edge metadata.

Return error from GetLoadedModuleFileSpec

This revision was automatically updated to reflect the committed changes.