[lldb-vscode] Add Support for Module Event

[• aelitashen]

Summary

Whenever a module is created, removed or changed, lldb-vscode is now sending an event that can be interpreted by the IDE so that modules can be rendered in the IDE, like the tree view in this screenshot

[clayborg]

So great first patch Yifan! A few things to fix up, see inlined comments.

We need to add a test for lldb::SBTarget::eBroadcastBitSymbolsLoaded if possible. This can be done by stripping "a.out" into "a.out.stripped" and then debugging "a.out.stripped" and then adding symbols using "target symbols add ..." to add the "a.out".

lldb/tools/lldb-vscode/JSONUtils.cpp

335

LLVM coding guidelines limit source lines to 80 characters. So just reformat as requested here.

340

Ditto

343

Ditto

346

remove empty line.

lldb/tools/lldb-vscode/JSONUtils.h

241

Need to add header documentation like the other functions in this file.

lldb/tools/lldb-vscode/VSCode.cpp

357–359

reformat per pre-merge checks

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

440

you check for "lldb::SBTarget::eBroadcastBitSymbolsLoaded" here, but didn't listen for that event in VSCode.cpp. Either remove lldb::SBTarget::eBroadcastBitSymbolsLoaded or add it to the events we want to listen for in VSCode.cpp

441

rename "solib_count" to "num_modules"?

442–445

Use for loop:

for (int i=0; i<num_modules; ++i) {
  auto module = lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);

[labath]

lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py

27–29

replace assertTrue(needle in haystack) with assertIn(needle, haystack)

lldb/tools/lldb-vscode/JSONUtils.cpp

336

You should use SBFileSpec::GetPath to get the file and dir components separated by the correct directory separator. (It's usage is somewhat clunky due to the SB API restrictions).

[• aelitashen]

Formatting the codes

[clayborg]

We need to add a test for the symbols added target notification. See my previous comment on stripping a.out to a.out.stripped and then using "a.out.stripped" as the main executable.

lldb/test/API/tools/lldb-vscode/module/main.cpp

3–10

We can probably simplify this to just be:

int main(int argc, char const *argv[]) {
  return 0; // breakpoint 1
}

lldb/tools/lldb-vscode/JSONUtils.cpp

337–340

Let LLDB take care of the directory delimiter. Otherwise this will be bad on windows:

char module_path[PATH_MAX];
module.GetFileSpec().GetPath(module_path, sizeof(module_path));

344–346

Let LLDB take care of the directory delimiter. Otherwise this will be bad on windows:

char symbol_path[PATH_MAX];
module.GetSymbolFileSpec().GetPath(module_path, sizeof(symbol_path));

349–350

We need to make sure the address returned is valid and we want the string value to be in hex. Are we sure std::to_string() will create a hex number? If not we can use sprintf:

uint64_t load_addr = module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target);
if (load_addr != LLDB_INVALID_ADDRESS) {
  char load_addr_str[64];
  snprintf(load_addr_str, sizeof(load_addr_str), "0x%016" PRIx64, load_addr);
  object.try_emplace("addressRange", load_addr_str);
}

lldb/tools/lldb-vscode/JSONUtils.h

241

Might be better with text like:

/// Converts a LLDB module to a VS Code DAP module for use in "modules" events.

[clayborg]

Here is a makefile that does stripping:

llvm-project/lldb/test/API/lang/objc/objc-ivar-stripped/Makefile

Then when creating the target, use a.out.stripped:

exe = self.getBuildArtifact("a.out.stripped")
symbols = exe = self.getBuildArtifact("a.out")
target = self.dbg.CreateTarget(exe)

Then after you get the modules, you can do:

self.dbg.HandleCommand('target symbols add "%s"' % (symbols))

And that should trigger the symbols added notification and then you can fetch the modules again and verify that there is now a symbol file for "a.out.stripped".

[• aelitashen]

lldb/tools/lldb-vscode/JSONUtils.cpp

337–340

This piece of code will make path unreadable in the log, and cause the modules view cannot be displayed properly.

344–346

Same reason as above.

349–350

Decimal to Hex conversion is done in the IDE, not sure if we need to move it here.

[• aelitashen]

Make LLDB take care of the directory delimiter

[clayborg]

See inline comments. Changes needed are:

• test for eBroadcastBitSymbolsLoaded
• add "version" to module info
• test all data we expect in module info ('name', 'symbolFilePath', 'version', 'symbolStatus', 'addressRange')

lldb/test/API/tools/lldb-vscode/module/Makefile

4

