[Lldb-commits] [PATCH] D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D72751#1848384 <https://reviews.llvm.org/D72751#1848384>, @paolosev wrote:

> Regarding:
>
> >> - make the base DynamicLoader class instantiatable, and use it whenever we fail to find a specialized plugin
> >> - same as above, but only do that for ProcessGDBRemote instances
> >> - make ProcessGDBRemote call LoadModules() itself if no dynamic loader instance is available WDYT?
> > 
> > I am fine with 1 as long as we document the DynamicLoader class to say that it will call Process::LoadModules() and will be used if no specialized loader is needed for your platform. I would like to a see a solution that will work for any process plug-in and not just ProcessGDBRemote. If we change solution 3 above to say "Make lldb_private::Process call LoadModules() itself if no dynamic loader instance is available" then solution 3 is also fine.
>
> there is a problem: if I remove DynamicLoaderWasmDYLD what happens is that `DynamicLoaderStatic` is found as a valid loader for a triple like **"wasm32-unknown-unknown-wasm"** because the Triple::OS is llvm::Triple::UnknownOS (I found out the hard way when I was registering DynamicLoaderWasmDYLD after DynamicLoaderStatic :-)).
>
>   ...
>   
>
> call stack:
>  DynamicLoaderStatic::CreateInstance(lldb_private::Process * process, bool force) Line 29
>  DynamicLoader::FindPlugin(lldb_private::Process * process, const char * plugin_name) Line 52
>  lldb_private::process_gdb_remote::ProcessGDBRemote::GetDynamicLoader() Line 3993
>  lldb_private::Process::CompleteAttach() Line 2931
>  lldb_private::Process::ConnectRemote(lldb_private::Stream * strm, llvm::StringRef remote_url) Line 3022
>
>   Could ProcessGDBRemote::GetDynamicLoader behave differently just when the architecture is wasm32, maybe? But then it is probably cleaner to add this plugin class, what do you think?


Well.. I think DynamicLoaderStatic is being too grabby. :)

However, when I though about that idea further, I realized that any default plugin we could implement this way could not be complete, as we would be missing the part which (un)loads modules when a new shared library (dis)appears. This is something that cannot be done in a generic way, as that usually requires setting breakpoint on some special symbol, or getting notifications about shared library events in some other way. I don't know whether this is something that you will also need/plan to implement for wasm, or if one can assume that all modules are loaded from the get-go, but this is what convinced me that putting this in a separate plugin is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72751/new/

https://reviews.llvm.org/D72751