Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Complete the Implementation of DAP modules explorer. #139934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 15, 2025

Conversation

da-viper
Copy link
Contributor

What it now looks like.

It is a good idea to remove the, symbol status and name from the tree children ?.

image

@llvmbot
Copy link
Member

llvmbot commented May 14, 2025

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

What it now looks like.

It is a good idea to remove the, symbol status and name from the tree children ?.

image


Full diff: https://github.com/llvm/llvm-project/pull/139934.diff

4 Files Affected:

  • (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+5-2)
  • (modified) lldb/tools/lldb-dap/package.json (+14)
  • (modified) lldb/tools/lldb-dap/src-ts/extension.ts (+9-1)
  • (modified) lldb/tools/lldb-dap/src-ts/ui/modules-data-provider.ts (+62-36)
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 279e6d3d93814..31eb91f9f6b61 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -416,8 +416,11 @@ llvm::json::Value CreateModule(lldb::SBTarget &target, lldb::SBModule &module,
   } else {
     object.try_emplace("symbolStatus", "Symbols not found.");
   }
-  std::string loaded_addr = std::to_string(
-      module.GetObjectFileHeaderAddress().GetLoadAddress(target));
+  std::string loaded_addr;
+  llvm::raw_string_ostream os_hex(loaded_addr);
+  os_hex << llvm::format_hex(
+      module.GetObjectFileHeaderAddress().GetLoadAddress(target),
+      sizeof(lldb::addr_t));
   object.try_emplace("addressRange", loaded_addr);
   std::string version_str;
   uint32_t version_nums[3];
diff --git a/lldb/tools/lldb-dap/package.json b/lldb/tools/lldb-dap/package.json
index e3e46526f379f..3c73534fd3180 100644
--- a/lldb/tools/lldb-dap/package.json
+++ b/lldb/tools/lldb-dap/package.json
@@ -244,6 +244,20 @@
         }
       }
     ],