We need to test eBroadcastBitSymbolsLoaded in this test. I would suggest we make an "a.out.stripped" (see the makefile form lldb/test/API/lang/objc/objc-ivar-stripped/Makefile for how to do this).

lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py

21

eBroadcastBitSymbolsLoaded needs to be tested: change this line to:

program_basename = 'a.out.stripped'
program = self.getBuildArtifact(program_basename)

after modifying the Makefile as suggested above.

28

Cached active modules, and use assertIn here and switch to "a.out.stripped", and verify the 'name' is in module info and that path is correct:

active_modules = self.vscode.get_active_modules()
self.assertIn(program_basename, active_modules, '%s module is in active modules' % (program_basename))
program_module = active_modules[program_basename]
self.assertIn('name', program_module, 'make sure 'path' key is in module')
self.assertEqual(program, program_module['name'])

30–31

use self.assertIn(...) and use the cached "active_modules" variable:

self.assertIn('symbolFilePath', program_module, 'Make sure 'symbolFilePath' key is in module info')
self.assertEqual(program, program_module['symbolFilePath'])

Then you need to verify all other information that you put into the Module like "symbolStatus" and "addressRange".

30–31

Now we would send a LLDB command line command to load the "a.out" symbols:

symbols = self.getBuildArtifact("a.out")
response = self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" "%s"' % (program, symbols)))

This will cause LLDB to load the symbol file "a.out" for the executable "a.out.stripped". This should cause the eBroadcastBitSymbolsLoaded notification to fire and the modules to get updated. Then you will get the active modules again and verify:

• program is still "program" (the full path to the a.out.stripped binary)
• 'symbolFilePath' is equal to our "symbols" variable from above (full path to "a.out")
• 'symbolStatus' is now set to 'Symbols Loaded.'
• 'addressRange' is still what we expect

lldb/tools/lldb-vscode/JSONUtils.cpp

344–345

Do we only want to try and add this "symbolFilePath" key if the path differs from "module_path"?

351

We should also fetch the version number from the module and populate the "version" key. To get the version number from a module you can use:

uint32_t version_nums[5];
const uint32_t num_versions = module.GetVersion(version_nums, sizeof(version_nums));
std::string version_str;
for (uint32_t i=0; i<num_versions; ++i) {
  if (!version_str.empty())
    version_str += ".";
  version_str += std::to_string(version_nums[i])
}
if (!version_str.empty()) 
  object.try_emplace("version", version_str);

[• aelitashen]

Add test for loading symbols, other module info and Add version to module info

[clayborg]

So looks like you didn't use "git commit --amend -a" again here. These changes are incremental changes on your patch which hasn't been submitted yet?

lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py

21

add a space after "program"

33–34

Do a similar check for ='path'

self.assertIn('name', program_module, 'make sure "name" is in module')
self.assertEqual(program_basename, program_module['name'])
self.assertIn('path', program_module, 'make sure "path" is in module')
self.assertEqual(program, program_module['path'])

In general, make sure you test every key that you have added support for.

35

Check for the symbol status here:

self.assertEqual('Symbols not found.', program_module['symbolStatus'])

40

verify 'path' again

lldb/tools/lldb-vscode/JSONUtils.cpp

329–349

Add an else clause here:

} else {
  object.try_emplace("symbolStatus", "Symbols not found.");
}

331–336

fix indentation.

337–339

remove {} here for single line if statement

[• aelitashen]

Fix messy diff stack

[clayborg]

Just test for paths and this will be good to go!

lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py

31

add a newline

36

add a check for the path:

self.assertEqual(program, program_module['path'])

43

check 'path' again here.

[• aelitashen]

Add Path Verification in Test

[clayborg]

Looks good!

[MaskRay]

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

[clayborg]

In D82477#2145565, @MaskRay wrote:

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

Can we modify this script to remove the unneeded tags instead of detecting it?

[MaskRay]

In D82477#2145967, @clayborg wrote:

In D82477#2145565, @MaskRay wrote:

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

Can we modify this script to remove the unneeded tags instead of detecting it?

@mehdi_amini ^^

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py

def __init__(self, recv, send, init_commands):
         self.recv_condition = threading.Condition()
         self.recv_thread = threading.Thread(target=read_packet_thread,
                                             args=(self,))
         self.process_event_body = None
         self.exit_status = None
         self.initialize_body = None
         self.thread_stop_reasons = {}
         self.breakpoint_events = []
