mingw: Use aro instead of clang for preprocessing import libs #17771

ehaas · 2023-10-29T05:25:03Z

Based on manual testing, this produces the same output for kernel32.def when compared to clang, other than minor whitespace differences (clang puts a newline at the beginning, and a trailing space on one line which aro does not)

Testing steps:

Clear zig cache
Create empty test.c
With status-quo zig: zig build-lib -target x86_64-windows-gnu -lkernel32 --verbose-cc test.c
a. This will print a zig clang line to stderr; the output path is the final argument
With this branch: ./zig-out/bin/zig build-lib -target x86_64-windows-gnu -lkernel32 --verbose-cc test.c
a. This will print output path: <ZIG_CACHE_PATH>/o/<SOME HASH>/kernel32.def
Diff the two files
Available targets are x86_64-windows-gnu, x86-windows-gnu, arm-windows-gnu, aarch64-windows-gnu

I manually tested error handling by editing kernel32.def.in to have an invalid preprocessing directive and got the following:

zig-out/lib/zig/libc/mingw/lib-common/kernel32.def.in:1:1: error: invalid preprocessing directive
#foo
^
1 error generated.
error: unable to generate DLL import .lib file for kernel32: AroPreprocessorFailed

Closes #17753

andrewrk · 2023-10-29T07:50:52Z

I suggest to put a check for build_options.only_core_functionality to prevent Aro from being compiled into zig1.wasm or zig2.c during the bootstrap process. This will also fix the current CI failures.

ehaas · 2023-10-30T06:46:58Z

Is there a way to test this locally if I don't have access to a windows machine? I don't want to spam the CI with attempts, since I'm not totally sure where the check should go. It seems like the function is being used, since we're hitting that panic - does that mean that if build_options.only_core_functionality is true, we should use the existing zig clang solution, and otherwise use the new aro one?

andrewrk · 2023-10-30T07:55:25Z

Yes, here is a way to test it:

put @compileError() somewhere inside aro
zig build -Donly-c

If you get the compile error, it's incorrectly including aro in the build.

The check goes at the top of buildImportLib:

if (build_options.only_core_functionality) @panic("building import libs not included in core functionality");

andrewrk · 2023-10-31T17:46:21Z

Oops, I gave you some bad advice. It should be checking only_c instead of only_core_functionality.

We don't want the logic to be part of zig1.wasm, however we do want the logic to be part of zig2.c.

The use case for only_core_functionality is for example package fetching. We certainly do not need zig2 to have the package manager logic inside of it.

ehaas · 2023-10-31T17:50:07Z

Ok, if I add that I assume it means it's also safe for me to remove the if (builtin.zig_backend == .stage2_c) @panic("the CBE cannot compile Aro yet!"); from later in the function, right before I import aro?

src/mingw.zig

andrewrk · 2023-10-31T17:51:11Z

src/mingw.zig

    }

    const lib_final_path = try comp.global_cache_directory.join(comp.gpa, &[_][]const u8{
        "o", &digest, final_lib_basename,
    });
    errdefer comp.gpa.free(lib_final_path);

+    if (!build_options.have_llvm) return error.ZigCompilerNotBuiltWithLLVMExtensions;


Ah, I forgot about this. I'll open another issue for making zig support writing import libraries from def files.

ehaas · 2023-11-02T22:14:10Z

@Vexu I will need your help integrating the arocc's GenerateDef changes with zig's build.zig - I'm not sure how to add the def files to the aro module.

Vexu · 2023-11-02T23:02:19Z

I don't think there's going to be an easy way to do so because of the bootstrapping process. I'd just copy the generated files from the cache and replace the def files.

Vexu · 2023-11-02T23:23:42Z

One possible option would be to add a stubbed def file and then in CMake add that as the module for the def files for the purpose of bootstrapping. For building them regularly it should be easy enough to copy some parts from Aro's build.zig.

This should enable bootstrapping:

