[lldb-vscode] Ensure that target matches the executable file

[anton.kolesov]

Summary

This commit fixes an issue with lldb-vscode failing to run programs that
use different architecture/platform than the "empty" in the target.
Original implementation was creating a default target without specifying
the target architecture, platform or program, and then would set
executable file through SBLaunchInfo, assuming that this would update
architecture and platform accordingly. However this wasn't really
happening, and architecture and platform would remain at whatever values
were in the "empty" target. The simple solution is to create target
already for a desired architecture and platform.

Function request_attach is updated in a similar fashion.

This commit also adds new JSON properties to "launch" and "attach"
packets to allow user to override desired platform and architecture.
This might be especially important for cases where information in ELF is
not enough to derive those values correctly.

New code has a behavior similar to LLDB MI [1], where typically IDE would
specify target file with -file-exec-and-symbols, and then only do -exec-run
command that would launch the process. In lldb-vscode those two actions are
merged into one request_launch function. Similarly in the interpreter
session, user would first do "file" command, then "process launch"

[1] https://github.com/lldb-tools/lldb-mi/blob/443e9592b27afd46828ddeef5f5439aad8207936/src/MICmdCmdFile.cpp#L108

[labath]

This needs a test case, though judging by your description it looks like pretty much every test case on affected platforms (I guess, windows) should hit this. Can you check if any of the existing vscode tests which are marked @skipIfWindows start passing now?

[clayborg]

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.

lldb/tools/lldb-vscode/lldb-vscode.cpp

1358 ↗

