✨ อัปเดตล่าสุด: eclipse omr/omr/pull/

sharanpatil123 · 2025-07-15T11:06:48Z

Use EnumProcesses and QueryFullProcessImageName to extract PID and executable paths on Windows.

sharanpatil123 · 2025-07-15T13:05:22Z

@keithc-ca - Please review

keithc-ca · 2025-07-15T14:46:00Z

+/*
+ * Fallback: Use Toolhelp32Snapshot to retrieve process name for restricted/system processes.
+ */
+static const char *
+getProcessNameFallback(DWORD pid, char *buffer, size_t bufferSize)
+{
+	HANDLE hSnap = NULL;
+	PROCESSENTRY32 pe;
+	pe.dwSize = sizeof(PROCESSENTRY32);
+	hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+	if (hSnap != INVALID_HANDLE_VALUE) {
+		if (Process32First(hSnap, &pe)) {
+			do {
+				if (pe.th32ProcessID == pid) {
+					strncpy(buffer, pe.szExeFile, bufferSize - 1);
+					buffer[bufferSize - 1] = '\0';
+					break;
+				}
+			} while (Process32Next(hSnap, &pe));
+		}
+		CloseHandle(hSnap);
+	}
+	return buffer;
+}


It's not clear to me that use of CreateToolhelp32Snapshot() is reasonable because a 32-bit process cannot retrieve information for a 64-bit processes.

@keithc-ca - There are some system/protected processes - such as csrss, System, wininit, Registry Memory Compression etc. Attempting to access them with OpenProcess fails due to access denial and these do not have an executable path. In hotspot these processes are also listed as part of this event, hence had to use the fallback function CreateToolhelp32Snapshot to get the process name. I’ve tested this and is able to list these processes.

If these processes are not needed, I can remove the fallback.

keithc-ca · 2025-07-15T14:46:53Z

+	DWORD numProcesses = 0;
+	DWORD bufferSize = 1024;
+	DWORD i = 0;
+	HANDLE hProcess = NULL;


Please move this declaration to line 2156 - it's only used within that scope.

keithc-ca · 2025-07-15T14:48:52Z

+	}
+	processes = (DWORD *)portLibrary->mem_allocate_memory(
+				portLibrary,
+				bufferSize * sizeof(DWORD),


Please just use bufferSize throughout instead of bufferSize * sizeof(DWORD). If reasonable, you could change the initial value:

DWORD bufferSize = 1024 * sizeof(DWORD);

keithc-ca · 2025-07-15T14:49:20Z

+				return (uintptr_t)(intptr_t)OMRPORT_ERROR_OPFAILED;
+	}
+	processes = (DWORD *)portLibrary->mem_allocate_memory(
+				portLibrary,


Excessive indentation (one tab too many).

keithc-ca · 2025-07-15T14:49:40Z

+					portLibrary,
+					processes,
+					bufferSize * sizeof(DWORD),
+					OMR_GET_CALLSITE(),
+					OMRMEM_CATEGORY_PORT_LIBRARY);


Indentation.

keithc-ca · 2025-07-15T14:50:35Z