+        self.module_events = {}
         self.sequence = 1
         self.threads = None
         self.recv_thread.start()
         self.output_condition = threading.Condition()
         self.output = {}
         self.configuration_done_sent = False
         self.frame_scopes = {}
         self.init_commands = init_commands
 
     @classmethod
     def encode_content(cls, s):
         return ("Content-Length: %u\r\n\r\n%s" % (len(s), s)).encode("utf-8")
 
     @classmethod
     def validate_response(cls, command, response):
         if command['command'] != response['command']:
             raise ValueError('command mismatch in response')
         if command['seq'] != response['request_seq']:
             raise ValueError('seq mismatch in response')
 
+    def get_active_modules(self):
+        return self.module_events
+        
     def get_output(self, category, timeout=0.0, clear=True):
         self.output_condition.acquire()
         output = None
         if category in self.output:
             output = self.output[category]
             if clear:
                 del self.output[category]
         elif timeout != 0.0:
def handle_recv_packet(self, packet):
                 self.thread_stop_reasons[tid] = body
             elif event == 'breakpoint':
                 # Breakpoint events come in when a breakpoint has locations
                 # added or removed. Keep track of them so we can look for them
                 # in tests.
                 self.breakpoint_events.append(packet)
                 # no need to add 'breakpoint' event packets to our packets list
                 return keepGoing
