[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I think the fix is better done to fix the root cause issue instead of working around it. I have suggested a fix in inline comments.



================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1358
+    // Create a target that matches the file.
+    g_vsc.target = g_vsc.debugger.CreateTarget(program.data());
 
----------------
You will lose any state that was set in the original target by "g_vsc.RunInitCommands();" if you do this. Not sure if this matters since users can use "RunPreRunCommands"...

Also the original g_vsc._target had tried to listen to events from this target. From the "initialize" code:

```
void request_initialize(const llvm::json::Object &request) {
  g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
  // Create an empty target right away since we might get breakpoint requests
  // before we are given an executable to launch in a "launch" request, or a
  // executable when attaching to a process by process ID in a "attach"
  // request.
  ...
  g_vsc.target = g_vsc.debugger.CreateTarget(nullptr);
  lldb::SBListener listener = g_vsc.debugger.GetListener();
  listener.StartListeningForEvents(
      g_vsc.target.GetBroadcaster(),
      lldb::SBTarget::eBroadcastBitBreakpointChanged);
  listener.StartListeningForEvents(g_vsc.broadcaster,
                                   eBroadcastBitStopEventThread);
```
Though if this is patch works for you this must not be needed?

Do breakpoints work fine for you with this patch when you set them before you run? This comment might be out of date as I think I fixed when I sent the packet:

```
{"event":"initialized","seq":0,"type":"event"}
```

Until after the process was launched.

Where things seems to go wrong is in "Target::Launch(...)":

```
  // If we're not already connected to the process, and if we have a platform
  // that can launch a process for debugging, go ahead and do that here.
  if (state != eStateConnected && platform_sp &&
      platform_sp->CanDebugProcess()) {
```

The platform could be incorrect if the executable inside the launch info doesn't match the platform, or even if it works for the platform, it might not be the platform that exists. 

A better fix might be to modify Target::GetOrCreateModule(...) (which is called by SBTarget::AddModule()) as follows:

```
    if (module_sp) {
      bool isExecutable = false; // NEW CODE
      ObjectFile *objfile = module_sp->GetObjectFile();
      if (objfile) {
        switch (objfile->GetType()) {
          case ObjectFile::eTypeExecutable:    /// A normal executable
            isExecutable = true;  // NEW CODE (and moved)
            LLVM_FALLTHROUGH;  // NEW CODE
          case ObjectFile::eTypeCoreFile: /// A core file that has a checkpoint of
                                        /// a program's execution state
        case ObjectFile::eTypeDynamicLinker: /// The platform's dynamic linker
                                             /// executable
        case ObjectFile::eTypeObjectFile:    /// An intermediate object file
        case ObjectFile::eTypeSharedLibrary: /// A shared library that can be
                                             /// used during execution
          break;
        case ObjectFile::eTypeDebugInfo: /// An object file that contains only
                                         /// debug information
          if (error_ptr)
            error_ptr->SetErrorString("debug info files aren't valid target "
                                      "modules, please specify an executable");
          return ModuleSP();
        case ObjectFile::eTypeStubLibrary: /// A library that can be linked
                                           /// against but not used for
                                           /// execution
          if (error_ptr)
            error_ptr->SetErrorString("stub libraries aren't valid target "
                                      "modules, please specify an executable");
          return ModuleSP();
        default:
          if (error_ptr)
            error_ptr->SetErrorString(
                "unsupported file type, please specify an executable");
          return ModuleSP();
        }
        // GetSharedModule is not guaranteed to find the old shared module, for
        // instance in the common case where you pass in the UUID, it is only
        // going to find the one module matching the UUID.  In fact, it has no
        // good way to know what the "old module" relevant to this target is,
        // since there might be many copies of a module with this file spec in
        // various running debug sessions, but only one of them will belong to
        // this target. So let's remove the UUID from the module list, and look
        // in the target's module list. Only do this if there is SOMETHING else
        // in the module spec...
        if (!old_module_sp) {
          if (module_spec.GetUUID().IsValid() &&
              !module_spec.GetFileSpec().GetFilename().IsEmpty() &&
              !module_spec.GetFileSpec().GetDirectory().IsEmpty()) {
            ModuleSpec module_spec_copy(module_spec.GetFileSpec());
            module_spec_copy.GetUUID().Clear();

            ModuleList found_modules;
            size_t num_found =
                m_images.FindModules(module_spec_copy, found_modules);
            if (num_found == 1) {
              old_module_sp = found_modules.GetModuleAtIndex(0);
            }
          }
        }

        // Preload symbols outside of any lock, so hopefully we can do this for
        // each library in parallel.
        if (GetPreloadSymbols())
          module_sp->PreloadSymbols();

        if (old_module_sp && m_images.GetIndexForModule(old_module_sp.get()) !=
                                 LLDB_INVALID_INDEX32) {
          m_images.ReplaceModule(old_module_sp, module_sp);
          Module *old_module_ptr = old_module_sp.get();
          old_module_sp.reset();
          ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
        } else {
          m_images.Append(module_sp, notify);
        }
        if (isExecutable)  // NEW CODE
          SetExecutableModule(module_sp, false);  // NEW CODE
      } else
        module_sp.reset();
    }
```
Look for "// NEW CODE" in the above snippet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847