Fix rendezvous for rebase_exec=true case

[emrekultursay]

Summary

When rebase_exec=true in DidAttach(), all modules are loaded
before the rendezvous breakpoint is set, which means the
LoadInterpreterModule() method is not called and m_interpreter_module
is not initialized.

This causes the very first rendezvous breakpoint hit with
m_initial_modules_added=false to accidentally unload the
module_sp that corresponds to the dynamic loader.

This bug (introduced in D92187) was causing the rendezvous
mechanism to not work in Android 28. The mechanism works
fine on older/newer versions of Android.

Test: Verified rendezvous on Android 28 and 29
Test: Added dlopen test

[mgorny]

I'm going to test this on FreeBSD in a minute. Could you fix clang-formatting in the meantime?

[mgorny]

Ok, I don't see any obvious regressions while testing, so LGTM modulo formatting changes.

[emrekultursay]

Applied clang-format

[labath]

Since the meaning of m_initial_modules_added is basically "should I do a full scan through the linked list" and LoadAllCurrentModules (called from DidAttach) does a full scan, it seems to me the real bug is that this function does not set m_initial_modules_added = true. Would that fix your issue?

Also, what makes Android 28 special? Is it the presence/absence of a dynamic linker symlink? I'm wondering if we could reproduce (test) this more generally by ensuring we launch a binary through a symlink (or not).

[emrekultursay]

Added m_initial_modules_added=true into LoadAllCurrentModules.

[emrekultursay]

In D109797#3003265, @labath wrote:

Since the meaning of m_initial_modules_added is basically "should I do a full scan through the linked list" and LoadAllCurrentModules (called from DidAttach) does a full scan, it seems to me the real bug is that this function does not set m_initial_modules_added = true. Would that fix your issue?

