This is an archive of the discontinued LLVM Phabricator instance.

Fetch object file load address if it isn't specified by the linker
ClosedPublic

Authored by tberghammer on Jun 16 2015, 3:59 PM.

Details

Summary

Fetch object file load address if it isn't specified by the linker

On Android the load address for the linker isn't specified in the rendezvous data structure. This patch add a new GDB packet to fetch this information from the inferior based on /proc/<pid>/maps if it happens.

Diff Detail

Event Timeline

tberghammer retitled this revision from to Fetch object file load address if it isn't specified by the linker.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added a reviewer: ovyalov.
tberghammer added a subscriber: Unknown Object (MLST).
labath added a subscriber: labath.Jun 16 2015, 4:36 PM
labath added inline comments.
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
2607

I am wondering whether we shouldn't thread the implementation of this down to NativeProcessLinux. Parsing of files in the proc filesystem is unlikely to be fruitful on non-linux systems...

2627

Nit: LLVM style guide recommends early exits to simplify things:

if (size() < 6)
  return true;
if (column[5] != file_name)
  return true;
...
ovyalov edited edge metadata.Jun 16 2015, 4:42 PM

Please see my comments.

include/lldb/Target/Process.h
3040

Nit - I'd rather call it GetModuleLoadAddress in order to underscore that it's not a regular file.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
113

Do we need to call RegisterMemberFunctionHandler on Handle_qFileLoadAddress?

2607

+1

source/Utility/StringExtractorGDBRemote.h
64

Please update docs/lldb-gdb-remote.txt with new command description.

tberghammer edited edge metadata.

Move down logic for reading /proc/<pid>/maps into NativeProcessLinux

P.S.: The previous version of the patch was totally broken. I managed to upload a wrong version what wasn't even compiled.

tberghammer added inline comments.Jun 16 2015, 4:48 PM
include/lldb/Target/Process.h
3040

I intentionally named it a FileLoadAddress because the function (at least with the current implementation) can because to find the load address of any file mapped into the memory. I am not sure if we will use it for anything else but it might be useful.

Add documentation

labath added inline comments.Jun 16 2015, 4:58 PM
include/lldb/Target/Process.h
3040

If you go down that road you better specify the contract of the function much more precisely. For a program module (.so), the load address is a relatively clear concept (even though I am not sure if it is not possible to have the same module loaded twice at different addresses). However, regular files can be mapped into memory in strange ways:

  • same file mapped multiple times
  • a portion of a file mapped, but starting at a non-zero file offset

What will be the "load address" in these cases?

Improve documentation

looks mostly good. I would just like to clarify a few things in the protocol...

docs/lldb-gdb-remote.txt
999 ↗(On Diff #27806)

What is going to be the endianness of this value? I propose big endian, as that is what most packets seem to be using.

1001 ↗(On Diff #27806)

I know F..F is not likely to be a valid load address, but what do you think about using one of the error codes to signify this condition? If it is not too much of a trouble to implement it, it sounds like a more reasonable solution.

include/lldb/Target/Process.h
3049

s/INAV/INVA

ADodds added a subscriber: ADodds.Jun 17 2015, 6:53 AM
ADodds removed a subscriber: ADodds.
aidan.dodds edited edge metadata.Jun 17 2015, 7:04 AM

I added a few inlined comments, but overall this looks good to me.

docs/lldb-gdb-remote.txt
1001 ↗(On Diff #27806)

An error code sounds good to me too as its less ambigous.

source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
415

On some targets 0 can be a valid load address (Hexagon for example). I dont think this will cause problems in that case, as the call to GetFileLoadAddress() should return 0 in that case. But if your changing this peice of code it may be good to keep in mind.

I quickly tested this patch on linux and you were correct, it seems to fix some problems I was seeing with the dyld breakpoint not functioning properly. Thanks for that Tamas.

tberghammer edited edge metadata.

Address review comments

docs/lldb-gdb-remote.txt
999 ↗(On Diff #27806)

Clarified documentation (big endian)

1001 ↗(On Diff #27806)

Done.
(Use "E01" for file not loaded error.)

include/lldb/Target/Process.h
3049

Done

source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
415

Thanks for the information. I believe this change won't cause any issue on hexagon but it is good to know.

lgtm, thanks for fixing this :)

ovyalov added inline comments.Jun 17 2015, 6:43 PM
include/lldb/Host/common/NativeProcessProtocol.h
297 ↗(On Diff #27871)

Nit - I think it's ok to make method pure virtual in order to follow existing style in NativeProcessProtocol

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
4121

Please set is_loaded = false.

4132

Could you check for return value of SendPacketAndWaitForResponse?

Address review comments

include/lldb/Host/common/NativeProcessProtocol.h
297 ↗(On Diff #27871)

Done

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
4121

Done

4132

Done

ovyalov accepted this revision.Jun 18 2015, 10:46 AM
ovyalov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 18 2015, 10:46 AM
This revision was automatically updated to reflect the committed changes.