+            elif event == 'module':
+                reason = body['reason']
+                if (reason == 'new' or reason == 'changed'):
+                    self.module_events[body['module']['name']] = body['module']
+                elif reason == 'removed':
+                    if body['module']['name'] in self.module_events:
+                        self.module_events.pop(body['module']['name'])
+                return keepGoing
+
         elif packet_type == 'response':
             if packet['command'] == 'disconnect':
                 keepGoing = False
         self.enqueue_recv_packet(packet)
         return keepGoing
 
     def send_packet(self, command_dict, set_sequence=True):
         '''Take the "command_dict" python dictionary and encode it as a JSON

lldb/test/API/tools/lldb-vscode/module/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py

+"""
+Test lldb-vscode setBreakpoints request
+"""
+
+from __future__ import print_function
+
+import unittest2
+import vscode
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbvscode_testcase
+
+
+class TestVSCode_module(lldbvscode_testcase.VSCodeTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    
+    def test_modules_event(self):
+        program= self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = "main.cpp"
+        breakpoint1_line = line_number(source, '// breakpoint 1')
+        lines = [breakpoint1_line]
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
+        self.continue_to_breakpoints(breakpoint_ids)
+        self.assertTrue('a.out' in self.vscode.get_active_modules(), 
+                        'Module: a.out is loaded')
+        self.assertTrue('symbolFilePath' in self.vscode.get_active_modules()['a.out'],
+                        'Symbol exists')
 No newline at end of file

lldb/test/API/tools/lldb-vscode/module/main.cpp

+
+int multiply(int x, int y) {
+  return x * y; // breakpoint 1
+}
+
+int main(int argc, char const *argv[]) {
+  int result = multiply(argc, 20);
+  return result < 0;
+}

lldb/tools/lldb-vscode/JSONUtils.h

 //===-- JSONUtils.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TOOLS_LLDB_VSCODE_JSONUTILS_H
 #define LLDB_TOOLS_LLDB_VSCODE_JSONUTILS_H
 
 #include <stdint.h>
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
 #include "VSCodeForward.h"
+#include "lldb/API/SBModule.h"
 
 namespace lldb_vscode {
 
 /// Emplace a StringRef in a json::Object after enusring that the
 /// string is valid UTF8. If not, first call llvm::json::fixUTF8
 /// before emplacing.
 ///
 /// \param[in] obj

 /// \return
 ///     A "Breakpoint" JSON object with that follows the formal JSON
 ///     definition outlined by Microsoft.
 llvm::json::Value
 CreateBreakpoint(lldb::SBBreakpoint &bp,
                  llvm::Optional<llvm::StringRef> request_path = llvm::None,
                  llvm::Optional<uint32_t> request_line = llvm::None);
 
+llvm::json::Value CreateModule(lldb::SBModule &module);
+
 /// Create a "Event" JSON object using \a event_name as the event name
 ///
 /// \param[in] event_name
 ///     The string value to use for the "event" key in the JSON object.
 ///
 /// \return
 ///     A "Event" JSON object with that follows the formal JSON
 ///     definition outlined by Microsoft.

lldb/tools/lldb-vscode/JSONUtils.cpp

 //===-- JSONUtils.cpp -------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <algorithm>
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBBreakpointLocation.h"
 #include "lldb/API/SBValue.h"
 #include "lldb/Host/PosixApi.h"
 
 #include "ExceptionBreakpoint.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "VSCode.h"
if (line != UINT32_MAX)
       object.try_emplace("line", line);
     object.try_emplace("source", CreateSource(line_entry));
   }
   // We try to add request_line as a fallback
   if (request_line)
     object.try_emplace("line", *request_line);
   return llvm::json::Value(std::move(object));
 }
 
+llvm::json::Value CreateModule(lldb::SBModule &module) {
+  llvm::json::Object object;
+  if (!module.IsValid())
+    return llvm::json::Value(std::move(object));
+  object.try_emplace("id", std::string(module.GetUUIDString()));
+  object.try_emplace("name", std::string(module.GetFileSpec().GetFilename())); // Path in remote
+  std::string module_path = std::string(module.GetFileSpec().GetDirectory()) + "/" + std::string(module.GetFileSpec().GetFilename());
+  object.try_emplace("path", module_path);
+  if (module.GetNumCompileUnits() > 0) {
+    object.try_emplace("symbolStatus", "Symbols loaded.");
+    std::string symbol_path = std::string(module.GetSymbolFileSpec().GetDirectory()) + "/" +  std::string(module.GetSymbolFileSpec().GetFilename());
+    object.try_emplace("symbolFilePath", symbol_path);
+  }
+  std::string loaded_addr = std::to_string(module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target));
+  object.try_emplace("addressRange", loaded_addr);
+  return llvm::json::Value(std::move(object));
+
+}
+
 void AppendBreakpoint(lldb::SBBreakpoint &bp, llvm::json::Array &breakpoints,
                       llvm::Optional<llvm::StringRef> request_path,
                       llvm::Optional<uint32_t> request_line) {
   breakpoints.emplace_back(CreateBreakpoint(bp, request_path, request_line));
 }
 
 // "Event": {
 //   "allOf": [ { "$ref": "#/definitions/ProtocolMessage" }, {
 //     "type": "object",
 //     "description": "Server-initiated event.",
 //     "properties": {

lldb/tools/lldb-vscode/VSCode.cpp

void VSCode::SetTarget(const lldb::SBTarget target) {
   if (target.IsValid()) {
     // Configure breakpoint event listeners for the target.
     lldb::SBListener listener = this->debugger.GetListener();
     listener.StartListeningForEvents(
         this->target.GetBroadcaster(),
         lldb::SBTarget::eBroadcastBitBreakpointChanged);
     listener.StartListeningForEvents(this->broadcaster,
                                      eBroadcastBitStopEventThread);
+    listener.StartListeningForEvents(
+      this->target.GetBroadcaster(),
+      lldb::SBTarget::eBroadcastBitModulesLoaded | lldb::SBTarget::eBroadcastBitModulesUnloaded);                                
   }
 }
 
 } // namespace lldb_vscode

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

 #include <chrono>
 #include <fstream>
 #include <map>
 #include <memory>
 #include <mutex>
 #include <set>
 #include <sstream>
 #include <thread>
+#include <vector>
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Errno.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
if (listener.WaitForEvent(1, event)) {
             source_bp.getAsObject()->erase("source");
 
             body.try_emplace("breakpoint", source_bp);
             body.try_emplace("reason", "changed");
             bp_event.try_emplace("body", std::move(body));
             g_vsc.SendJSON(llvm::json::Value(std::move(bp_event)));
           }
         }
+      } else if (lldb::SBTarget::EventIsTargetEvent(event)) {
+        if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded ||
+            event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded ||
+            event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded) {
+          int solib_count = lldb::SBTarget::GetNumModulesFromEvent(event);
+          int i = 0;
+          while (i < solib_count) {
+            auto module = lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
+            i++;
+            auto module_event = CreateEventObject("module");
+            llvm::json::Value module_value = CreateModule(module);
+            llvm::json::Object body;
+            if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded) {
+              body.try_emplace("reason", "new");
+            } else if (event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded) {
+              body.try_emplace("reason", "removed");
+            } else if (event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded) {
+              body.try_emplace("reason", "changed");
+            }
+            body.try_emplace("module", module_value);
+            module_event.try_emplace("body", std::move(body));
+            g_vsc.SendJSON(llvm::json::Value(std::move(module_event)));
+          }
+        }
       } else if (event.BroadcasterMatchesRef(g_vsc.broadcaster)) {
         if (event_mask & eBroadcastBitStopEventThread) {
           done = true;
         }
       }
     }
   }
 }