-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
Conversation
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesWhat it now looks like. It is a good idea to remove the, symbol status and name from the tree children ?. Full diff: https://github.com/llvm/llvm-project/pull/139934.diff 4 Files Affected:
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 [];
}
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
lldb/tools/lldb-dap/JSONUtils.cpp
Outdated
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)); |
There was a problem hiding this comment.
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());
There was a problem hiding this comment.
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.
lldb/tools/lldb-dap/package.json
Outdated
"menus": { | ||
"view/item/context": [ |
There was a problem hiding this comment.
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.
What it now looks like.
It is a good idea to remove the, symbol status and name from the tree children ?.