Skip to content
Open
21 changes: 10 additions & 11 deletions Extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4770,15 +4770,8 @@
"attach": {
"type": "object",
"default": {},
"required": [
"program"
],
"required": [],
"properties": {
"program": {
"type": "string",
"description": "%c_cpp.debuggers.program.description%",
"default": "${workspaceRoot}/a.out"
},
"targetArchitecture": {
"type": "string",
"description": "%c_cpp.debuggers.targetArchitecture.description%",
Expand Down Expand Up @@ -4824,6 +4817,10 @@
"description": "%c_cpp.debuggers.useExtendedRemote.description%",
"default": false
},
"program": {
"type": "string",
"markdownDescription": "%c_cpp.debuggers.program.attach.markdownDescription%"
},
"processId": {
"markdownDescription": "%c_cpp.debuggers.processId.anyOf.markdownDescription%",
"anyOf": [
Expand Down Expand Up @@ -5789,15 +5786,17 @@
"attach": {
"type": "object",
"default": {},
"required": [
"processId"
],
"required": [],
"properties": {
"symbolSearchPath": {
"type": "string",
"description": "%c_cpp.debuggers.symbolSearchPath.description%",
"default": ""
},
"program": {
"type": "string",
"markdownDescription": "%c_cpp.debuggers.program.attach.markdownDescription%"
},
"processId": {
"markdownDescription": "%c_cpp.debuggers.processId.anyOf.markdownDescription%",
"anyOf": [
Expand Down
8 changes: 7 additions & 1 deletion Extension/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,13 @@
"{Locked=\"`${command:pickProcess}`\"}"
]
},
"c_cpp.debuggers.symbolSearchPath.description": "Semicolon separated list of directories to use to search for symbol (that is, pdb) files. Example: \"c:\\dir1;c:\\dir2\".",
"c_cpp.debuggers.program.attach.markdownDescription": {
"message": "Optional full path to program executable. When specified, the debugger will search for a running process matching this executable name and attach to it. If multiple processes match, a selection prompt will be shown. Use either `program` or `processId`, not both.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should executable name be executable path?

Copy link
Contributor

Choose a reason for hiding this comment

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

path to program executable should be path to the program executable.

"comment": [
"{Locked=\"`program`\"} {Locked=\"`processId`\"}"
]
},
"c_cpp.debuggers.symbolSearchPath.description": "Semicolon separated list of directories to use to search for .so files. Example: \"c:\\dir1;c:\\dir2\".",
Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows with MSVC it uses .pdb files and not so files, so the description could be something like search for symbols (that is, pdb or .so files).

"c_cpp.debuggers.dumpPath.description": "Optional full path to a dump file for the specified program. Example: \"c:\\temp\\app.dmp\". Defaults to null.",
"c_cpp.debuggers.enableDebugHeap.description": "If false, the process will be launched with debug heap disabled. This sets the environment variable '_NO_DEBUG_HEAP' to '1'.",
"c_cpp.debuggers.symbolLoadInfo.description": "Explicit control of symbol loading.",
Expand Down
68 changes: 62 additions & 6 deletions Extension/src/Debugger/configurationProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { PlatformInformation } from '../platform';
import { rsync, scp, ssh } from '../SSH/commands';
import * as Telemetry from '../telemetry';
import { AttachItemsProvider, AttachPicker, RemoteAttachPicker } from './attachToProcess';
import { AttachItem, showQuickPick } from './attachQuickPick';
import { ConfigMenu, ConfigMode, ConfigSource, CppDebugConfiguration, DebuggerEvent, DebuggerType, DebugType, IConfiguration, IConfigurationSnippet, isDebugLaunchStr, MIConfigurations, PipeTransportConfigurations, TaskStatus, WindowsConfigurations, WSLConfigurations } from './configurations';
import { NativeAttachItemsProviderFactory } from './nativeAttach';
import { Environment, ParsedEnvironmentFile } from './ParsedEnvironmentFile';
Expand Down Expand Up @@ -354,13 +355,24 @@ export class DebugConfigurationProvider implements vscode.DebugConfigurationProv
// Pick process if process id is empty
if (config.request === "attach" && !config.processId) {
let processId: string | undefined;
if (config.pipeTransport || config.useExtendedRemote) {
const remoteAttachPicker: RemoteAttachPicker = new RemoteAttachPicker();
processId = await remoteAttachPicker.ShowAttachEntries(config);

// If program is specified, try to find matching process by name
Copy link
Contributor

Choose a reason for hiding this comment

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

matching process by name->the matching process by name.

if (config.program) {
processId = await this.findProcessByProgramName(config.program, config, token);
if (!processId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this would cause a failure for users who currently have processName set to an invalid value, which is the default.

Can you instead fall back to the process picker if findProcessByProgramName doesn't return a valid processId and don't print any error message?

void logger.getOutputChannelLogger().showErrorMessage(`No running process found matching "${config.program}".`);
Copy link
Contributor

Choose a reason for hiding this comment

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

These user facing strings should be passed to the localize function with some unique key, see the example such as void logger.getOutputChannelLogger().showErrorMessage(localize('missing.properties.copyFile', '"host", "files", and "targetDir" are required in {0} steps.', isScp ? 'SCP' : 'rsync'));

I saw a spot in this file where pre-existing code appears to be missing a localize call (we can fix that ourselves later).

return undefined;
}
} else {
const attachItemsProvider: AttachItemsProvider = NativeAttachItemsProviderFactory.Get();
const attacher: AttachPicker = new AttachPicker(attachItemsProvider);
processId = await attacher.ShowAttachEntries(token);
// Show process picker if no program specified
Copy link
Contributor

Choose a reason for hiding this comment

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

Show process picker if no program specified->Show the process picker if no program is specified.

if (config.pipeTransport || config.useExtendedRemote) {
const remoteAttachPicker: RemoteAttachPicker = new RemoteAttachPicker();
processId = await remoteAttachPicker.ShowAttachEntries(config);
} else {
const attachItemsProvider: AttachItemsProvider = NativeAttachItemsProviderFactory.Get();
const attacher: AttachPicker = new AttachPicker(attachItemsProvider);
processId = await attacher.ShowAttachEntries(token);
}
}

if (processId) {
Expand Down Expand Up @@ -1157,6 +1169,50 @@ export class DebugConfigurationProvider implements vscode.DebugConfigurationProv
}
return true;
}

private async findProcessByProgramName(
programPath: string,
config: CppDebugConfiguration,
token?: vscode.CancellationToken
): Promise<string | undefined> {
const programBaseName: string = path.basename(programPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you first check if the programPath is a valid path? Otherwise, invalid paths like "this is an invalid path.exe" will use "path.exe" as the programBaseName.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call checkExecutableWithoutExtensionExists.

let processes: AttachItem[];

// Get process list using the same logic as interactive attach
Copy link
Contributor

Choose a reason for hiding this comment

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

Get process list using the same logic as interactive attach->Get the process list using the same logic as interactive attach.

if (config.pipeTransport || config.useExtendedRemote) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this check to the very beginning before "program" is checked and don't show any error? Otherwise, those users would start to get an error.

// For remote attach, we need to use RemoteAttachPicker's methods
void logger.getOutputChannelLogger().showErrorMessage(
"Finding process by program name is not yet supported for remote attach. Please use processId instead."
);
return undefined;
} else {
const attachItemsProvider: AttachItemsProvider = NativeAttachItemsProviderFactory.Get();
processes = await attachItemsProvider.getAttachItems(token);
}

// Find processes matching the program name
const matchingProcesses: AttachItem[] = processes.filter(p => {
const processName: string = p.label.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

The toLowerCase should only be used on Windows where the paths are case insensitive.

const targetName: string = programBaseName.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved out of the lambda and into a local variable at the out scope to avoid unnecessarily calling it more than once.

Copy link
Contributor

Choose a reason for hiding this comment

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

And you could use something like

                    if (isWindows) {
                        targetName = targetName.endsWith(".exe") ? targetName : (targetName + ".exe");
                    }

// Match if the process name exactly matches or starts with the target name
return processName === targetName || processName.startsWith(targetName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this startsWith to prevent the an unintended process from being accidentally attached to?

});

if (matchingProcesses.length === 0) {
return undefined;
} else if (matchingProcesses.length === 1) {
void logger.getOutputChannelLogger().appendLine(
`Found process "${matchingProcesses[0].label}" with PID ${matchingProcesses[0].id}`
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to log here.

);
return matchingProcesses[0].id;
} else {
// Multiple matches - let user choose
Copy link
Contributor

Choose a reason for hiding this comment

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

let user choose->let the user choose.

void logger.getOutputChannelLogger().appendLine(
Copy link
Contributor

Choose a reason for hiding this comment

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

This logging is unnecessary -- users will see the quick pick UI and not see the logging.

`Multiple processes found matching "${programBaseName}". Please select one:`
);
return showQuickPick(() => Promise.resolve(matchingProcesses));
}
}
}

export interface IConfigurationAssetProvider {
Expand Down
Loading