Moving executable module symbols parsing to target creation method.

[PatriosTheGreat]

Summary

In our project we are using remote client-server LLDB configuration.
We want to parse as much debugging symbols as we can before debugger starts attachment to the remote process.
To do that we are passing the path of the local executable module to CreateTarget method at the client.
But, it seems that this method are not parsing the executable module symbols.
To fix this I added PreloadSymbols call for executable module to target creation method.

Another problem is that after attaching to a process, DynamicLoader tries to resolve the executable.
To do this, it compares the target executable module with the executable module from the remote process only by path + architecture.
Since the local path to the executable module and the remote path may differ, the DynamicLoader decides to reset the target executable module.
Which leads to the re-parsing of the module symbols after.
To fix this, I added a GetModuleSpec call from the Dynamic Loader.
This call gets full module information from remote process, including the UUID.

[jarin]

lldb/source/Target/TargetList.cpp

402

I think we should only preload if target_sp->GetPreloadSymbols() is set (just like it is done in Target::GetOrCreateModule).

[labath]

The idea of fetching&comparing the uuid sounds right. The part that bothers me about the implementation is that the process for finding executables is very different from that of shared libraries. It doesn't sound like the search process should care about the exe-vs-lib distiction too much (or at all). Process::GetModuleSpec will be called for shared libraries (and so I believe the reinitialization problem will not occur for those) , but only after passing through several other layers (we have way too many of those). I believe the path would be something like DynamicLoader::LoadModuleAtAddress -> Target::GetOrCreateModule -> Platform::GetSharedModule -> Platform::GetRemoteSharedModule -> Process::GetModuleSpec.

Ideally we would hook in slightly higher in this chain. Have you looked at that possibility. In an ideal world I would say that even LoadModuleAtAddress could be reused for executables, but I think that would be quite tricky. However, Target::GetOrCreateModule or Platform::GetSharedModule looks like it could be achieved.

lldb/source/Target/TargetList.cpp

402

yep

[PatriosTheGreat]

Hi Pavel.

Here is another way to fix a problem in the behavior of the dynamic loader.
The ResolveExecutableModule method already calls the platform_sp->ResolveExecutable to get executable module.
So all layers that you talked about should be called, if I'm not mistaken.
The only thing I need to add here is to set new executable module only if UUID is different.

[labath]

Well... that's an interesting take. If it works, I think I'd like it, but I do have two questions/concerns about it:

