Normalized C++ compilation options for single-threaded targets #10143

nuald · 2021-11-13T21:45:29Z

The PR contains two major changes:

C++ is compiled without threading support for single-threaded targets (was already partially implemented for WASM, now it's applied to all single-threaded targets).
RunStep has been extended to run artifacts with the external executors if enabled.

The reason for (1) is that the current functionality is a little bit confusing as C++ standard library is compiled with the threading support even if an artifact is explicitly flagged to use the single-threaded mode. I've discovered that during dealing with #6573. The original LLVM commit referenced there is being abandoned, and the only way to use C++ for ARM in Zig is using modified standard library, hence this PR.

There are few reason for (2), but none of them are critical, therefore the change could be dropped:

.enable_qemu and similar flags could be applied to the executable, but the corresponding RunStep if invoked would give the IncorrectExe error. It's not immediately obvious that the RunStep is applicable for native targets only, and .enable_qemu is no-op for executables, but for tests only. In theory, those flags should be applied to tests and run steps only, but it's out of scope of this PR.
some tests involve running the actual executables, e.g. the integration tests like the c_compiler modified in this PR. It's handy to have this ability out-of-box.
some build processes require cross-platform runs. One particular example here is one of my project that has the pure Zig build process for LuaJIT: buildvm should be invoked on the target platform (please see Cross-compiling 32 bit LuaJIT on a 64 bit host LuaJIT/LuaJIT#664 for the reference). I could successfully compile and use LuaJIT on ARM, but need to use the external executor for buildvm. The first commit uses ExecutorRunStep that is partially taken from the project.

Testing done by:

running the full test suite: zig build test
running the c_compiler test with 3 targets (x86_64 Linux host, arm-linux-musleabi, wasm32-wasi):

cd test/standalone/c_compiler
zig build test
zig build test -Dtarget=arm-linux-musleabi -Denable-qemu -Dsingle-threaded
zig build test -Dtarget=wasm32-wasi -Denable-wasmtime -Dsingle-threaded
zig build test -Dtarget=x86_64-windows-gnu -Denable-wine
zig build test -Dtarget=x86_64-windows-gnu -Denable-wine -Dsingle-threaded
zig build test -Dtarget=x86_64-macos-gnu -Denable-darling
zig build test -Dtarget=x86_64-macos-gnu -Denable-darling -Dsingle-threaded

Additional notes:

for WASM target the -fno-exceptions flag is applied to C++ standard library only, not the final executables. If could be addressed in this or another PR if you want;
macos doesn't properly support C++ exceptions, additional work is required (probably libunwind and DWARF CFI should be added), and for now I've just disabled testing it;
_LIBCPP_HAS_NO_THREADS is added for buildLibCXXABI for single-threaded targets too as it uses C++ standard library;
test.c/test.cpp code have mixed spaces/tabs, I've restyled the code for the consistency;
RunStep.getExternalExecutor is a little bit verbose, but I anticipate that other changes could be required so left it as is for now

kubkon · 2021-11-14T05:01:38Z

lib/std/build/RunStep.zig

@@ -136,6 +137,23 @@ pub fn expectStdErrEqual(self: *RunStep, bytes: []const u8) void {
    self.stderr_action = .{ .expect_exact = self.builder.dupe(bytes) };
 }

+/// Returns true if the step could be run, otherwise false
+pub fn isRunnable(


I might have missed that, but why is this logic duplicated from cross_target.zig?

I'm not quite sure what exactly is duplicated here as CrossTarget declares the executors to use, and this code is just using those. In particular this helper function could be used in the RunStep to check if the step is viable (going to work) to avoid runtime errors.

Oh sorry, what I meant to point out is that we already have the ability to run (cross-target) tests that require an emulator or similar, therefore I don't really see the need for this in RunStep.zig. See for instance output of zig test --help:

... Test Options: --test-filter [text] Skip tests that do not match filter --test-name-prefix [text] Add prefix to all tests --test-cmd [arg] Specify test execution command one arg at a time --test-cmd-bin Appends test binary path to test cmd args --test-evented-io Runs the test in evented I/O mode --test-no-exec Compiles test binary without running it ...

We already have a way of specifying a custom execution command which is exactly how we invoke foreign tests on some host for instace, like Wasm in Wasmtime.

Yes, but as I mentioned in the comment, the current RunStep behavior is inconsistent. I guess if it would be called NativeRunStep then it will be less confusing, but right now, then one specifies that it's possible to use QEMU, but RunStep gives IncorrectExe error, it's not immediately obvious that RunStep works with native target only.

Moreover, sometimes it's required to run the exe (not tests) in the emulator. Few use-cases for myself are:

complicated build processes (like LuaJIT compilation I've mentioned in the PR comment). buildvm (compiled from the source code using Zig in my toolchain) that is used for building LuaJIT VM doesn't support cross-compilation, and it's required to be invoked with the target architecture (i.e. to produce 32-bit LuaJIT, I have to use 32-bit buildvm). It can't be run as a test as it's standalone program and doesn't export anything to be linked into tests. Please take a look to build.zig for the reference.

cross-platform benchmarks. I'm working with the embedded systems, and need to measure the libraries footprint (e.g. pcre vs hyperscan for regexes). The benchmarks are compiled into executables for the deployment on the target platforms, and it's handy to run them first with QEMU to verify file sizes and memory consumption. While I could definitely use shell scripting for that, using extended RunStep functionality provided in this PR, makes the task much easier.

However, as I said, it's not critical as I have the workaround (like ExecutorRunStep in the referenced sources above). If you still have doubts, I can just remove it.

Ah gotcha. Hmm, ok, I'll need to have @andrewrk take another look at this as we might already have a way to do this that I'm not aware of.

kubkon · 2021-11-14T05:01:56Z

lib/std/build/RunStep.zig

@@ -148,6 +166,42 @@ fn stdIoActionToBehavior(action: StdIoAction) std.ChildProcess.StdIo {
    };
 }

+fn getExternalExecutor(artifact: *LibExeObjStep) !?[]const u8 {


Again, like in the comment above, why do we need to duplicate getExternalExecutor here?

It doesn't duplicate the code but the name. I guess I can rename it to avoid confusion. This function utilizes CrossTarget's getExternalExecutor(), but besides that have a check if the executor is enabled. It's similar to the logic for tests.

kubkon · 2021-11-14T16:18:37Z

Just an FYI that C++ exceptions require additional functionality in the zld linker to work on macOS. It's on my roadmap but didn't have time to add that in yet.

nuald · 2021-11-14T18:09:53Z

Just an FYI that C++ exceptions require additional functionality in the zld linker to work on macOS. It's on my roadmap but didn't have time to add that in yet.

Yes, I've noticed that you ~~read __eh_info from __TEXT segment instead of __unwind_info and even~~ have a comment about that, but the change seems out of scope for this PR. I was planning to fix it later, but macOS support for my projects are not priority and I guess you'll fix it faster :)

UPDATE: Looks like __eh_info is still located in __TEXT segment, it's just not used for unwinding if __unwind_info presented:

If the linker sees both compact unwind and dwarf unwind for a function, it picks the compact unwind and discards the dwarf.

andrewrk

Thank you for these changes, @nuald. I think we can definitely get this to a mergeable point, and it will be a nice improvement. I have a couple of requests which involve moving logic from the build system to src/Compilation.zig.

But also I want to make some build system improvements prior to merging this. I hope you don't mind if I go ahead and work on those, and then ask you to rebase this.

lib/std/build.zig

test/standalone/c_compiler/build.zig

Previously there was only `--single-threaded`. This flag now matches other boolean flags, instead of only being able to opt in to single-threaded builds, you can now force multi-threaded builds. Currently this only has the possibility to emit an error message, but it is a better user experience to understand why one cannot choose to enable threads in some cases. This is breaking change to the CLI. Related: #10143

andrewrk · 2021-12-03T00:22:54Z

test/standalone/c_compiler/build.zig

+    // macos C++ exceptions could be compiled, but not being catched,
+    // additional support is required, possibly unwind + DWARF CFI
+    if (target.getCpuArch().isWasm() or os_tag == .macos) {
+        exe_cpp.defineCMacro("_LIBCPP_NO_EXCEPTIONS", null);
+    }


What's going on here? this define logic belongs in the compiler, not in this build.zig file.

As you know, C++ standard library (use older STL acronym later) heavily uses exceptions, and I've updated the test to use them (as it could be a key functionality for C++ applications like in my use-case). Unfortunately, wasm and macOS (currently) has one issue that I'm not quite sure how to overcome: the code is compiled, but doesn't work correctly. I guess the proper solution would be to stop compiling the code if compiler detects that exceptions are used for wasm and macOS, but it means that C++ STL won't be compiled at all on those platforms, and that would be quite breaking change (as STL still could be used with incorrectly working exceptions if used carefully). I didn't want to risk introducing it as it could affect way too many users.

So, for now I've just declared a macro (it could have any name though) that disables exception code in the test. I was going to fix macOS exception handling later if you team won't have time for that, but for now I've just inserted that stub.

andrewrk · 2021-12-03T10:35:05Z

Alright I did the aforementioned prerequisite changes in 1cac99c. If you don't mind, can you rework your changes against latest master and let's go from there?

nuald · 2021-12-19T17:27:33Z

@andrewrk Could you please re-review the changes?

andrewrk · 2022-01-29T21:47:36Z

Sorry for the delay, I have this in my high priority list for next week.

… libcxx. Fixed single-threaded mode for Windows.

* back out the changes to RunStep * move the disabled test to the .cpp code and avoid a confusing name-collision with the _LIBCPP macro prefix * fix merge conflict with the edits to the same test that ensure global initializers are called. Now this branch is only concerned with single-threaded targets and passing the correct macro defines to libc++.

kubkon reviewed Nov 14, 2021

View reviewed changes

nuald force-pushed the single-threaded-cpp1 branch 3 times, most recently from 58ed467 to 58fd5ae Compare November 23, 2021 01:30

andrewrk requested changes Dec 1, 2021

View reviewed changes

lib/std/build.zig Outdated Show resolved Hide resolved

test/standalone/c_compiler/build.zig Outdated Show resolved Hide resolved

test/standalone/c_compiler/build.zig Outdated Show resolved Hide resolved

andrewrk modified the milestones: 0.9.0, 0.9.1 Dec 1, 2021

andrewrk removed this from the 0.9.0 milestone Dec 1, 2021

nuald force-pushed the single-threaded-cpp1 branch from 58fd5ae to 44037f8 Compare December 2, 2021 06:19

andrewrk mentioned this pull request Dec 2, 2021

improve detection of how to execute binaries on the host #10265

Merged

andrewrk requested changes Dec 3, 2021

View reviewed changes

nuald force-pushed the single-threaded-cpp1 branch from 44037f8 to b38cb84 Compare December 17, 2021 05:58

Added _LIBCPP_HAS_NO_THREADS for single_threaded binaries linked with…

3997828

… libcxx. Fixed single-threaded mode for Windows.

andrewrk force-pushed the single-threaded-cpp1 branch from b38cb84 to 6c4ed42 Compare May 10, 2022 23:42

andrewrk force-pushed the single-threaded-cpp1 branch from 6c4ed42 to 4541509 Compare May 11, 2022 00:32

andrewrk merged commit f5edf78 into ziglang:master May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalized C++ compilation options for single-threaded targets #10143

Normalized C++ compilation options for single-threaded targets #10143

nuald commented Nov 13, 2021 •

edited

kubkon Nov 14, 2021

nuald Nov 14, 2021

kubkon Nov 14, 2021

nuald Nov 14, 2021

kubkon Nov 14, 2021

kubkon Nov 14, 2021

nuald Nov 14, 2021

kubkon commented Nov 14, 2021

nuald commented Nov 14, 2021 •

edited

andrewrk left a comment

andrewrk Dec 3, 2021

nuald Dec 4, 2021

andrewrk commented Dec 3, 2021

nuald commented Dec 19, 2021

andrewrk commented Jan 29, 2022

Normalized C++ compilation options for single-threaded targets #10143

Normalized C++ compilation options for single-threaded targets #10143

Conversation

nuald commented Nov 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubkon commented Nov 14, 2021

nuald commented Nov 14, 2021 • edited

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk commented Dec 3, 2021

nuald commented Dec 19, 2021

andrewrk commented Jan 29, 2022

nuald commented Nov 13, 2021 •

edited

nuald commented Nov 14, 2021 •

edited