pub fn with(comptime Properties: type) type {
    return struct {
        tag: Tag = @enumFromInt(0),
        properties: Properties = undefined,
        pub const max_param_count = 1;
        pub const longest_name = 0;
        pub const data = [_]@This(){.{}};
        pub inline fn fromName(_: []const u8) ?@This() {
            return if (@hasField(Properties, "declspec")) null else .{};
        }
        pub fn nameFromUniqueIndex(_: u16, _: []u8) []u8 {
            return "";
        }
        pub fn uniqueIndex(_: []const u8) ?u16 {
            return null;
        }
        pub const Tag = enum(u16) { _ };
        pub fn nameFromTag(_: Tag) NameBuf {
            return .{};
        }
        pub fn tagFromName(name: []const u8) ?Tag {
            return @enumFromInt(name.len);
        }
        pub const NameBuf = struct {
            pub fn span(_: *const NameBuf) []const u8 {
                return "";
            }
        };
    };
}

Closes ziglang#17753

ref: 45eb8a700bfa885af7144a54ffd01f291c6f598f

Vexu

~~I forgot about the "file exists in multiple modules" error again and made the stub work for both. It could be simplified for them individually but this works too.~~

I simplified the names stub while fixing the build error.

ehaas · 2023-11-03T19:08:46Z

The issue I was seeing was that stage3 was unable to build the zig compiler due to not finding the def files. I fixed it with the following diff:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index dcbddc476..d5fd9b4b1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -647,8 +647,11 @@ set(ZIG_STAGE2_SOURCES
     "${CMAKE_SOURCE_DIR}/src/windows_sdk.zig"
     "${CMAKE_SOURCE_DIR}/src/stubs/aro_builtins.zig"
     "${CMAKE_SOURCE_DIR}/src/stubs/aro_attributes.zig"
+    "${CMAKE_SOURCE_DIR}/deps/aro/Attribute/names.def"
+    "${CMAKE_SOURCE_DIR}/deps/aro/Builtins/Builtin.def"
 )

+
 if(MSVC)
     set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK")
     if(IS_DIRECTORY ${MSVC_DIA_SDK_DIR})
@@ -947,3 +950,10 @@ install(CODE "set(ZIG_BUILD_ARGS \"${ZIG_BUILD_ARGS}\")")
 install(CODE "set(CMAKE_INSTALL_PREFIX \"${CMAKE_INSTALL_PREFIX}\")")
 install(CODE "set(CMAKE_SOURCE_DIR \"${CMAKE_SOURCE_DIR}\")")
 install(SCRIPT "${CMAKE_SOURCE_DIR}/cmake/install.cmake")
+
+
+file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/deps/aro/Attribute/names.def
+     DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/deps/aro/Attribute)
+
+file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/deps/aro/Builtins/Builtin.def
+     DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/deps/aro/Builtins)

I'm not sure if this is the correct fix; I also did this without your changes to this branch or to aro from today.

andrewrk · 2023-11-03T19:10:59Z

Please don't add any additional logic to CMakeLists.txt. Zig should depend on Aro only as a set of zig files. I don't want to add any Aro-related build system logic to zig's build process.

Co-authored-by: Veikka Tuominen <[email protected]>

Vexu · 2023-11-03T20:20:36Z

The issue I was seeing was that stage3 was unable to build the zig compiler due to not finding the def files.

I think the issue was Vexu/arocc@9f4c28a

ehaas · 2023-11-03T21:07:41Z

If we want to completely eliminate the CMakeLists.txt changes (so, no stub modules or anything) I have a local patch that does that by replacing the .def file contents with the generated versions. It succeeds at the bootstrap + stage4 build. Let me know if that is desired and I can push it.

andrewrk · 2023-11-07T19:46:35Z

Thanks @ehaas. I think this is fine - passing a few more args to building zig2.c is OK, I just didn't want to add any more steps to the process. For example, to update #17892 after merging this PR, I will only need to adjust a few command line args, but not introduce more logic into bootstrap.c.

ehaas force-pushed the mingw-aro branch 2 times, most recently from 68f16db to fb0fb5b Compare October 29, 2023 06:53