+	}
+	numProcesses = bytesReturned / sizeof(DWORD);
+	for (i = 0; i < numProcesses; i++) {
+		char exePath[MAX_PATH] = {0};


Please don't initialize an array like this; instead after line 2156:

exePath[0] = '\0';

keithc-ca · 2025-07-15T14:51:13Z

+		DWORD pid = processes[i];
+		hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_QUERY_INFORMATION, FALSE, pid);
+		if (NULL != hProcess) {
+			if (0 == QueryFullProcessImageNameA(hProcess, 0, exePath, &pathLen)) {


Please use QueryFullProcessImageName() instead of QueryFullProcessImageNameA().

keithc-ca · 2025-07-15T14:53:25Z

+			getProcessNameFallback(pid, exePath, MAX_PATH);
+		}
+		/* Skip entries with no name. */
+		if (exePath[0] == '\0') {


Constant on left of ==:

if ('\0' == exePath[0]) {

keithc-ca · 2025-07-15T14:54:52Z

+	numProcesses = bytesReturned / sizeof(DWORD);
+	for (i = 0; i < numProcesses; i++) {
+		char exePath[MAX_PATH] = {0};
+		DWORD pathLen = MAX_PATH;


The only use of MAX_PATH should be on line 2153; other places should use sizeof(exePath).

sharanpatil123 · 2025-07-16T07:00:19Z

@keithc-ca - Addressed review comments.

keithc-ca · 2025-07-16T20:43:13Z

 }

+/*
+ * Fallback: Use Toolhelp32Snapshot to retrieve process name for restricted/system processes.


I don't think we want to use this function at all: the documentation for CreateToolhelp32Snapshot() says 32-bit processes cannot retrieve information for a 64-bit process. If we do need to keep it, the comment should refer to the actual system call used.

Ok, I have removed the fallback.

keithc-ca · 2025-07-16T20:43:49Z

+	PROCESSENTRY32 pe;
+	pe.dwSize = sizeof(PROCESSENTRY32);
+	hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+	if (hSnap != INVALID_HANDLE_VALUE) {


Constant on the left of !=, please:

if (INVALID_HANDLE_VALUE != hSnap) {

keithc-ca · 2025-07-16T20:44:16Z

+{
+	HANDLE hSnap = NULL;
+	PROCESSENTRY32 pe;
+	pe.dwSize = sizeof(PROCESSENTRY32);


The rest of pe should be cleared:

memset(&pe, 0, sizeof(pe)); pe.dwSize = sizeof(pe);

keithc-ca · 2025-07-16T20:45:00Z

+		}
+		CloseHandle(hSnap);
+	}
+	return buffer;


The return value is not actually used; suggest this should have void return type (if the function stays).

keithc-ca · 2025-07-16T20:46:06Z

+				portLibrary,
+				OMRPORT_ERROR_OPFAILED,
+				"Callback function is NULL.");
+				return (uintptr_t)(intptr_t)OMRPORT_ERROR_OPFAILED;


Line 2116 should be indented the same as line 2112.

keithc-ca · 2025-07-16T20:46:42Z

+		HANDLE hProcess = NULL;
+		hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_QUERY_INFORMATION, FALSE, pid);


Please join these lines:

HANDLE hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_QUERY_INFORMATION, FALSE, pid);

sharanpatil123 · 2025-07-18T13:36:40Z

@keithc-ca - Addressed review comments.

keithc-ca · 2025-07-18T15:17:20Z

+		char exePath[MAX_PATH];
+		DWORD pathLen = sizeof(exePath);
+		DWORD pid = processes[i];
+		HANDLE hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_QUERY_INFORMATION, FALSE, pid);


This should request the minimum permissions necessary:

HANDLE hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, pid);

keithc-ca · 2025-07-18T15:20:28Z

+		if (NULL != hProcess) {
+			if (0 == QueryFullProcessImageName(hProcess, 0, exePath, &pathLen)) {
+				CloseHandle(hProcess);
+				continue;
+			}
+			CloseHandle(hProcess);
+		}
+		/* Skip entries with no name. */
+		if ('\0' == exePath[0]) {
+			continue;
+		}


The return type of QueryFullProcessImageName() is BOOL which should not be compared with 0. Suggest using pathLen to detect no name. Only one call to CloseHandle() is needed:

if (NULL == hProcess) { continue; } if (!QueryFullProcessImageName(hProcess, 0, exePath, &pathLen)) { pathLen = 0; } CloseHandle(hProcess); /* Skip entries with no name. */ if (0 == pathLen) { continue; }

keithc-ca

I think this should be squashed. The commit message should mention QueryFullProcessImageName instead of QueryFullProcessImageNameA and should spell "Windows" consistently.

Use EnumProcesses and QueryFullProcessImageName to extract PID and executable paths on Windows. Signed-off-by: Sharanabasava <sharan16@in.ibm.com>

babsingh · 2025-07-22T15:55:21Z

jenkins build all

babsingh

The Linux x86 and riscv64 failures are infra-related.

sharanpatil123 requested a review from babsingh as a code owner July 15, 2025 11:06

keithc-ca suggested changes Jul 15, 2025

View reviewed changes

sharanpatil123 requested a review from keithc-ca July 16, 2025 07:00

keithc-ca suggested changes Jul 16, 2025

View reviewed changes

sharanpatil123 requested a review from keithc-ca July 18, 2025 13:36

keithc-ca suggested changes Jul 18, 2025

View reviewed changes

keithc-ca reviewed Jul 21, 2025

View reviewed changes

Extend SystemProcess API Implementation to Windows

0e59648

Use EnumProcesses and QueryFullProcessImageName to extract PID and executable paths on Windows. Signed-off-by: Sharanabasava <sharan16@in.ibm.com>

sharanpatil123 force-pushed the systemprocess-windows branch from 15786fc to 0e59648 Compare July 21, 2025 13:54

sharanpatil123 requested a review from keithc-ca July 21, 2025 13:56

keithc-ca approved these changes Jul 21, 2025

View reviewed changes

babsingh approved these changes Jul 22, 2025

View reviewed changes

babsingh merged commit 4ea0790 into eclipse-omr:master Jul 22, 2025

sharanpatil123 mentioned this pull request Jul 25, 2025

SystemProcess event on Windows extracts only executable path due to lack of documented API for command-line arguments #7864

Open

pshipton mentioned this pull request Jul 28, 2025

cmdLineTester_jfrEvents_0 FAILED test jfr system process Success condition was not found eclipse-openj9/openj9#22312

Open

pshipton mentioned this pull request Oct 21, 2025

Update 0.56 release notes eclipse-openj9/openj9#22773

Merged

		HANDLE hProcess = NULL;
		hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION \| PROCESS_QUERY_INFORMATION, FALSE, pid);

Uh oh!

Conversation

sharanpatil123 commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharanpatil123 commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharanpatil123 commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharanpatil123 commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithc-ca left a comment

Choose a reason for hiding this comment

Uh oh!

babsingh commented Jul 22, 2025

Uh oh!

babsingh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharanpatil123 commented Jul 15, 2025 •

edited

Loading