Add lldb-dap #317
Add lldb-dap #317
Conversation
Summary of ChangesHello @waruqi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the debugging capabilities of the XMake VSCode extension by integrating support for LLDB Debug Adapter Protocol and GDB Debug Adapter Protocol. It also includes a substantial refactoring of internal path resolution mechanisms, moving asset and resource path handling into a dedicated utility module to streamline development and ensure robust operation across different environments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for LLDB DAP and GDB DAP debuggers, expanding the available debugging options. The package.json was updated to include these new debugger types, and src/debugger.ts and src/launchDebugger.ts were modified to handle their specific configurations, including environment variable parsing. A new src/utils.ts file was added to centralize path resolution for assets, resources, and templates, and existing code across src/completion.ts, src/explorer.ts, src/launchDebugger.ts, and src/xmake.ts was refactored to utilize these new utility functions. Review comments identified a critical bug in src/xmake.ts where target_informations.lua was incorrectly used to list all targets, which needs to be reverted to a script that returns an array of target names. Additionally, the reviewer noted redundant assignments and hardcoded .exe extensions in the lldb-dap and gdb-dap debug configurations within src/debugger.ts, recommending the use of path.basename(targetInformations.path) for cross-platform compatibility. Refactoring of duplicated environment variable processing logic and more accurate type assertions were also suggested for src/debugger.ts. Finally, the src/utils.ts file was advised to refactor its path resolution logic into a single helper function to improve maintainability.
| let getTargetInformationsScript = utils.getAssetsScriptPath("target_informations.lua"); | ||
| if (fs.existsSync(getTargetInformationsScript)) { | ||
| targets = (await process.iorunv(config.executable, ["l", getTargetInformationsScript], { "COLORTERM": "nocolor" }, config.workingDirectory)).stdout.trim(); | ||
| } |
There was a problem hiding this comment.
The script target_informations.lua is being used here to get a list of all targets. However, when called without arguments, this script returns information for only the default target as a JSON object, not an array of all target names. This will cause a runtime error when targetlist.forEach is called on the resulting object.
You should use a script that returns a list of all target names. The previous implementation used assets/targets.lua, which seems correct. Please revert this to use a script that lists all targets.
| let getTargetInformationsScript = utils.getAssetsScriptPath("target_informations.lua"); | |
| if (fs.existsSync(getTargetInformationsScript)) { | |
| targets = (await process.iorunv(config.executable, ["l", getTargetInformationsScript], { "COLORTERM": "nocolor" }, config.workingDirectory)).stdout.trim(); | |
| } | |
| let getTargetsScript = utils.getAssetsScriptPath("targets.lua"); // Or another script that lists all target names | |
| if (fs.existsSync(getTargetsScript)) { | |
| targets = (await process.iorunv(config.executable, ["l", getTargetsScript], { "COLORTERM": "nocolor" }, config.workingDirectory)).stdout.trim(); | |
| } |
| // Switch to LLDB DAP if needed | ||
| if (config.debugConfigType == "lldb-dap") { | ||
| config.type = 'lldb-dap'; | ||
| config.stopOnEntry = config.stopOnEntry; | ||
| // LLDB DAP doesn't support newExternal | ||
| if (config.terminal == 'newExternal') { | ||
| config.terminal = 'external'; | ||
| } | ||
|
|
||
| // LLDB DAP use program key for search a running process | ||
| if (config.request == 'attach') { | ||
| config.stopOnEntry = false; | ||
| config.program = `${targetInformations.name}.exe`; | ||
| } | ||
| } |
There was a problem hiding this comment.
This block has a couple of issues:
- The assignment
config.stopOnEntry = config.stopOnEntry;is redundant and can be removed for clarity. - Hardcoding the
.exeextension for the program name in attach mode will fail on non-Windows platforms. You should usepath.basename(targetInformations.path)to get the platform-correct executable name.
I've suggested the changes for the lldb-dap block; please apply similar changes for the gdb-dap block.
// Switch to LLDB DAP if needed
if (config.debugConfigType == "lldb-dap") {
config.type = 'lldb-dap';
// LLDB DAP doesn't support newExternal
if (config.terminal == 'newExternal') {
config.terminal = 'external';
}
// LLDB DAP use program key for search a running process
if (config.request == 'attach') {
config.stopOnEntry = false;
config.program = path.basename(targetInformations.path);
}
}| // Switch to GDB DAP if needed | ||
| if (config.debugConfigType == "gdb-dap") { | ||
| config.type = 'gdb'; | ||
| config.stopOnEntry = config.stopOnEntry; | ||
| // GDB DAP doesn't support newExternal | ||
| if (config.terminal == 'newExternal') { | ||
| config.terminal = 'external'; | ||
| } | ||
|
|
||
| // GDB DAP use program key for search a running process | ||
| if (config.request == 'attach') { | ||
| config.stopOnEntry = false; | ||
| config.program = `${targetInformations.name}.exe`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the lldb-dap block, this block has a redundant assignment and hardcodes the .exe extension.
- The assignment
config.stopOnEntry = config.stopOnEntry;is redundant. - Hardcoding
.exefor attach mode will fail on non-Windows platforms. Please usepath.basename(targetInformations.path)instead.
// Switch to GDB DAP if needed
if (config.debugConfigType == "gdb-dap") {
config.type = 'gdb';
// GDB DAP doesn't support newExternal
if (config.terminal == 'newExternal') {
config.terminal = 'external';
}
// GDB DAP use program key for search a running process
if (config.request == 'attach') {
config.stopOnEntry = false;
config.program = path.basename(targetInformations.path);
}
}| } else if (lldbdap) { | ||
| // LLDB DAP configuration | ||
| var envs = {}; | ||
| for (let item of (targetRunEnvs as Array<Object>)) { | ||
| let map = item as Map<String, String>; | ||
| if (map) { | ||
| let name = map["name"]; | ||
| let value = map["value"]; | ||
| if (name && value) { | ||
| envs[name] = value; | ||
| } | ||
| } | ||
| } | ||
| debugConfig = { | ||
| name: `launch: ${targetName}`, | ||
| type: 'lldb-dap', | ||
| request: 'launch', | ||
| program: targetProgram, | ||
| args: args, | ||
| stopAtEntry: true, | ||
| cwd: targetRunDir, | ||
| env: envs, | ||
| externalConsole: false, | ||
| }; | ||
| } else if (gdbdap) { | ||
| // GDB DAP configuration | ||
| var gdbEnvs = []; | ||
| for (let item of (targetRunEnvs as Array<Object>)) { | ||
| let map = item as Map<String, String>; | ||
| if (map) { | ||
| let name = map["name"]; | ||
| let value = map["value"]; | ||
| if (name && value) { | ||
| gdbEnvs.push({name: name, value: value}); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication in how environment variables are processed for codelldb, lldbdap, and gdbdap. The loops are very similar and can be refactored into a helper function to improve maintainability and reduce redundancy.
Additionally, the type assertion item as Map<String, String> is not accurate. The targetRunEnvs is an array of objects like { name: string, value: string }. Using item as { name: string, value: string } would be more precise. The use of String (capital S) is also unconventional for type annotations in TypeScript; string is preferred.
Consider creating a utility function to handle environment variable processing for different debugger formats.
| // get assets script path | ||
| export function getAssetsScriptPath(scriptName: string): string { | ||
| // In development mode (running from source), __dirname points to src/ | ||
| // In compiled extension (running from out), __dirname points to out/ | ||
| // So we need to handle both cases | ||
| if (__dirname.endsWith('src')) { | ||
| // Development mode: go up two levels from src/ | ||
| return path.resolve(__dirname, "..", "..", "assets", scriptName); | ||
| } else { | ||
| // Compiled mode: go up one level from out/ | ||
| return path.resolve(__dirname, "..", "assets", scriptName); | ||
| } | ||
| } | ||
|
|
||
| // get res directory path | ||
| export function getResourcePath(resourceName: string = ""): string { | ||
| // In development mode (running from source), __dirname points to src/ | ||
| // In compiled extension (running from out), __dirname points to out/ | ||
| // So we need to handle both cases | ||
| if (__dirname.endsWith('src')) { | ||
| // Development mode: go up two levels from src/ | ||
| return path.resolve(__dirname, "..", "..", "res", resourceName); | ||
| } else { | ||
| // Compiled mode: go up one level from out/ | ||
| return path.resolve(__dirname, "..", "res", resourceName); | ||
| } | ||
| } | ||
|
|
||
| // get template directory path | ||
| export function getTemplatePath(templateName: string = ""): string { | ||
| // In development mode (running from source), __dirname points to src/ | ||
| // In compiled extension (running from out), __dirname points to out/ | ||
| // So we need to handle both cases | ||
| if (__dirname.endsWith('src')) { | ||
| // Development mode: go up two levels from src/ | ||
| return path.resolve(__dirname, "..", "..", "assets", "newfiles", templateName); | ||
| } else { | ||
| // Compiled mode: go up one level from out/ | ||
| return path.resolve(__dirname, "..", "assets", "newfiles", templateName); | ||
| } | ||
| } |
There was a problem hiding this comment.
The functions getAssetsScriptPath, getResourcePath, and getTemplatePath contain duplicated logic for determining the base path based on whether the extension is running in development or compiled mode. This can be refactored into a single private helper function to improve maintainability. For example:
function getBasePath(): string {
if (__dirname.endsWith('src')) {
return path.resolve(__dirname, "..", "..");
} else {
return path.resolve(__dirname, "..");
}
}
export function getAssetsScriptPath(scriptName: string): string {
return path.join(getBasePath(), "assets", scriptName);
}
// ... and so on for other functions
Uh oh!
There was an error while loading. Please reload this page.