Yes, it does fix my bug, thanks for the suggestion. Would you like me to remove the lines 445-447 that I added, or keep them as a secondary protection (since module_sp != m_interpreter_module.lock() condition doesn't seem to be strong enough).

In D109797#3003265, @labath wrote:

Also, what makes Android 28 special? Is it the presence/absence of a dynamic linker symlink? I'm wondering if we could reproduce (test) this more generally by ensuring we launch a binary through a symlink (or not).

On Android, GetExecutableModule() returns GetModuleAtIndex(0). For Android 27/29, this returns app_process64 (correct), but for Android 28 it returns [vdso], which is incorrect. This causes ResolveExecutableModule() to fall through due to module_spec (app_process64) not matching module_sp ([vdso]). This causes target.SetExecutableModule to be called, which triggers ClearModules. Since all modules are cleared, we cannot get the load address of the .dynamic section when we check whether we should rebase (i.e., addr.GetLoadAddress(&target) returns invalid address), thus rebase_exec=true. The rest is in CL description.

To test this, we would need a binary that (1) doesn't have eTypeExecutable in any of its modules (2) has a module other than the executable module at location 0.

[labath]

In D109797#3004232, @emrekultursay wrote:

In D109797#3003265, @labath wrote:

Since the meaning of m_initial_modules_added is basically "should I do a full scan through the linked list" and LoadAllCurrentModules (called from DidAttach) does a full scan, it seems to me the real bug is that this function does not set m_initial_modules_added = true. Would that fix your issue?

Yes, it does fix my bug, thanks for the suggestion. Would you like me to remove the lines 445-447 that I added, or keep them as a secondary protection (since module_sp != m_interpreter_module.lock() condition doesn't seem to be strong enough).

I suppose keeping it is fine. It will make sure m_interpreter_module is up to date, if anyone needs to access it in the future.

In D109797#3003265, @labath wrote:

Also, what makes Android 28 special? Is it the presence/absence of a dynamic linker symlink? I'm wondering if we could reproduce (test) this more generally by ensuring we launch a binary through a symlink (or not).

On Android, GetExecutableModule() returns GetModuleAtIndex(0). For Android 27/29, this returns app_process64 (correct), but for Android 28 it returns [vdso], which is incorrect. This causes ResolveExecutableModule() to fall through due to module_spec (app_process64) not matching module_sp ([vdso]). This causes target.SetExecutableModule to be called, which triggers ClearModules. Since all modules are cleared, we cannot get the load address of the .dynamic section when we check whether we should rebase (i.e., addr.GetLoadAddress(&target) returns invalid address), thus rebase_exec=true. The rest is in CL description.

To test this, we would need a binary that (1) doesn't have eTypeExecutable in any of its modules (2) has a module other than the executable module at location 0.

I'm not sure all of this is relevant. I've just tried to debug a very simple (linux) executable that does a dlopens a library. If I attach to the executable before the dlopen call, lldb does not notice the new library. This patch fixes the issue. So I guess the test could be just that. You could do something similar to TestLoadUnload -- either add a new test case which attaches to that binary, or use it as inspiration to write a new test.

[emrekultursay]

Add new dlopen test

[emrekultursay]

Minor touch-ups to the newly added test.

[emrekultursay]

In D109797#3005885, @labath wrote:

I'm not sure all of this is relevant. I've just tried to debug a very simple (linux) executable that does a dlopens a library. If I attach to the executable before the dlopen call, lldb does not notice the new library. This patch fixes the issue. So I guess the test could be just that. You could do something similar to TestLoadUnload -- either add a new test case which attaches to that binary, or use it as inspiration to write a new test.

PTAL. I added a new dlopen test for this. I can only test on Linux at the moment, so LMK if there's something I need to do to support more platforms or remove them from testing matrix.

[emrekultursay]

Update test to pass on MacOS

[emrekultursay]

Minor fix in test.

[emrekultursay]

plabath/mgorny: This change is ready for another round of review. Thanks.

[labath]

The test looks mostly fine. I made some comments on how to make it compatible with windows. I'm not sure if it would be enough to make it run there, but I think it has a fair chance. I might just let it run and then disable it if it turns out to be failing.

lldb/test/API/functionalities/dlopen/TestDlopen.py

14–16 ↗

(On Diff #374027)

see self.platformContext.shlib_prefix/extension for how to portably build this path (TestLoadUnload uses a mixture of these, as only those tests which work/make sense on windows have been updated).

lldb/test/API/functionalities/dlopen/main.cpp

8–10 ↗

(On Diff #374027)

why this argument dance? can we remove it?

26 ↗

(On Diff #374027)

see dylib.h and the functions within (the inferior of TestLoadUnload uses them) for a windows-compatible way to load shared libraries.

[emrekultursay]

Make test also run on Windows.

[emrekultursay]

PTAL. The test now passes on Linux, MacOS, and Windows.

lldb/test/API/functionalities/dlopen/main.cpp

26 ↗

(On Diff #374027)

Since we are attaching to an already running process, which cannot find the dynamic library unless I pass the full path to dlopen(). That's why I couldn't use dylib.h (which doesn't add full path), but created my own version here.

[labath]

lgtm, modulo comments.

lldb/test/API/functionalities/dlopen/TestDlopen.py

1 ↗

(On Diff #374237)

I'd put this under functionalities/load_after_attach/TestLoadAfterAttach.py to better fit into the existing naming scheme, and to emphasize the attach aspect of the test (as that is what really makes this test special).

lldb/test/API/functionalities/dlopen/b.cpp

1–4 ↗

(On Diff #374237)

What's up with the whitespace?

lldb/test/API/functionalities/dlopen/main.cpp

11–17 ↗

(On Diff #374237)

std::this_thread::sleep_for

26 ↗

(On Diff #374027)

Ok, what I think you're saying is that when we run a process for attaching, we skip the code paths which set ((DY)LD_LIBRARY_)PATH, which is what makes the relative imports work. It shouldn't be too hard to extend the launch infrastructure to do that. Let's commit this in this form, and I'll do that as a follow-up.

[emrekultursay]

Renamed test to load_after_attach.

[emrekultursay]

Done. Can you also submit it please?

lldb/test/API/functionalities/dlopen/main.cpp

26 ↗

(On Diff #374027)

Yes. That SGTM. Thanks.

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

if (m_rendezvous.ModulesDidLoad() || !m_initial_modules_added) {
     }
     for (; I != E; ++I) {
       ModuleSP module_sp =
           LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
       if (module_sp.get()) {
         if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
                 &m_process->GetTarget()) == m_interpreter_base &&
             module_sp != m_interpreter_module.lock()) {
+          if (m_interpreter_module.lock() == nullptr) {
+            m_interpreter_module = module_sp;
+          } else {
             // If this is a duplicate instance of ld.so, unload it.  We may end up
             // with it if we load it via a different path than before (symlink
             // vs real path).
             // TODO: remove this once we either fix library matching or avoid
             // loading the interpreter when setting the rendezvous breakpoint.
             UnloadSections(module_sp);
             loaded_modules.Remove(module_sp);
             continue;
           }
+        }
 
         loaded_modules.AppendIfNeeded(module_sp);
         new_modules.Append(module_sp);
       }
     }
     m_process->GetTarget().ModulesDidLoad(new_modules);
   }