Page MenuHomePhabricator

POSIX DYLD: add workaround for android L loader
Needs RevisionPublic

Authored by compnerd on Aug 27 2019, 6:32 PM.

Details

Summary

In certain cases, the loader does not report the base address of the DSO.
Attempt to infer it from the loaded address of the object file. This was
originally added in the Swift fork of LLDB, this simply is upstreaming the
workaround for the system loader. Unfortunately, without a recent test case, it
is difficult to construct a test case for this. However, this problem is also
something which is worked around in the unwinder and has been seen multiple
times previously.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

compnerd created this revision.Aug 27 2019, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 6:33 PM
Herald added subscribers: abidh, srhines. · View Herald Transcript
davide accepted this revision.Aug 28 2019, 7:36 AM

LGTM. Thanks. I'm going to clean the diff downstream.

This revision is now accepted and ready to land.Aug 28 2019, 7:36 AM
xiaobai accepted this revision.Aug 28 2019, 10:23 AM

LGTM, small typo tho

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

lirbary -> library

labath requested changes to this revision.Aug 28 2019, 11:31 AM
labath added a subscriber: labath.

It looks like this code would be better placed inside UpdateBaseAddrIfNecessary, as that's exactly what it is doing.

This revision now requires changes to proceed.Aug 28 2019, 11:31 AM

My other concerns about this patch are:

  • the base_addr field is the *difference* between the preferred load address encoded in the object file, and the actual load address. This means that this workaround will fire every time the object is loaded at it's preferred load address, which is definitely not what this patch advertises to do...
  • it looks like it should be possible to test this using the "gdb-client" test suite. I am hoping that it all it would take is to mock the appropriate responses to the qXfer:libraries and qFileLoadAddress packets. We should at least try to do that...
  • just because this patch was fast-tracked into the swift fork of lldb without proper scrutiny (it seems), it does not mean it should be fast-tracked here (in fact, I would say the opposite is true...)