Repository navigation
Conversation
milishar
commented
Oct 7, 2026
- Detect ARM64/ARM64EC platforms in CMake via CMAKE_GENERATOR_PLATFORM.
- cliloader: compare PE header machine types instead of IsWow64Process, which cannot distinguish ARM64EC from ARM64. Warn and continue if a type is unknown.
- cliloader: while loading the intercept DLL into the loader process, disable the controls that create log, trace, and report files, then restore them so the target process does not inherit the overrides.
- Skip the report at shutdown if no enqueues occurred.
- Patch the IAT of every loaded module so DLLs that import opencl.dll indirectly (e.g. SYCL runtimes) are also intercepted. Links Psapi.
- Add Utils::OpenOutputFile() and use it for all dump files. Together with CreateDirectoryW for dump directories, this uses the ?\ prefix on Windows to support paths longer than MAX_PATH.
…hing - Detect ARM64/ARM64EC platforms in CMake via CMAKE_GENERATOR_PLATFORM. - cliloader: compare PE header machine types instead of IsWow64Process, which cannot distinguish ARM64EC from ARM64. Warn and continue if a type is unknown. - cliloader: while loading the intercept DLL into the loader process, disable the controls that create log, trace, and report files, then restore them so the target process does not inherit the overrides. - Skip the report at shutdown if no enqueues occurred. - Patch the IAT of every loaded module so DLLs that import opencl.dll indirectly (e.g. SYCL runtimes) are also intercepted. Links Psapi. - Add Utils::OpenOutputFile() and use it for all dump files. Together with CreateDirectoryW for dump directories, this uses the \?\ prefix on Windows to support paths longer than MAX_PATH.
bashbaug
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This mostly looks good, but I would like to see one change before merging.
If you're up to it, could you also add this build config to CI? If not, no problem, I'll try to do this in a separate PR (probably after merging this one).
| if( m_EnqueueCounter.load(std::memory_order_relaxed) > 0 ) | ||
| { | ||
| report(); |
There was a problem hiding this comment.
I understand the motivation behind this change, but I think it is a bit too heavy-handed, since there are still reasons why you might want report data even for applications that do not enqueue any work. For example, host performance timing might still be relevant (to see which APIs were called, if nothing else), and leak checking could still be valuable. So, I don't think we should omit the report by default based on this condition.
Some other options to consider:
- Add a new control like
SuppressReport, analogous to the existingSuppressLogging, which allows an application to omit the report in this condition when desired. Note, I would be fine tying this to the existing cliloader-qoption. - Be a bit more selective as to when the report data is omitted - for example, if there is no host performance timing data, device performance timing data, leak checking, etc., so the report file would otherwise be empty, omit the report.
- Add a counter for the number of calls, not just the number of enqueues. If the number of calls is zero, we should be able to safely omit the report. This option sounds a bit scary and intrusive, but I think we can increment the counter from one of the existing per-function macros (e.g. getting or incrementing the enqueue counter, or starting or ending host performance timing) so it shouldn't be too bad.
Let me know what you think, or if there is another option we should consider.
There was a problem hiding this comment.
If this is going to be contentious or require a lot of work, I'd also support removing this change from this PR for now, and addressing it in a subsequent PR.
Don't bother with this - I have a draft PR that adds this in #492. I'll rebase it after this PR gets merged. Thanks! |
| // Use the \\?\ prefix with the wide-character API to support paths | ||
| // longer than MAX_PATH (260 characters). The \\?\ prefix requires | ||
| // backslashes, so convert any forward slashes in the path. | ||
| std::string dirPath = "\\\\?\\" + fileName.substr( 0, pos ); |
There was a problem hiding this comment.
I think that the \\?\ prefix doesn't work with relative paths or with network paths (\\server\share\folder). Can you please check these cases? We may want to go down this \\?\ codepath selectively, only when path names are too long.
| MultiByteToWideChar( | ||
| CP_UTF8, 0, dirPath.c_str(), -1, &wDirPath[0], wLen ); | ||
| CreateDirectoryW( wDirPath.c_str(), NULL ); |
There was a problem hiding this comment.
I think there's a potential issue here, at least in theory, with the current implementation: The "dump directories" are created by converting the path to UTF-8, but there are still places that open the file with standard C++ APIs. Some possible solutions:
- Switch every place that opens a file to use the new utility function, then we will use UTF-8 consistently.
- Convert to the "ANSI code page" (
CP_ACP) instead, which is consistent with the standard C++ APIs.
(1) is probably the better solution, but it's more code to change than (2).
| // This is necessary for applications that use OpenCL indirectly through | ||
| // another DLL (e.g., SYCL runtime DLLs like sycl7.dll) rather than | ||
| // importing opencl.dll directly in the main executable. | ||
| HMODULE hModules[1024] = {}; |
There was a problem hiding this comment.
Can we allocate this dynamically (say as an std::vector)? Or, at the very least, handle the case when there are more than 1024 modules? I guess since EnumProcessModules takes the size of the array we won't overwrite memory, but we would silently leave some modules un-patched, which could be very difficult to debug.
| std::wstring wFileName( wLen, 0 ); | ||
| MultiByteToWideChar( | ||
| CP_UTF8, 0, longPath.c_str(), -1, &wFileName[0], wLen ); | ||
| os.open( wFileName.c_str(), mode ); |
There was a problem hiding this comment.
I think that this function (ofstream::open(const wchar_t*)) is an MVSC extension. Is there a portable equivalent? If not, we should guard this with an MVSC define, not a Windows define.
Note, we may want to move this function to the OS wrappers also, both since it's currently very OS-specific, and because it is important to keep it aligned with the function to create dump directories.
| if(CMAKE_SIZEOF_VOID_P EQUAL 8) | ||
| if(CMAKE_GENERATOR_PLATFORM STREQUAL "ARM64") | ||
| set(CLINTERCEPT_PLATFORM_NAME "ARM64") | ||
| elseif(CMAKE_GENERATOR_PLATFORM STREQUAL "ARM64EC") | ||
| set(CLINTERCEPT_PLATFORM_NAME "ARM64EC") | ||
| elseif(CMAKE_SIZEOF_VOID_P EQUAL 8) |
There was a problem hiding this comment.
This is an OK change, and getting this "wrong" isn't a huge problem (everything still builds correctly, it just gets installed into a mis-named directory), but it's also a touch fragile since CMAKE_GENERATOR_PLATFORM is only set for Visual Studio builds and only when the -A compiler option is passed.
If we want to make this more robust, one option is to check CMAKE_CXX_COMPILER_ARCHITECTURE_ID instead:
string(TOUPPER "${CMAKE_CXX_COMPILER_ARCHITECTURE_ID}" CLINTERCEPT_ARCH_ID)
if(CLINTERCEPT_ARCH_ID STREQUAL "ARM64EC")
set(CLINTERCEPT_PLATFORM_NAME "ARM64EC")
elseif(CLINTERCEPT_ARCH_ID STREQUAL "ARM64")
set(CLINTERCEPT_PLATFORM_NAME "ARM64")
elseif(CMAKE_SIZEOF_VOID_P EQUAL 8)
...Longer-term, I'd like to remove this Windows-specific handling, and just install into an "install" directory by default, similar to the other OSes.
| std::ofstream output(dumpDirectory + "kernel.cl", std::ios::out | std::ios::binary); | ||
| std::ofstream output; | ||
| Utils::OpenOutputFile(output, dumpDirectory + "kernel.cl", std::ios::out | std::ios::binary); |
There was a problem hiding this comment.
Leaving a comment here, but this is really a general comment: This PR is calling this OpenOutputFile utility function for the capture-replay functionality, but there are still a number of other dump files that call the standard C++ functions directly. We should really switch all of them...