-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
WIN32: Linking with the CRT at runtime. #570
Conversation
Excellent work. I'm going through the code now. If you look at the appveyor link below, this pull request builds successfully in MSVC and passes all the tests. But it breaks the MinGW build:
|
I'm looking into the ramifications of removing that now. Thanks for the quick review! |
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.
Really great work. I left 1 comment for you.
src/os.cpp
Outdated
@@ -999,3 +1000,266 @@ void os_stderr_set_color(TermColor color) { | |||
set_color_posix(color); | |||
#endif | |||
} | |||
|
|||
static Buf* WindowsSDKPath = buf_alloc(); |
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.
No code execution in static initializers please. Two reasons: 1, we don't have code execution in static initializers in zig, and the C++ code is roughly written in a zig style even though it's c++. 2. code execution in static initializers is kind of a hack, that I'd rather not depend on. Better to not have too much hidden stuff run before main().
More importantly, I think that os.cpp is not a good place to save state such as WindowsSDKPath and WindowsSDKVersion. For example, when zig acts as a compiler server (which is planned) we don't want to have to restart Zig if you install/uninstall MSVC with zig running.
A better place to save the state of already having found MSVC is the CodeGen struct (in all_types.hpp).
It's still good to isolate the COM stuff into this os.cpp file. But os.hpp could have an opaque type that has the found MSVC state which codegen/analyze can pass to e.g. os_get_win32_ucrt_lib_path
. And then we don't want to forget to pass the state from parent CodeGen to child CodeGen in link.cpp (in the code where it builds compiler_rt.o and builtin.o).
If this is unclear, let me know, I'd be happy to finish that up in this pull request. You've done the bulk of the work, and what's left is kind of the glue / refactoring which I'd be happy to take care of for you.
POSIX build broke too, looks like some missing
|
4e1ad52
to
1e64aad
Compare
src/analyze.cpp
Outdated
if (!g->libc_include_dir || buf_len(g->libc_include_dir) == 0) { | ||
Win32SDK win_sdk; |
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 should go on the CodeGen
struct, and we should use the same value for find_libc_include_path
and find_libc_lib_path
.
src/all_types.hpp
Outdated
Buf *zig_lib_dir; | ||
Buf *zig_std_dir; | ||
Buf *zig_c_headers_dir; | ||
Buf *zig_std_special_dir; | ||
Buf *dynamic_linker; | ||
Buf *ar_path; | ||
Buf* win32_sdk_path; |
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.
instead of these 2 things, you probably want a Win32SDK
here.
Disclaimer: Forgive me if my format sucks, I've never submitted a PR before!
Fixes: #517
I added a few things to allow zig to link with the CRT properly both statically and dynamically. In Visual Studio 2017, Microsoft changed how the c-runtime is factored again. With this change, they also added a COM interface to allow you to query the respective Visual Studio instance for two of them. This does that and also falls back on a registry query for 2015 support. If you're using a Visual Studio instance older than 2015, you'll have to use the existing options available with the zig compiler. Changes are listed below along with a general description of the changes.
all_types.cpp:
The separate variables for msvc/kern32 have been removed and all win32 libc directory paths have been combined into a ZigList since we're querying more than two directories and differentiating one from another doesn't matter to lld.
analyze.cpp:
The existing functions were extended to support querying libc libs & libc headers at runtime.
codegen.cpp/hpp:
Microsoft uses the new 'Universal C Runtime' name now. Doesn't matter from a functionality standpoint. I left the compiler switches as is to not introduce any breaking changes.
link.cpp:
We're linking 4 libs and generating another in order to support the UCRT.
Dynamic: msvcrt/d, vcruntime/d, ucrt/d, legacy_stdio_definitions.lib
Static: libcmt/d, libvcruntime/d libucrt/d, legacy_stdio_definitions.lib
main.cpp:
Update function call names.
os.cpp/hpp:
COM/Registry interface for querying Windows UCRT/SDK.
Sources:
Windows CRT
VS 2015 Breaking Changes