Page MenuHomePhabricator

[lldb] [POSIX-DYLD] Update the cached exe path after attach
ClosedPublic

Authored by mgorny on Nov 28 2020, 4:12 AM.

Details

Summary

Fix the POSIX-DYLD plugin to update the cached executable path after
attaching. Previously, the path was cached in DYLDRendezvous
constructor and not updated afterwards. This meant that if LLDB was
attaching to a process (e.g. via connecting to lldb-server), the code
stored the empty path before DidAttach() resolved it. The fix updates
the cached path in DidAttach().

This fixes a new instance of https://llvm.org/pr17880

Diff Detail

Event Timeline

mgorny requested review of this revision.Nov 28 2020, 4:12 AM
mgorny created this revision.

Seems reasonable(ish), but needs a test case. Do you need to launch a separate server instance, or would just a plain attach work?

Seems reasonable(ish), but needs a test case. Do you need to launch a separate server instance, or would just a plain attach work?

It failed only when connecting to remote lldb-server.

mgorny updated this revision to Diff 309684.Dec 4 2020, 3:41 PM

With D92667, I've finally found a way to reproduce this without running lldb-server explicitly — we need to spawn the program via relative path though. I've added a test that does that, and compares the address from image list with the one gotten after explicitly creating target via file path. This clearly reproduces the problem if you don't modify the sources (i.e. mismatch between 0x400000 and 0x200000).

Seems reasonable to me

I am having second thoughts about the image list approach to testing this. The regex matching is somewhat messy.

A bad load address should manifest itself in plenty of other ways.. Could we print the value of some global variable to check that we got the correct one?

lldb/test/API/commands/process/attach/TestProcessAttach.py
101

Do you need to attach at any particular moment during the process startup? The attach command could run pretty quickly, before the loader finished its work...

mgorny added a comment.Dec 7 2020, 1:50 AM

I am having second thoughts about the image list approach to testing this. The regex matching is somewhat messy.

A bad load address should manifest itself in plenty of other ways.. Could we print the value of some global variable to check that we got the correct one?

I suppose we could do that.

lldb/test/API/commands/process/attach/TestProcessAttach.py
101

I don't know ;-). Do you have any particular suggestion? That said, I suppose this means other tests suffer from the same problem, in particular they could attach before lldb_enable_attach().

mgorny updated this revision to Diff 309871.Dec 7 2020, 4:23 AM
mgorny edited the summary of this revision. (Show Details)

Rewritten the test wrt comments, that is:

  1. Added a breakpoint-continue to ensure that the program is past all initial work before we test it.
  2. Print a global variable to test it.

I have also confirmed that reading the variable fails without the code change (because of reading wrong memory address), but also the breakpoint doesn't get set correctly.

mgorny marked an inline comment as done.Dec 7 2020, 4:23 AM
labath accepted this revision.Dec 10 2020, 12:46 AM

Rewritten the test wrt comments, that is:

  1. Added a breakpoint-continue to ensure that the program is past all initial work before we test it.

I am not sure that will help -- I'm guessing the dynamic loader plugin will go through a substantially different codepath if the attach happens early on vs. if it happens after main is reached. It may be better to update this to do the wait_for_file_on_target dance, which would also prevent attach denied errors.

lldb/test/API/commands/process/attach/TestProcessAttach.py
101

Good point. Some of our attach tests have the additional lldbutil.wait_for_file_on_target call as an additional synchronization. I guess this test is not one of them...

This revision is now accepted and ready to land.Dec 10 2020, 12:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 12:31 AM