Fix Base.libllvm_path and jl_get_libllvm don't support non-ASCII characters in path on Windows (#45126) #45127
Conversation
This file contains hidden or 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
vtjnash
approved these changes
Apr 30, 2022
src/codegen.cpp
Outdated
| DWORD n16 = GetModuleFileNameW(mod, path16, MAX_PATH); | ||
| if (n16 <= 0) | ||
| return jl_nothing; | ||
| char path8[MAX_PATH * 2]; |
There was a problem hiding this comment.
I can bump it up to 3 to be safe
vtjnash
reviewed
Apr 30, 2022
src/codegen.cpp
Outdated
| if (n16 <= 0) | ||
| return jl_nothing; | ||
| char path8[MAX_PATH * 2]; | ||
| if (!WideCharToMultiByte(CP_UTF8, 0, path16, n16, path8, MAX_PATH * 2, NULL, NULL)) |
There was a problem hiding this comment.
Might need +1 to add a trailing null byte here?
There was a problem hiding this comment.
Yeah, you're right, I need to fix the null, I'll do that.
|
Is this somehow related to #33486? |
I'm not sure, but I doubt it. This is a very local issue just inside jl_get_libllvm. But there do seem to be a few more non-ASCII username bugs floating around, so I could be wrong. |
|
I still have similar problems |
|
Sorry we forgot to merge this! |
|
@KristofferC that is a rather awkward commit message, which seems to mention several changes that you squashed out of the final version |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Fixes issue #45126, where Base.libllvm_path doesn't support non-ASCII characters. jl_get_libllvm_impl has been changed from using GetModuleFileNameA to using GetModuleFileNameW, along with WideCharToMultiByte to turn it into UTF-8.