+    "commands": [
+      {
+        "command": "lldb-dap.modules.copyProperty",
+        "title": "Copy Value"
+      }
+    ],
+    "menus": {
+      "view/item/context": [
+        {
+          "command": "lldb-dap.modules.copyProperty",
+          "when": "view == lldb-dap.modules && viewItem == property"
+        }
+      ]
+    },
     "breakpoints": [
       {
         "language": "ada"
diff --git a/lldb/tools/lldb-dap/src-ts/extension.ts b/lldb/tools/lldb-dap/src-ts/extension.ts
index a5c0a09ae60cf..c8e5146e29cea 100644
--- a/lldb/tools/lldb-dap/src-ts/extension.ts
+++ b/lldb/tools/lldb-dap/src-ts/extension.ts
@@ -6,7 +6,10 @@ import { LaunchUriHandler } from "./uri-launch-handler";
 import { LLDBDapConfigurationProvider } from "./debug-configuration-provider";
 import { LLDBDapServer } from "./lldb-dap-server";
 import { DebugSessionTracker } from "./debug-session-tracker";
-import { ModulesDataProvider } from "./ui/modules-data-provider";
+import {
+  ModulesDataProvider,
+  ModuleProperty,
+} from "./ui/modules-data-provider";
 
 /**
  * This class represents the extension and manages its life cycle. Other extensions
@@ -40,6 +43,11 @@ export class LLDBDapExtension extends DisposableContext {
       ),
       vscode.window.registerUriHandler(new LaunchUriHandler()),
     );
+
+    vscode.commands.registerCommand(
+      "lldb-dap.modules.copyProperty",
+      (node: ModuleProperty) => vscode.env.clipboard.writeText(node.value),
+    );
   }
 }
 
diff --git a/lldb/tools/lldb-dap/src-ts/ui/modules-data-provider.ts b/lldb/tools/lldb-dap/src-ts/ui/modules-data-provider.ts
index 478c162de8878..091c1d69ac647 100644
--- a/lldb/tools/lldb-dap/src-ts/ui/modules-data-provider.ts
+++ b/lldb/tools/lldb-dap/src-ts/ui/modules-data-provider.ts
@@ -2,60 +2,86 @@ import * as vscode from "vscode";
 import { DebugProtocol } from "@vscode/debugprotocol";
 import { DebugSessionTracker } from "../debug-session-tracker";
 
-/** A tree data provider for listing loaded modules for the active debug session. */
-export class ModulesDataProvider
-  implements vscode.TreeDataProvider<DebugProtocol.Module>
-{
-  private changeTreeData = new vscode.EventEmitter<void>();
-  readonly onDidChangeTreeData = this.changeTreeData.event;
+export interface ModuleProperty {
+  key: string;
+  value: string;
+}
 
-  constructor(private readonly tracker: DebugSessionTracker) {
-    tracker.onDidChangeModules(() => this.changeTreeData.fire());
-    vscode.debug.onDidChangeActiveDebugSession(() =>
-      this.changeTreeData.fire(),
-    );
+/** Type to represent both Module and ModuleProperty since TreeDataProvider
+ * expects one concrete type */
+type TreeData = DebugProtocol.Module | ModuleProperty;
+
+function isModule(type: TreeData): type is DebugProtocol.Module {
+  return (type as DebugProtocol.Module).id !== undefined;
+}
+
+class ModuleItem extends vscode.TreeItem {
+  constructor(module: DebugProtocol.Module) {
+    super(module.name, vscode.TreeItemCollapsibleState.Collapsed);
+    this.description = module.symbolStatus;
   }
 
-  getTreeItem(module: DebugProtocol.Module): vscode.TreeItem {
-    let treeItem = new vscode.TreeItem(/*label=*/ module.name);
-    if (module.path) {
-      treeItem.description = `${module.id} -- ${module.path}`;
-    } else {
-      treeItem.description = `${module.id}`;
-    }
+  static getProperties(module: DebugProtocol.Module): ModuleProperty[] {
+    // does not include the name and symbol status as it is show in the parent.
+    let children: ModuleProperty[] = [];
+    children.push({ key: "id:", value: module.id.toString() });
 
-    const tooltip = new vscode.MarkdownString();
-    tooltip.appendMarkdown(`# ${module.name}\n\n`);
-    tooltip.appendMarkdown(`- **ID**: ${module.id}\n`);
     if (module.addressRange) {
-      tooltip.appendMarkdown(
-        `- **Load address**: 0x${Number(module.addressRange).toString(16)}\n`,
-      );
+      children.push({
+        key: "load address:",
+        value: module.addressRange,
+      });
     }
     if (module.path) {
-      tooltip.appendMarkdown(`- **Path**: ${module.path}\n`);
+      children.push({ key: "path:", value: module.path });
     }
     if (module.version) {
-      tooltip.appendMarkdown(`- **Version**: ${module.version}\n`);
-    }
-    if (module.symbolStatus) {
-      tooltip.appendMarkdown(`- **Symbol status**: ${module.symbolStatus}\n`);
+      children.push({ key: "version:", value: module.version });
     }
     if (module.symbolFilePath) {
-      tooltip.appendMarkdown(
-        `- **Symbol file path**: ${module.symbolFilePath}\n`,
-      );
+      children.push({ key: "symbol filepath:", value: module.symbolFilePath });
+    }
+    return children;
+  }
+}
+
+/** A tree data provider for listing loaded modules for the active debug session. */
+export class ModulesDataProvider implements vscode.TreeDataProvider<TreeData> {
+  private changeTreeData = new vscode.EventEmitter<void>();
+  readonly onDidChangeTreeData = this.changeTreeData.event;
+
+  constructor(private readonly tracker: DebugSessionTracker) {
+    tracker.onDidChangeModules(() => this.changeTreeData.fire());
+    vscode.debug.onDidChangeActiveDebugSession(() =>
+      this.changeTreeData.fire(),
+    );
+  }
+
+  getTreeItem(module: TreeData): vscode.TreeItem {
+    if (isModule(module)) {
+      return new ModuleItem(module);
     }
 
-    treeItem.tooltip = tooltip;
-    return treeItem;
+    let item = new vscode.TreeItem(module.key);
+    item.description = module.value;
+    item.tooltip = `${module.key} ${module.value}`;
+    item.contextValue = "property";
+    return item;
   }
 
-  getChildren(): DebugProtocol.Module[] {
+  getChildren(element?: TreeData): TreeData[] {
     if (!vscode.debug.activeDebugSession) {
       return [];
     }
 
-    return this.tracker.debugSessionModules(vscode.debug.activeDebugSession);
+    if (!element) {
+      return this.tracker.debugSessionModules(vscode.debug.activeDebugSession);
+    }
+
+    if (isModule(element)) {
+      return ModuleItem.getProperties(element);
+    }
+
+    return [];
   }
 }

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment on lines 419 to 423
std::string loaded_addr;
llvm::raw_string_ostream os_hex(loaded_addr);
os_hex << llvm::format_hex(
module.GetObjectFileHeaderAddress().GetLoadAddress(target),
sizeof(lldb::addr_t));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed the load address was a "number" but the spec says it's a "string" in which case I agree it makes more sense to the hex conversion here.

You can simplify this something like this:

object.try_emplace("addressRange", llvm::format({0:x}, module.GetObjectFileHeaderAddress().GetLoadAddress(target)).str());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

microsoft/debug-adapter-protocol#424 was filed to clarify how the address range is supposed to be represented. It hasn't had any movement recently, so we could ping the issue to see if the folks that work on the DAP have any more recent thoughts.

@da-viper da-viper requested a review from ashgti May 14, 2025 18:33
Comment on lines 253 to 254
"menus": {
"view/item/context": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also disable the Copy Value command from the command prompt?

adding:

"commandPalette": [{"command": "lldb-dap.modules.copyProperty", "when": "false"}],

Should hide it, since its not useful without the context of a tree view item.

@da-viper da-viper merged commit 42ee758 into llvm:main May 15, 2025
10 checks passed
@da-viper da-viper deleted the complete-dap-modules branch May 15, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.