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

[Paolo Severini via Phabricator via lldb-commits]

paolosev added a comment.

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 :-)).
There is an explicit check for UnknownOS:

  DynamicLoader *DynamicLoaderStatic::CreateInstance(Process *process,
                                                     bool force) {
    bool create = force;
    if (!create) {
      const llvm::Triple &triple_ref =
          process->GetTarget().GetArchitecture().GetTriple();
      const llvm::Triple::OSType os_type = triple_ref.getOS();
      if ((os_type == llvm::Triple::UnknownOS))
        create = true;
    }
    ...
  
  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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72751