• the uuid check in the dynamic loader... it feels like that should be a direct pointer comparison instead. If everything works out right, I would expect that this function would return the exact same Module object (the one you have preloaded symbols for). If it merely returns an "equivalent" module, then it means that it has determined it knows better and we probably shouldn't be overriding it. Or, more practically, it means it has done some work (like downloading the module file from the remote system) needlessly. Maybe the real bug here is that this function should try harder to reuse one of the existing modules we have around? I am not sure, I always get entangled when I look at the module searching code. However, it may be interesting to compare what happens here vs what happens when we're adding a shared library that is already present in the target (perhaps because we're re-launching the process).
• eliding the SetExecutableModule completely probably isn't right, as it has other effects besides changing the "executable" module. This should be easy to solve with a small refactor, or it may not even be necessary if we can ensure that the old and new module_sp objects are one and the same.

[PatriosTheGreat]

Currently m_platform->ResolveExecutable will create new module.
Moreover this method will add new module to SharedModuleList.

The problem is that we it calls PlatformPOSIX::ResolveExecutable with specs that doesn't contains UUID.
PlatformPOSIX::ResolveExecutable trying to get executable specs by calling:
Platform::GetCachedExecutable -> Platform::GetRemoteSharedModule -> Platform::GetModuleSpec() ->PlatformRemoteGDBServer::ResolveExecutable -> ModuleList::GetSharedModule to get module specs with proper UUID.

However Platform::GetCachedExecutable does not update module_spec.uuid, but it updates FileSpec.

After that PlatformPOSIX::ResolveExecutable will try to get target module from SharedModule without passing UUID.
Which cause ModuleList to create new module in shared modules list.

One way to fix this is to set UUID to module_spec in Platform::GetCachedExecutable.
I'm not so sure how proper this solution is, but it seems to work and after that PlatformPOSIX::ResolveExecutable will return same module.

[labath]

Hmm.. I think you're onto something here.

I can see how updating the module spec UUID will fix things, but it does leave one to wonder why is PlatformPOSIX::ResolveExecutable re-resolving the module if GetCachedExecutable has already found one. I think that the proper fix would be to stop doing that. It seems we should just return immediately after calling GetCachedExecutable, regardless of whether it has found a module or not.

It may seem somewhat radical, but the truth is that GetCachedExecutable will end up calling PlatformRemoteGDBServer::ResolveExecutable which will do the exact same ModuleList::GetSharedModule dance that PlatformPOSIX::ResolveExecutable will repeat. In fact, looking at the history, it seems this used to be a direct m_remote_platform->ResolveExecutable call before a layer of caching was added.

It's hard to say whether this thing will break in some corner case, as this code is severely under-tested and the nobody fully understands how it works. But the only way to improve that situation is to start removing layers instead of adding them (in similar spirit as we did in D77529). Therefore, I say we fix that and deal with any potential fallout.

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

754

Now that this returns the exact same module, are you sure that you need to do anything special here? It seems like re-setting the target's executable module to the exact same object should be harmless...

[PatriosTheGreat]

Hi Pavel.

This solution seems to work.
Also I don't see additional failures in test suite compared to master branch.

[labath]

In D78654#2012517, @PatriosTheGreat wrote:

Hi Pavel.

This solution seems to work.
Also I don't see additional failures in test suite compared to master branch.

Cool. Now could you please reply to inline comment I left the last time.

Once that is settled, we can figure out what to do about testing. :D

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

113 ↗

(On Diff #261195)

Now you can remove this else branch and un-indent the code inside.

[PatriosTheGreat]

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

754

After second call of SetExecutableModule lldb will re-parse executable module on SetJITBreakpoint call.
The problem is that SetExecutableModule will add executable module to target.m_images list even if it's already there.
https://github.com/llvm/llvm-project/blob/master/lldb/source/Target/Target.cpp#L1403

I can fix this inside SetExecutableModule or I can leave this check before call.
What you think would be better?

[labath]

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

754

Hm... It is true that it is adding it, but it is also removing all modules before that (line 1394).

And that should be fine -- since we're just talking about pointers to the exact same object, deleting and re-adding it should be fine.

And it should not cause any re-parsing as all of the parsed information is tied to a module instance. So if there's still some re-parsing going on after this, that's probably a bug in the parsing/caching code...

[PatriosTheGreat]

I re-checked the behavior with the executable and this solution works without re-parsing. It seems like I mismatched logs previously.

Regarding the tests, I found the ListenToModuleLoadedEvents test, which seems to check for module loading events. I'm not sure if this the correct way to check an executable module parsing. However, I could not find tests which checks the symbols preloading logic. Maybe you know any tests on that logic?

[labath]

I am happy that we have converged to a solution, and I am very happy about how simple the solution has turned out to be.

As for testing. This patch is basically two independent patches (adding preloading for the main executable and making sure it does not get overwritten), and so I think we should treat it as such.

Testing preloading is tricky as if it's done right, it shouldn't have any observable effects. We have a test (TestLoadUnload) which runs some checks both with preloading enabled and disabled, and I think we could say that this change is already covered by that test.

The not-overwriting of a module does have a visible effect, and we should be able to create a test for it. At the highest level, I think we could observe the difference is with target.GetExecutable(). Without this patch I believe that will return the path the module was downloaded into (cached), whereas with the patch it should return the path that was passed to CreateTarget. It could also be observed more directly by calling ResolveExecutable.

These are roughly the two possible testing strategies.

• a c++ unit test which creates a PlatformPOSIX (with a mock remote platform?) and calls ResolveExecutable
• a full integration test which tests the effects of ResolveExecutable indirectly

Both require mocking the "remote system" at some level. The unit test approach has bigger unknowns, as we don't have any similar test to compare this to. However, Platform objects are relatively standalone, so I am hoping that it wouldn't be too hard.

For the intergration test, we have a bunch of existing tests which are roughly similar (in test/API/functionalities/gdb_remote_client/), but I believe this test would be a bit more complicated than the existing tests (which are already not trivial), as it would require mocking of both the "platform" and "server" connections (so far we've only needed to mock one or the other).

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

734

Please revert this change, as you're no longer even touching this file.

[PatriosTheGreat]

Hi Pavel.

Thanks for review and advice regarding tests.
I decide to go with unit test way, it seems to be more obvious what this test checks.

[labath]

Yes, this looks fine now. Thanks for your patience.

lldb/unittests/Platform/POSIX/PlatformPOSIXTest.cpp

1–2 ↗

(On Diff #263121)

bad formatting

[PatriosTheGreat]

Hi Pavel
Thank you for review.

Could you take this to master, since I don't have commit access?

[jarin]

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

112 ↗

(On Diff #263121)

Should not you do the same change in PlatformWindows.cpp, as well?

[labath]

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

112 ↗

(On Diff #263121)

Actually, the fix there should be to merge these two functions. I've now done that, rebased the patch and commited as 631048e81178. Since I had to modify the test to account for the move anyway, I also took the opportunity to gmock-ify it.

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

if (!m_process->GetProcessInfo(process_info)) {
     return;
   }
 
   LLDB_LOGF(
       log, "DynamicLoaderPOSIXDYLD::%s - got executable by pid %" PRIu64 ": %s",
       __FUNCTION__, m_process->GetID(),
       process_info.GetExecutableFile().GetPath().c_str());
 
-  ModuleSpec module_spec(process_info.GetExecutableFile(),
-                         process_info.GetArchitecture());
+  ModuleSpec module_spec;
+  m_process->GetModuleSpec(process_info.GetExecutableFile(),
+                           process_info.GetArchitecture(), module_spec);
   if (module_sp && module_sp->MatchesModuleSpec(module_spec))
     return;
 
   const auto executable_search_paths(Target::GetDefaultExecutableSearchPaths());
   auto error = platform_sp->ResolveExecutable(
       module_spec, module_sp,
       !executable_search_paths.IsEmpty() ? &executable_search_paths : nullptr);
   if (error.Fail()) {
     StreamString stream;
     module_spec.Dump(stream);
 
     LLDB_LOGF(log,
               "DynamicLoaderPOSIXDYLD::%s - failed to resolve executable "
               "with module spec \"%s\": %s",
               __FUNCTION__, stream.GetData(), error.AsCString());
     return;
   }
 
   target.SetExecutableModule(module_sp, eLoadDependentsNo);
 }
 
 bool DynamicLoaderPOSIXDYLD::AlwaysRelyOnEHUnwindInfo(
     lldb_private::SymbolContext &sym_ctx) {
   ModuleSP module_sp;
   if (sym_ctx.symbol)
     module_sp = sym_ctx.symbol->GetAddressRef().GetModule();
   if (!module_sp && sym_ctx.function)
     module_sp =
         sym_ctx.function->GetAddressRange().GetBaseAddress().GetModule();
   if (!module_sp)
     return false;
 
   return module_sp->GetFileSpec().GetPath() == "[vdso]";
 }

lldb/source/Target/TargetList.cpp

if (error.Success() && exe_module_sp) {
         }
         return error;
       }
       target_sp.reset(new Target(debugger, arch, platform_sp, is_dummy_target));
       target_sp->SetExecutableModule(exe_module_sp, load_dependent_files);
       if (user_exe_path_is_bundle)
         exe_module_sp->GetFileSpec().GetPath(resolved_bundle_exe_path,
                                              sizeof(resolved_bundle_exe_path));
+
+      exe_module_sp->PreloadSymbols();
     }
   } else {
     // No file was specified, just create an empty target with any arch if a
     // valid arch was specified
     target_sp.reset(new Target(debugger, arch, platform_sp, is_dummy_target));
   }
 
   if (target_sp) {