ehaas force-pushed the mingw-aro branch from fb0fb5b to b79112d Compare October 31, 2023 07:38

andrewrk requested changes Oct 31, 2023

View reviewed changes

ehaas mentioned this pull request Nov 1, 2023

Zig workaround: don't use packed structs larger than 128 bits. Vexu/arocc#532

Merged

ehaas added 5 commits November 2, 2023 19:17

mingw: Use aro instead of clang for preprocessing import libs

9f2cb92

Closes ziglang#17753

mingw: Don't implement buildImportLib in only-c mode

ba6eb0b

mingw: Check for only_c, not only_core_functionality

59749e4

mingw: remove "aro" literal from from cache hash

f83f14a

mingw: remove stage2_c check for buildImportLib

ac3772a

ehaas force-pushed the mingw-aro branch from bfb637f to dff675d Compare November 3, 2023 05:22

sync Aro dependency

9ea0c3b

ref: 45eb8a700bfa885af7144a54ffd01f291c6f598f

ehaas force-pushed the mingw-aro branch from dff675d to 7b260f0 Compare November 3, 2023 05:52

Vexu approved these changes Nov 3, 2023

View reviewed changes

Vexu force-pushed the mingw-aro branch 2 times, most recently from 1459d8a to caa2c12 Compare November 3, 2023 17:42

ehaas and others added 2 commits November 3, 2023 22:18

aro: Add stub definitions to enable bootstrapping

e6b9312

Co-authored-by: Veikka Tuominen <[email protected]>

comp: remove CBE guards for aro

3eb8a09

Vexu force-pushed the mingw-aro branch from caa2c12 to 3eb8a09 Compare November 3, 2023 20:19

Vexu requested a review from andrewrk November 7, 2023 10:59

andrewrk merged commit 77bc8e7 into ziglang:master Nov 7, 2023
10 checks passed

ehaas deleted the mingw-aro branch February 25, 2024 04:39

squeek502 mentioned this pull request Mar 13, 2024

Cross compile to Windows fails with relative global cache directory path #19284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mingw: Use aro instead of clang for preprocessing import libs #17771

mingw: Use aro instead of clang for preprocessing import libs #17771

ehaas commented Oct 29, 2023

andrewrk commented Oct 29, 2023

ehaas commented Oct 30, 2023

andrewrk commented Oct 30, 2023

andrewrk commented Oct 31, 2023

ehaas commented Oct 31, 2023

andrewrk Oct 31, 2023

andrewrk Oct 31, 2023

ehaas commented Nov 2, 2023

Vexu commented Nov 2, 2023

Vexu commented Nov 2, 2023 •

edited

Loading

Vexu left a comment •

edited

Loading

ehaas commented Nov 3, 2023

andrewrk commented Nov 3, 2023

Vexu commented Nov 3, 2023

ehaas commented Nov 3, 2023

andrewrk commented Nov 7, 2023

mingw: Use aro instead of clang for preprocessing import libs #17771

mingw: Use aro instead of clang for preprocessing import libs #17771

Conversation

ehaas commented Oct 29, 2023

andrewrk commented Oct 29, 2023

ehaas commented Oct 30, 2023

andrewrk commented Oct 30, 2023

andrewrk commented Oct 31, 2023

ehaas commented Oct 31, 2023

andrewrk Oct 31, 2023

Choose a reason for hiding this comment

andrewrk Oct 31, 2023

Choose a reason for hiding this comment

ehaas commented Nov 2, 2023

Vexu commented Nov 2, 2023

Vexu commented Nov 2, 2023 • edited Loading

Vexu left a comment • edited Loading

Choose a reason for hiding this comment

ehaas commented Nov 3, 2023

andrewrk commented Nov 3, 2023

Vexu commented Nov 3, 2023

ehaas commented Nov 3, 2023

andrewrk commented Nov 7, 2023

Vexu commented Nov 2, 2023 •

edited

Loading

Vexu left a comment •

edited

Loading