-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
[rc] Add llvm-tools-<maj>
on windows as well
#285
Open
h-vetinari
wants to merge
7
commits into
conda-forge:rc
Choose a base branch
from
h-vetinari:rc
base: rc
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
45aa599
Add `llvm-tools-<maj>` on windows as well
h-vetinari d190fca
compile the wrappers for binaries in llvm-tools on win
h-vetinari 37056e9
when forwarding call to versioned binary, always quote arguments
h-vetinari 0c711d9
remove obsolete CMake policy setting
h-vetinari 36e09b7
reduce footprint of wrappers by linking to msvcrt.dll
h-vetinari 9a1589c
only search for versioned binary in folder of wrapper
h-vetinari 6b37242
document overall idea of win_forwarder.c
h-vetinari File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
// adapted from example in MSFT docs, see | ||
// https://learn.microsoft.com/en-us/windows/win32/procthread/creating-processes | ||
#include <windows.h> | ||
#include <stdio.h> | ||
#include <string.h> | ||
#include <tchar.h> | ||
|
||
// the idea here is as follows; this wrapper will be compiled and renamed into | ||
// a file corresponding to the versioned binaries created in install_llvm.bat. | ||
// For example, we'll have llc.exe and llc-19.exe, both under %LIBRARY_BIN%. | ||
// To avoid security holes, we ensure that the wrapper can only call binaries | ||
// in %LIBRARY_BIN%, otherwise we fail. For example: | ||
// * user calls `llc -version`; argv[0] == "llc", argv[1] == "-version" | ||
// * determine path of calling binary, i.e. %LIBRARY_BIN%\llc.exe | ||
// * construct versioned path, i.e. %LIBRARY_BIN%\llc-19.exe | ||
// * collect all other arguments & quote them, e.g. `"argv[1]" "argv[2]" ...` | ||
// * invoke `%LIBRARY_BIN%\llc-19.exe "argv[1]" "argv[2]" ...` | ||
// * collect return value from inner call and return the same from wrapper | ||
|
||
int _tmain( int argc, TCHAR *argv[] ) | ||
{ | ||
STARTUPINFO si; | ||
PROCESS_INFORMATION pi; | ||
DWORD error_code; | ||
|
||
ZeroMemory( &si, sizeof(si) ); | ||
si.cb = sizeof(si); | ||
ZeroMemory( &pi, sizeof(pi) ); | ||
|
||
// reconstruct commandline call we received, pointing to versioned binary; | ||
// 32767 == maximum length for lpCommandLine argument | ||
char forwarded[32767]; | ||
// initialize buffer with dummy so that strlen below is not UB | ||
strcat(forwarded, "xxx"); | ||
|
||
// argv[0] is the name of the calling function, which might be a relative path; | ||
// for security reasons, get the absolute path of the binary we're calling from | ||
// with help from the MIT-licensed https://github.com/gpakosz/whereami | ||
void* addr = _ReturnAddress(); | ||
HMODULE hm = NULL; | ||
// non-zero return means success | ||
if (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | | ||
GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, | ||
(LPCSTR) addr, &hm)) | ||
GetModuleFileNameA(hm, forwarded, sizeof(forwarded)); | ||
|
||
unsigned int len = strlen(forwarded); | ||
if (len < 4) { | ||
// realistically, minimum len(r"C:\a") == 4; dummy value has length 3 | ||
printf( "Could not determine location of calling binary!\n" ); | ||
return GetLastError(); | ||
} | ||
// chop off ".exe" (if the binary we're in was invoked like that) | ||
if (strcmp(&forwarded[len-4], ".exe") == 0) | ||
// set null-terminator to finish the string early | ||
forwarded[len-4] = '\0'; | ||
// point to versioned binary | ||
strcat(forwarded, "-{{ majorversion }}.exe"); | ||
// rest stays the same, but wrap everything in quotes, because | ||
// the contents of argv[i] get stripped of those, which fails | ||
// if there's any argument that relies on atomicity, e.g. paths | ||
// with spaces in them (c.f. "C:\Program Files\...") | ||
for (int i = 1; i < argc; i++) { | ||
strcat(forwarded, " \""); | ||
strcat(forwarded, argv[i]); | ||
strcat(forwarded, "\""); | ||
} | ||
|
||
// Start the child process. | ||
if( !CreateProcess( NULL, // No module name (use command line) | ||
(char*)&forwarded, // Command line | ||
NULL, // Process handle not inheritable | ||
NULL, // Thread handle not inheritable | ||
FALSE, // Set handle inheritance to FALSE | ||
0, // No creation flags | ||
NULL, // Use parent's environment block | ||
NULL, // Use parent's starting directory | ||
&si, // Pointer to STARTUPINFO structure | ||
&pi ) ) // Pointer to PROCESS_INFORMATION structure | ||
{ | ||
printf( "Forwarding call to versioned binary failed:\n" ); | ||
printf( "%s", forwarded ); | ||
return GetLastError(); | ||
} | ||
|
||
// Wait until child process exits. | ||
WaitForSingleObject( pi.hProcess, INFINITE ); | ||
|
||
// Needs be called before the thread terminates, even though the value | ||
// only gets populated after the thread terminates. | ||
// If GetExitCodeProcess succeeds, it will correctly set &error_code to | ||
// the result of the inner call, but will return a non-zero value itself. | ||
// If it fails, use the last available error. For details see | ||
// https://learn.microsoft.com/en-gb/windows/win32/api/processthreadsapi/nf-processthreadsapi-getexitcodeprocess | ||
if( !GetExitCodeProcess( pi.hProcess, &error_code ) ) | ||
error_code = GetLastError(); | ||
|
||
// Close process and thread handles. | ||
CloseHandle( pi.hProcess ); | ||
CloseHandle( pi.hThread ); | ||
|
||
return error_code; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What's the license on this?
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.
MIT, see here, which covers the code in doc source
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.
Any other concerns besides the license?
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.
Gentle ping on this @isuruf
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.
Another ping here... I'm aware this is a trade-off between consistency across platforms and extra complexity (though we hopefully shouldn't have to touch it going forward; might also consider breaking this out into something reusable wherever we have versioned binaries). I don't have a clear answer what is better, so I'd appreciate your input @isuruf.
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.
Can we just use symlink-exe-substitute-feedstock ?
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.
It looks like that's built to symlink an exe into another folder, not to a differently-named file in the same folder (it also doesn't use the relative path logic). That said, the approach could probably be extended to name the target symlink, and it's cleaner because it doesn't duplicate the 12kb per link (at the cost of introducing an extra run-dependency).