Skip to content

compiler_rt: export floorl #10163

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

Merged
merged 1 commit into from
Nov 21, 2021
Merged

compiler_rt: export floorl #10163

merged 1 commit into from
Nov 21, 2021

Conversation

jcmoyer
Copy link
Contributor

@jcmoyer jcmoyer commented Nov 17, 2021

This fixes a linker error when compiling with VS2022 on Windows:

zig1.obj : LNK2019: unresolved external symbol floorl referenced in function Sema.coerceInMemoryAllowedPtrs [D:\zig\build\zig.vcxproj]

I'm not entirely sure this is the correct place to export or if it should go into one of the conditional branches above.

E: So actually this is the classic C thing where long double isn't necessarily bigger than double, this will probably require some more thought. I'm going to have a closer look at what andrew suggested in IRC (related: #7265) since it looks like there's a better way to write this already in std/special/c_stage1.zig.

@jcmoyer jcmoyer marked this pull request as draft November 17, 2021 10:55
This fixes a stage1 linker error when compiling with VS2022 on Windows.
@jcmoyer
Copy link
Contributor Author

jcmoyer commented Nov 17, 2021

Is it correct to delete code from c_stage1.zig as it is moved to compiler_rt or will this cause problems elsewhere?

@matu3ba
Copy link
Contributor

matu3ba commented Nov 17, 2021

@jcmoyer As compiler_rt is intended to match LLVM (see the README), this does not look like the right place to me.
Another argument for keeping it matching to LLVM is simpler review, tests and fuzzing.

So personally I think another structure resembling compiler_rt sounds useful for this and I communicated it here.
see below.

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.

This looks OK to merge to me. What's the problem as it stands?

Is it correct to delete code from c_stage1.zig as it is moved to compiler_rt or will this cause problems elsewhere?

Yes it is correct 👍

@andrewrk
Copy link
Member

compiler_rt is intended to match LLVM

It's not intended to match LLVM exactly. It already has extra things for Zig to rely on that are not provided by LLVM's compiler-rt. See #7265 for more details.

@matu3ba
Copy link
Contributor

matu3ba commented Nov 18, 2021

It's not intended to match LLVM exactly.

I find it confusing to call the document compiler_rt.zig, when it is not describing/abstracting the files in the folder compiler_rt/ (which is ported and reimplemented from LLVM).
In llvm you have this with a differentiation between builtins, sanitizer runtimes, profile and BlocksRuntime.

A small README of the structure would be nice and personally I would finding a rename of the folder compiler_rt to builtin more clear on what is happening, if the object export is simpler in one file.

@LemonBoy What is your take on this?

@jcmoyer jcmoyer marked this pull request as ready for review November 18, 2021 00:41
@LemonBoy
Copy link
Contributor

@LemonBoy What is your take on this?

IMO compiler-rt must follow LLVM's structure (of course with the only exception of our custom stack probes) which in turn (loosely) follows libgcc, having one extra CU is better than shoving everything into a single huge file (ir.cpp flashbacks intensify).

But let's take one step back. You don't want floorl, what you want is to get around this LLVM problem by implementing some custom lowering in stage1/stage2 like it's being done for fmaq.

#7265 arguments are weak, merging everything together doesn't simplify anything for developers nor for the average user.

@andrewrk
Copy link
Member

andrewrk commented Nov 20, 2021

merging everything together doesn't simplify anything for developers nor for the average user.

one thing is simpler than two things. what's the argument for having two things? is it to follow some standard? but already we see that llvm's compiler-rt, libgcc, and msvc's equivalent are subtly different. the only realistic way to make sure everything works is to provide every symbol ourselves, but allow overriding (which is why we use weak_odr linkage).

plus we have zig-specific stuff that requires non-"standard" symbols, such as math with arbitrary sized integers which we will need to lower to a compiler-rt call of a function that llvm's compiler-rt will never have. #1485

also, as this pull request demonstrates, this solves a problem that is currently causing windows to not link, precisely because of the improved simplicity of compiler-rt having more of the, well, compiler's runtime!

what you want is to get around this LLVM problem

Thank you for making that patch upstream btw.

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.

4 participants