(On Diff #231514)

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.

[clayborg]

The other fix would be to add a new LLDB API function:

lldb::SBModule lldb::SBTarget::SetExecutable(const char *path, const char *triple, const char *symfile);

Where we would pass in "nullptr" for "triple" and "symfile" from lldb-vscode and we call that. But I do think fix SBTarget::AddModule to "do the right thing" is a good fix.

[anton.kolesov]

In D70847#1763797, @labath wrote:

This needs a test case, though judging by your description it looks like pretty much every test case on affected platforms (I guess, windows) should hit this. Can you check if any of the existing vscode tests which are marked @skipIfWindows start passing now?

Yes, that should be tested by existing launch tests already. My case is a proprietary Process plugin, not a Windows target and because my target is an embedded system with no ability to pass environemnt variables, command line arguments or an implementation of process exit I can't really run the existing tests for my target. I will try to build lldb for Windows and see if lldb-vscode tests will start working on it - as far as I can see all lldb-vscode tests are currently skipped for Windows.

[anton.kolesov]

Reimplement the solution based on a comment from Greg.

Tested with testsuite on Linux for x64 - there are no changes in test results. Also manually tested with a proprietary process plugin - currently it is not possible to run existing testsuite with that plugin because it targets embedded systems which has no cwd, exit() or environment.

I also attemted to test this against Windows hosts and targets. The change, apparently, resolves that launch capability on Windows, however setting of breakpoints doesn't work - it is only possible to stop application on entry, and on resume it will run till the end. Among the existing lldb-vscode tests some of them depend on <unistd.h> unavailable on windows, others fail because of the breakpoints that don't work.

[anton.kolesov]

Ping.

[clayborg]

context is missing with this diff (use git diff -U9999999). Changes look good. We do need a test though. All we need to do is create a target with nothing, then add the image to the target and make sure the platform and arch are updated. No need to run anything. Seeing as this wasn't working before this change, we don't have a test that covers it. You can create an ELF file and run obj2yaml on it and save the text file into the test directory. The test can run yaml2obj to generate the file, create an empty target, use the API that his patch fixes and very info on the target.

[anton.kolesov]

Added a testcase. Because target's architecture is not directly exposed through an API, test looks at the target triplet - it is empty for targets created without an exe file. Without the patch, triplet remains unchanged after adding an executable, but with the patch, it changes to the architecture of the executable file, whichever it is.

[clayborg]

So it might be better to create a ELF file for a remote platform, then run "obj2yaml" on it. Then the test will run "yaml2obj" on it, there are many test cases that do this, so look for "*.yaml" in the test directories. Then the test can check for a hard coded target triple, and also verify that the platform was changed. I will add inline comments for clarification in the test python file.

lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py

16 ↗

(On Diff #240533)

Create a yaml file and put it in the same directory as this file. Change this line to be:

src_dir = self.getSourceDir()
yaml_path = os.path.join(src_dir, "a.yaml")
obj_path = self.getBuildArtifact("a.out")
self.yaml2obj(yaml_path, obj_path)

Make an ELF file that that is for a remote platform where the triple and platform won't match typical hosts.

21 ↗

(On Diff #240533)

Don't know if I would run this assert here. I could see a target that is created with nothing possibly using the host architecture and host platform by default. So maybe remote this assert where the target triple is not set.

26 ↗

(On Diff #240533)

Check the exact triple of the remote executable you end up adding.

Also add a check that the platform gets updated. Platforms must be compatible with the architecture of the target's main executable, so it is good to verify that the platform gets updated as well. Something like:

platform = target.GetPlatform()
set.assertEqual(platform.GetName(), "remote-linux")

This will make this test complete. If the platform isn't right we will need to fix this as well.

[anton.kolesov]

lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py

16 ↗

(On Diff #240533)

Do you have any specific suggestions that also have freely available toolchain? I have immediate access only to Windows/Linux x86 and Synopsys ARC, and ARC elf files don't change the platform - it remains as "host" even when connecting to a GDB-server, plus obj2yaml segfaults on ARC elf files.

21 ↗

(On Diff #240533)

As far as I can understand from the TargetList::CreateTargetInternal, new target will not inherit architecture from the platform when exe file is not specified and neither architecture nor platform are given explicitly. So it seems that assuming that triplet will be empty is safe.

26 ↗

(On Diff #240533)

The platform doesn't change with the current code. Target::SetExecutableModule does direct assignment of architecture and doesn't touch the platform, so this is needed:

--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1405,7 +1405,7 @@ void Target::SetExecutableModule(ModuleSP &executable_sp,
     // If we haven't set an architecture yet, reset our architecture based on
     // what we found in the executable module.
     if (!m_arch.GetSpec().IsValid()) {
-      m_arch = executable_sp->GetArchitecture();
+      SetArchitecture(executable_sp->GetArchitecture(), true);
       LLDB_LOG(log,
                "setting architecture to {0} ({1}) based on executable file",
                m_arch.GetSpec().GetArchitectureName(),

It works for me in the test case, but I wonder if SetArchitecture wasn't used there in the first place for a good reason.

[labath]

lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py

16 ↗

(On Diff #240533)

Would this do the job?

--- !ELF
FileHeader:
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_DYN
  Machine:         EM_AARCH64
  Entry:           0x00000000000006C0
Sections:
  - Name:            .text
    Type:            SHT_PROGBITS
    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
    Address:         0x00000000000006C0
    AddressAlign:    0x0000000000000008
    Content:         DEADBEEF
...

[anton.kolesov]

lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py

16 ↗

(On Diff #240533)

It is definitely needed to change ET_DYN to ET_EXEC in this yaml, because the patch specifically modifies behavior only for executables. With that change this works on my x86_64/Linux, but I don't known how it will behave if test would be run on aarch64 host - will platform still change to "remote-linux" for this ELF or will it remain at "host"? I'm not sure how this process of platform selection works - can it distinguish between elf for aarch64-linux and elf for baremetal aarch64-elf?

[labath]

Hmm... now that I actually have read what this patch does, I am starting to think that this is not a good idea:

• on linux it was possible to load an executable via "dlopen", at least until last year, when it was (partially?) disabled by https://patchwork.ozlabs.org/patch/1055380/
• opening an executable via dlopen still works on a mac (10.15.1 Catalina), though I'm not sure how much it's officially supported
• on windows LoadLibrary is explicitly documented to be able to open .exe files

Now given that the effect of SetExecutableModule is to completely destroy the information about other loaded shared libraries, and that dlopening an exe works (for some definition of "works") on all major platforms, I don't think calling this function in response to a module being added (e.g. because of dlopen) is reasonable.

I think we need to go back to the drawing board and handle this at a different level -- either by changing lldb-vscode, or some of the higher-level launching logic.

[anton.kolesov]

My original patch was changing lldb-vscode - it was creating a new Target object with ELF given to a constructor. That patch, though definitely would need an update - it should delete the original empty target after creating the new one, but it was leaving it alive. To me this solution looks preferable, because I'm a fanboy for immutable objects in general, and in this specific case I don't fully understand all of the potential side effects of the SetExecutable call.

[clayborg]

We can do this in lldb-vscode.cpp. When I first made the lldb-vscode plug-in I was sending the "initialized" notification back to the IDE too early. This resulted in breakpoint packets being sent _before_ "launch" or "attach" packets. A good change would be to default construct the g_vscode.target, and set the target in "launch" and "attach". Now that the "initialized" packet is being sent after the "launch" or "attach" packets, we can make the target in "launch" or "attach" handler functions with no issues.

This will also allow us to add "triple" and "platform" as new key/value pairs for "launch" and "attach" so they can be correctly set before the executable is set in the target. Why is this important? Because many ELF files don't have enough info in them to properly set the vendor and OS fields of a triple correctly. So often we need to manually specify the triple and platform (more than one platform can use ARM elf files: remote-linux, remote-android for example. And if we just create a target with an ELF file that doesn't have any ELF notes, we might set things up incorrectly. So I believe this would also fix your problem.

[clayborg]

To further clarify the issue with the "initialized" packet being sent too early: if it gets sent too early, then the breakpoints packets immediately follow and we will need a target to set the breakpoints in. This is why en empty target was created. If the target is now set in "launch" or "attach", we will do the launch/attach and then send the "initialized" packet, the breakpoint packets will be sent, and we will have a target to set them in.

[anton.kolesov]

Reverted to the original idea of modifying lldb-vscode. Unlike first version, this commit also modifies request_attach to have the same behaviour. Two new properties are added to request arguments: "targetTriple" and "platformName" to specify values to respective arguments of SBDebugger::CreateTarget().

[clayborg]

Very nice, just need to add a VSCode::SetTarget(...) as detailed in inlined comments and this will be good to go.

lldb/tools/lldb-vscode/lldb-vscode.cpp

113 ↗

(On Diff #243828)

Better to add a:

void VSCode::SetTarget(lldb::SBTarget target)

and this to that function

539 ↗

(On Diff #243828)

Add a SetTarget() to the g_vsc struct definition and do the breakpoint listening there if the target is valid?

562 ↗

(On Diff #243828)

Move to VSCode::SetTarget(lldb::SBTarget) function to avoid having to do this in two functions.

1408 ↗

(On Diff #243828)

call g_vsc.SetTarget(g_vsc.debugger.CreateTarget(...))

1431 ↗

(On Diff #243828)

This will be in VSCode::SetTarget, remove.

[labath]

This approach looks good to me. It'd just be good to do something about the code duplication.

[anton.kolesov]

Updated in attempt to reduce amount of code duplication between request_attach and request_launch.
Built and tested with testsuite on Linux/x64, also built and manually tested on Windows/x64 host with a baremetal ARC cpu target.

[clayborg]

Looks great, even more refactoring than asked. Thanks for sticking with this doing the work!

lldb/source/Target/Target.cpp

     // there wasn't an equivalent module in the list already, and if there was,
     // let's remove it.
     if (module_sp) {
+      bool isExecutable = false;
       ObjectFile *objfile = module_sp->GetObjectFile();
       if (objfile) {
         switch (objfile->GetType()) {
+        case ObjectFile::eTypeExecutable: /// A normal executable
+          isExecutable = true;
+          LLVM_FALLTHROUGH;
         case ObjectFile::eTypeCoreFile: /// A core file that has a checkpoint of
                                         /// a program's execution state
-        case ObjectFile::eTypeExecutable:    /// A normal executable
         case ObjectFile::eTypeDynamicLinker: /// The platform's dynamic linker
                                              /// executable
         case ObjectFile::eTypeObjectFile:    /// An intermediate object file

         } else {
           m_images.Append(module_sp, notify);
         }
+
+        // Ensure that architecture of the Target matches that of the
+        // executable file. Otherwise Target might use a "default" platform
+        // that can't actually debug the executable. For example, if the Target
+        // is created and by default assumes that it should use "gdb-remote"
+        // process, however executable has an architecture that requires a
+        // different Process class - without explicitly set executable module
+        // Target would attempt to use "gdb-remote" created initially.
+        if (isExecutable)
+          SetExecutableModule(module_sp, eLoadDependentsNo);
       } else
         module_sp.reset();
     }