Repository navigation
Windows: ARM64EC support, long paths, and indirect OpenCL import patching #491
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| #pragma once | ||
|
|
||
| #include <Windows.h> | ||
| #include <algorithm> | ||
| #include <string> | ||
| #include <stdint.h> | ||
|
|
||
|
|
@@ -306,9 +307,17 @@ inline void Services::MakeDumpDirectories( | |
| pos = fileName.find( "/", ++pos ); | ||
| while( pos != std::string::npos ) | ||
| { | ||
| CreateDirectoryA( | ||
| fileName.substr( 0, pos ).c_str(), | ||
| NULL ); | ||
| // 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 ); | ||
|
Comment on lines
+310
to
+313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the |
||
| std::replace( dirPath.begin(), dirPath.end(), '/', '\\' ); | ||
| int wLen = MultiByteToWideChar( | ||
| CP_UTF8, 0, dirPath.c_str(), -1, NULL, 0 ); | ||
| std::wstring wDirPath( wLen, 0 ); | ||
| MultiByteToWideChar( | ||
| CP_UTF8, 0, dirPath.c_str(), -1, &wDirPath[0], wLen ); | ||
| CreateDirectoryW( wDirPath.c_str(), NULL ); | ||
|
Comment on lines
+318
to
+320
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
(1) is probably the better solution, but it's more code to change than (2). |
||
|
|
||
| pos = fileName.find( "/", ++pos ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,23 +43,33 @@ static void replaceFunction( | |
|
|
||
| #define REPLACE_FUNCTION( _name, _fname ) if( !strcmp(_name, #_fname) ) { replaceFunction( firstThunk, _fname ); } | ||
|
|
||
| extern "C" __declspec(dllexport) | ||
| DWORD cliprof_init( void* dummy ) | ||
| // Patch the IAT of a single loaded module for all opencl.dll imports. | ||
| static void patchModuleIAT( char* base ) | ||
| { | ||
| char *base = (char*)GetModuleHandle(NULL); | ||
| // Validate DOS header: | ||
| PIMAGE_DOS_HEADER dosHeader = (PIMAGE_DOS_HEADER)(base); | ||
| if( dosHeader->e_magic != IMAGE_DOS_SIGNATURE ) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Get pointer to NT headers: | ||
| PIMAGE_DOS_HEADER dosHeader = (PIMAGE_DOS_HEADER)(base); | ||
| PIMAGE_NT_HEADERS ntHeaders = (PIMAGE_NT_HEADERS)(base + dosHeader->e_lfanew); | ||
| if( ntHeaders->Signature != IMAGE_NT_SIGNATURE ) | ||
| { | ||
| return false; | ||
| return; | ||
| } | ||
|
|
||
| // Get pointer to import directory: | ||
| DWORD importOffset = | ||
| ntHeaders->OptionalHeader. | ||
| DataDirectory[IMAGE_DIRECTORY_ENTRY_IMPORT].VirtualAddress; | ||
|
|
||
| if( importOffset == 0 ) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| PIMAGE_IMPORT_DESCRIPTOR importDesc = | ||
| (PIMAGE_IMPORT_DESCRIPTOR)(base + importOffset); | ||
|
|
||
|
|
@@ -220,6 +230,35 @@ DWORD cliprof_init( void* dummy ) | |
| } | ||
| importDesc++; | ||
| } | ||
| } | ||
|
|
||
| extern "C" __declspec(dllexport) | ||
| DWORD cliprof_init( void* dummy ) | ||
| { | ||
| // Patch the IAT of all loaded modules, not just the main executable. | ||
| // 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] = {}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we allocate this dynamically (say as an |
||
| DWORD cbNeeded = 0; | ||
|
|
||
| if( EnumProcessModules( | ||
| GetCurrentProcess(), | ||
| hModules, | ||
| sizeof(hModules), | ||
| &cbNeeded ) ) | ||
| { | ||
| DWORD numModules = cbNeeded / sizeof(HMODULE); | ||
| for( DWORD i = 0; i < numModules; i++ ) | ||
| { | ||
| patchModuleIAT( (char*)hModules[i] ); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Fallback: patch only the main executable. | ||
| patchModuleIAT( (char*)GetModuleHandle(NULL) ); | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
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.
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_PLATFORMis only set for Visual Studio builds and only when the-Acompiler option is passed.If we want to make this more robust, one option is to check
CMAKE_CXX_COMPILER_ARCHITECTURE_IDinstead: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.