Skip to content

@"identifier" syntax accidentally exposes all LLVM intrinsics #2291

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

Open
andrewrk opened this issue Apr 16, 2019 · 3 comments
Open

@"identifier" syntax accidentally exposes all LLVM intrinsics #2291

andrewrk opened this issue Apr 16, 2019 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior
Milestone

Comments

@andrewrk
Copy link
Member

@shritesh discovered that if you do something like this:

extern fn @"llvm.wasm.memory.size.i32"(u32) u32;
extern fn @"llvm.wasm.memory.grow.i32"(u32, u32) i32;

This actually exposes the underlying LLVM intrinsics. #2286 takes advantage of this to provide the global system allocator for the WebAssembly target.

It's definitely problematic to have this behavior and this bug will get fixed, but we can't just pull the rug out. We need a solution for this use case.

Another use case @shritesh pointed out is the NVPTX target: https://llvm.org/docs/NVPTXUsage.html#reading-ptx-special-registers

In the same way WebAssembly needs these intrinsics, NVPTX needs all the ones in the list linked above.

So far, none of the builtin functions of Zig are target-specific. We could start adding target-specific builtins, or we could take a different approach. The current way to do target-specific instructions is inline assembly. We could teach zig about the (memory.grow) and (memory.shrink) wasm instructions and rely on asm. Zig would detect this pattern and still emit LLVM intrinsics.

@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Apr 16, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Apr 16, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 11, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Feb 10, 2020
@kubkon
Copy link
Member

kubkon commented Jun 1, 2020

@andrewrk any thoughts on this beyond what's already written up in the issue description? I've actually implemented the two mentioned intrinsics as target-specific builtins (needed for something else) before stumbling upon this issue (yeah, I know...).

I like the idea of having inline (w)asm, but I'm not quite sure how complicated it would be to accommodate instructions such as pushing/popping off the stack intermingled with the Zig code. I could definitely have a look into that though, perhaps initially focusing only on memory.size and memory.grow instructions, anything else generating a hard compile error.

Oh, and just for the record, branch with the intrinsics as target-specific builtins.

@daurnimator
Copy link
Contributor

I think some of the current builtins (like @clz) should also move to whatever mechanism we select here.

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 30, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Nov 6, 2020
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 May 19, 2021
ifreund added a commit to tigerbeetle/tigerbeetle that referenced this issue Nov 26, 2021
Currently the stage1 zig compiler allows access to all LLVM instrinsics
if you know what you are doing: ziglang/zig#2291

This is however a bug, and we should use the @prefetch() builtin when
availible ziglang/zig#3600.
@ghost
Copy link

ghost commented Jan 25, 2022

My immediate though upon seeing the names is to have a std.llvm module in LLVM builds, that just binds all the intrinsics. So for instance you’d have std.llvm.wasm.memory.size.i32, with no extern in the program itself. It’s the laziest solution, but it covers all current cases without necessarily counting as a bug.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants