Skip to content
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

Merged
merged 16 commits into from Nov 1, 2017
Merged

Conversation

dimenus
Copy link
Contributor

@dimenus dimenus commented Oct 30, 2017

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

@dimenus dimenus changed the title WIN32: Linking with the CRT WIN32: Linking with the CRT at runtime. Oct 30, 2017
@andrewrk
Copy link
Member

andrewrk commented Oct 30, 2017

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:

In file included from C:/projects/zig-d3l86/src/os.cpp:29:0:
C:/projects/zig-d3l86/src/windows_com.hpp:463:3: error: '_Deref_out_opt_' has not been declared
   _Deref_out_opt_ IEnumSetupInstances** ppenum
   ^~~~~~~~~~~~~~~
C:/projects/zig-d3l86/src/windows_com.hpp:463:38: error: expected ',' or '...' before '*' token
   _Deref_out_opt_ IEnumSetupInstances** ppenum
                                      ^

@dimenus
Copy link
Contributor Author

dimenus commented Oct 30, 2017

I'm looking into the ramifications of removing that now. Thanks for the quick review!

Copy link
Member

@andrewrk andrewrk left a 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();
Copy link
Member

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.

@andrewrk
Copy link
Member

POSIX build broke too, looks like some missing #ifdefs:

/home/travis/build/zig-lang/zig/src/os.cpp:1015:2: error: unknown type name
      'HKEY'
        HKEY key;
        ^

@dimenus dimenus force-pushed the master branch 2 times, most recently from 4e1ad52 to 1e64aad Compare October 31, 2017 17:04
src/analyze.cpp Outdated
if (!g->libc_include_dir || buf_len(g->libc_include_dir) == 0) {
Win32SDK win_sdk;
Copy link
Member

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.

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;
Copy link
Member

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.

@andrewrk andrewrk merged commit 38f05d4 into ziglang:master Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants