Skip to content

Using @sin, @cos on x86_64-macos fails with missing symbol _sincosf when building optimized #10318

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

Closed
ikskuh opened this issue Dec 12, 2021 · 22 comments · Fixed by #11684
Closed
Labels
bug Observed behavior contradicts documented or intended behavior os-macos
Milestone

Comments

@ikskuh
Copy link
Contributor

ikskuh commented Dec 12, 2021

Zig Version

0.9.0-dev.1945+efdb94486

Steps to Reproduce

Build the following file:

//  zig build-exe -O ReleaseSafe -target x86_64-macos repro.zig 
const std = @import("std");

pub fn main() !void {
    var runtime_val: f32 = @intToFloat(f32, std.time.timestamp());

    var mat = rotationMat(runtime_val);

    std.debug.print("{any}\n", .{mat});
}

fn rotationMat(angle: f32) [2][2]f32 {
    const s = @sin(angle);
    const c = @cos(angle);
    return .{
        .{ c, -s },
        .{ s, c },
    };
}

Expected Behavior

The code builds successfully, like when building to x86_64-linux

Actual Behavior

[felix@denkplatte-v2 8eb0bcd9]$ zig build-exe -O ReleaseSafe -target x86_64-macos repro.zig 
error(link): undefined reference to symbol '_sincosf'
error(link):   first referenced in '/tmp/8eb0bcd9/zig-cache/o/967695a63a2a01c5bfcf22751e917e32/repro.o'
error: UndefinedSymbolReference
exit code: 1
@ikskuh ikskuh added the bug Observed behavior contradicts documented or intended behavior label Dec 12, 2021
ikskuh pushed a commit to ikskuh/TinyVG that referenced this issue Dec 12, 2021
@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

What about building non-optimized such as Debug?

@ikskuh
Copy link
Contributor Author

ikskuh commented Dec 12, 2021

Works fine. Only release modes fail

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

After quick investigation, similar thing happens when targeting aarch64-macos, and also, there is no symbol _sincosf in libcompiler_rt.a regardless of the build mode:

Debug

$ zig build-exe main.zig -target x86_64-macos --verbose-link                                  
zig ld -dynamic zig-cache/o/d4f43bef5af52e99c4982df0aa8e24c0/main.o /home/kubkon/.cache/zig/o/fd7f14d344898ae4ac14081091432de5/libcompiler_rt.a -o main -lSystem -lc
$ llvm-nm /home/kubkon/.cache/zig/o/fd714d344898ae4ac14081091432de5/libcompiler_rt.a | grep sincosf

ReleaseSafe

$ zig build-exe main.zig -target x86_64-macos -OReleaseSafe --verbose-link                                  
zig ld -dynamic zig-cache/o/df22d71174e2aecc6185c0e43d17ca47/main.o /home/kubkon/.cache/zig/o/2c0e99c5e79b03827fc761a65678893c/libcompiler_rt.a -o main -lSystem -lc
$ llvm-nm /home/kubkon/.cache/zig/o/2c0e99c5e79b03827fc761a65678893c/libcompiler_rt.a | grep sincosf

@ikskuh
Copy link
Contributor Author

ikskuh commented Dec 12, 2021

I think I was at this point once already: #7267
I think the problem is that it is shipped via libc stubs?

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

Oh, I see now. We don't provide this function at all when compiling for macOS, and it works in Debug since LLVM calls separate functions _sinf and _cosf which incidentally are provided by libSystem.dylib. However, _sincosf is not provided under this name (LLVM optimises two calls into one call of _sincosf when building in Release); instead, it is provided by libSystem.dylib under the name of ___sincosf. I'll need to check if perhaps we could reexport ___sincosf from libSystem.dylib as _sincosf or provide our own implementation as part of libcompiler_rt.a when targeting macOS.

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

Here's a proposed fix:

diff --git a/lib/std/special/c_stage1.zig b/lib/std/special/c_stage1.zig
index 3ae93c2bd..a7c730028 100644
--- a/lib/std/special/c_stage1.zig
+++ b/lib/std/special/c_stage1.zig
@@ -687,7 +687,7 @@ export fn sincos(a: f64, r_sin: *f64, r_cos: *f64) void {
     r_cos.* = math.cos(a);
 }
 
-export fn sincosf(a: f32, r_sin: *f32, r_cos: *f32) void {
+pub export fn sincosf(a: f32, r_sin: *f32, r_cos: *f32) void {
     r_sin.* = math.sin(a);
     r_cos.* = math.cos(a);
 }
diff --git a/lib/std/special/compiler_rt.zig b/lib/std/special/compiler_rt.zig
index 95edfee86..6348f870c 100644
--- a/lib/std/special/compiler_rt.zig
+++ b/lib/std/special/compiler_rt.zig
@@ -45,6 +45,10 @@ comptime {
     const __getf2 = @import("compiler_rt/compareXf2.zig").__getf2;
     @export(__getf2, .{ .name = "__getf2", .linkage = linkage });
 
+    if (builtin.os.tag.isDarwin()) {
+        _ = @import("c_stage1.zig").sincosf;
+    }
+
     if (!is_test) {
         @export(__lesf2, .{ .name = "__cmpsf2", .linkage = linkage });
         @export(__ledf2, .{ .name = "__cmpdf2", .linkage = linkage });

Lemme know what every thinks!

EDIT: @MasterQ32 I haven't run the resulting binary on an actual macOS but I don't see why it would fail at runtime. Anyhow, if we agree this is the right approach, I'll make the necessary adjustments and test everything out. Also, FWIW I'm working on a standalone test harness for the self-hosted linker and this will naturally get a test case of its own.

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

Actually, it occurred to me just now that we should really re-export it as a weak symbol - this way, if Apple decides to expose the symbol under the name of _sincosf (rather than ___sincosf as it currently is), we will avoid symbol collision errors. Having said that though, the order for symbol resolution is always relocatable > archive > dylib meaning that even if Apple starts providing the symbol, we will always prefer our own version.

@LemonBoy
Copy link
Contributor

Lemme know what every thinks!

Instead of fixing the immediate problem it's better to ask why it's not working.

However, _sincosf is not provided under this name (LLVM optimises two calls into one call of _sincosf when building in Release); instead, it is provided by libSystem.dylib under the name of ___sincosf

According to LLVM source code this should not happen at all, unless Zig is somehow telling LLVM the abi is gnu...
Shall we check?

Aw shit, if you check the emitted .ll file you can clearly see that's the case:
target triple = "x86_64-unknown-macos-gnu"

And... that's exactly what I reported in #9089?
So fix that instead of working around the problem kthx.

I'll also say it loud again, none of this libc/libm shit has a place in compiler_rt.

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

Lemme know what every thinks!

Instead of fixing the immediate problem it's better to ask why it's not working.

However, _sincosf is not provided under this name (LLVM optimises two calls into one call of _sincosf when building in Release); instead, it is provided by libSystem.dylib under the name of ___sincosf

According to LLVM source code this should not happen at all, unless Zig is somehow telling LLVM the abi is gnu... Shall we check?

Wait, so I'm looking at the LLVM source, and it should produce an undefined symbol for __sincosf_stret regardless of the ABI. However, this doesn't seem what we are trying to import here anyway, and I don't see any prong that would generate ___sincosf on macOS, unless this is not what we are trying to import at all? If we should be importing the former, is somehow specifying the ABI as GNU take over the precedence over the import options generated in the TT.isOSDarwin() here?

Aw shit, if you check the emitted .ll file you can clearly see that's the case: target triple = "x86_64-unknown-macos-gnu"

And... that's exactly what I reported in #9089? So fix that instead of working around the problem kthx.

I'll also say it loud again, none of this libc/libm shit has a place in compiler_rt.

Also, this is the first time I'm looking into this, and I really don't think any hostile behaviour or a sarcastic outburst like this is warranted.

@SpexGuy
Copy link
Contributor

SpexGuy commented Dec 12, 2021

FWIW this has been the behavior on mac os with clang for a long time in my experience, I've profiled non-zig applications that use sincosf in release builds. It's a neat function, it puts (x, x + pi/2) into a simd vector and then does a simd sin calculation to get both answers at once.

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

FWIW this has been the behavior on mac os with clang for a long time in my experience, I've profiled non-zig applications that use sincosf in release builds. It's a neat function, it puts (x, x + pi/2) into a simd vector and then does a simd sin calculation to get both answers at once.

Missing symbols, is that what you mean?

@SpexGuy
Copy link
Contributor

SpexGuy commented Dec 12, 2021

No, I mean clang generating calls to sincosf in release modes when sin and cos are used near each other.

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

No, I mean clang generating calls to sincosf in release modes when sin and cos are used near each other.

Mhm, gotcha. Do you know what the symbol resolves to by any chance?

@SpexGuy
Copy link
Contributor

SpexGuy commented Dec 12, 2021

I don't, all I know is that instruments was able to dump the assembly of _sincosf when profiling.

@LemonBoy
Copy link
Contributor

Wait, so I'm looking at the LLVM source, and it should produce an undefined symbol for __sincosf_stret regardless of the ABI. However, this doesn't seem what we are trying to import here anyway, and I don't see any prong that would generate ___sincosf on macOS, unless this is not what we are trying to import at all? If we should be importing the former, is somehow specifying the ABI as GNU take over the precedence over the import options generated in the TT.isOSDarwin() here?

SINCOS_STRET_F32 is not SINCOS_F32, you can check the sin+cos merging logic in LegalizeDAG.cpp (check isSinCosLibcallAvailable and friends).

Let's take this small piece of code lifted from LLVM's unit tests:

define float @test1(float %x) nounwind {
  %call = tail call float @sinf(float %x) nounwind readnone
  %call1 = tail call float @cosf(float %x) nounwind readnone
  %add = fadd float %call, %call1
  ret float %add
}

declare float  @sinf(float) readonly
declare float @cosf(float) readonly

Now check and compare the output of:

  • llc-13 -O3 -mtriple=x86_64-macos11 -o - foo.ll
  • llc-13 -O3 -mtriple=x86_64-macos10.8 -o - foo.ll
  • llc-13 -O3 -mtriple=x86_64-none-gnu -o - foo.ll

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

SINCOS_STRET_F32 is not SINCOS_F32, you can check the sin+cos merging logic in LegalizeDAG.cpp (check isSinCosLibcallAvailable and friends).

Let's take this small piece of code lifted from LLVM's unit tests:

define float @test1(float %x) nounwind {
  %call = tail call float @sinf(float %x) nounwind readnone
  %call1 = tail call float @cosf(float %x) nounwind readnone
  %add = fadd float %call, %call1
  ret float %add
}

declare float  @sinf(float) readonly
declare float @cosf(float) readonly

Now check and compare the output of:

* `llc-13 -O3 -mtriple=x86_64-macos11 -o - foo.ll`

So for the first one, I do see a call to extern symbol ___sincosf_stret.

* `llc-13 -O3 -mtriple=x86_64-macos10.8 -o - foo.ll`

Interestingly, for this old version of macOS, I see two separate calls to _sinf and _cosf.

* `llc-13 -O3 -mtriple=x86_64-none-gnu -o - foo.ll`

And for the last one, I see a bizarre call to sincosf via PLT. OK, so gnu ABI is definitely wrong and seems to take precedence over the fact we're targeting macOS generating a wrong call. Judging by the triples used for the first two examples, we should set the macOS (or Darwin for that matter with the exception of the simulator and macabi ABIs) to none (or unspecified), am I correct?

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

Ohh, wait, the last example is for unspecified OS it seems. Does that mean it assumes native? I did try the first example as x86_64-macos11-gnu and it generated the same call as without the ABI specified.

@LemonBoy
Copy link
Contributor

Ohh, wait, the last example is for unspecified OS it seems. Does that mean it assumes native? I did try the first example as x86_64-macos11-gnu and it generated the same call as without the ABI specified.

The feature detection stuff is quite delicate, somehow the ABI part has lower priority than the OS part (eg. x86_64-macos10.8 vs x86_64-macos10.8-gnu, you get a sincosf call even though the target is macos).

@kubkon
Copy link
Member

kubkon commented Dec 12, 2021

Ohh, wait, the last example is for unspecified OS it seems. Does that mean it assumes native? I did try the first example as x86_64-macos11-gnu and it generated the same call as without the ABI specified.

The feature detection stuff is quite delicate, somehow the ABI part has lower priority than the OS part (eg. x86_64-macos10.8 vs x86_64-macos10.8-gnu, you get a sincosf call even though the target is macos).

Yeah, this is exactly what it is. I just did a quick test on the original test case filed in this issue, I swapped out the triple from the generated LLVM IR from x86_64-unknown-macos-gnu to

  • x86_64-unknown-macos11-none => llvm-nm main.o | grep sin returns ___sincosf_stret
  • x86_64-unknown-macos11-gnu => llvm-nm main.o | grep sin returns ___sincosf_stret
  • x86_64-unknown-macos-none => llvm-nm main.o | grep sin returns _sinf (meaning it wasn't optimised into a single extern call, meaning we didn't enter neither the OS prong nor the ABI prong in LLVM lowering code)
  • x86_64-unknown-macos-gnu => llvm-nm main.o | grep sin returns _sincosf (which is wrong)
  • x86_64-unknown-macos10-gnu => llvm-nm main.o | grep sin returns _sincosf (which, again, is wrong)

This seems to suggest we should assume the ABI as unspecified (by default) when targeting Darwin. Would you agree with this conclusion?

@LemonBoy
Copy link
Contributor

Would you agree with this conclusion?

Yep, IMO every other target beside linux (and possibly other known exceptions) should default to none instead of gnu, fixing the problem with BSDs too.

Specifying the macos version may or may not be needed, I don't know if we currently specify that in some other way.

@andrewrk
Copy link
Member

I'll also say it loud again, none of this libc/libm shit has a place in compiler_rt.

You have asserted this a few times now, and I have no doubt that you have solid reasoning behind it, but you have not shared the reasoning with me yet. There are no arguments against #7265 listed on that issue and my comment here is sad and lonely, with no response to it: #10163 (comment)

Consider the following case: A user builds zig code on freestanding into an ELF file. No C code is involved in this process whatsoever. The LLVM backend is used and we emit a call to the LLVM intrinsic @llvm.sin. Or better yet, @llvm.memcpy. So we will get undefined reference if such symbol is not provided.

You're claiming that the linker line should have: app.o compiler-rt.o libc.o on it. libc? This is a freestanding target with no C code and no C ABI to match. It makes no sense for libc.o to be on the linker line in such case.

I'm not convinced; I will definitely need to hear reasoning to swing my opinion around to a different one.

@deckarep
Copy link

deckarep commented Apr 14, 2022

For another datapoint: I'm also hitting the same build error when compiling on x86_64-macos for a game which utilizes the Raylib library.

Follow-up edit: Upon 2nd pass actually reading this whole thread, this has nothing to with Raylib and everything to do with LLVM's optimization of @sin/@cos.

This blows up with both ReleaseSafe/ReleaseFast and is fine in Debug mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior os-macos
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants