Skip to content
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

Regression: Stack overflow when initializing large struct with initializer syntax. #17289

Open
IntegratedQuantum opened this issue Sep 26, 2023 · 5 comments
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@IntegratedQuantum
Copy link
Contributor

Zig Version

0.12.0-dev.596+2adb932ad (worked in 0.12.0-dev.494+a8d2ed806)

Steps to Reproduce and Observed Behavior

const std = @import("std");

const World = struct {
	name: []const u8,
	placeHolderOtherStructFields: [33693888]u8 = undefined,

	pub fn init(self: *World, name: []const u8) void {
		self.* = World {
			.name = name,
			// ...
		};
	}
};

var world: World = undefined;
pub fn main() !void {
	world.init("abcde");
}

Output:

$ zig build-exe test.zig
$ gdb ./test
...
(gdb) run
Starting program: test 

Program received signal SIGSEGV, Segmentation fault.
0x00000000022b4e54 in __zig_probe_stack () at /home/mint/Downloads/zig/lib/compiler_rt/stack_probe.zig:53
53	            asm volatile (

Expected Behavior

It should not take excessive amounts of stack memory to initialize big structs using the initializer syntax.

Now you might just think that I am insane for using such excessively big structs and maybe I am, but I like to avoid unnecessary indirection where possible, so when I know the size of an array upfront, I just put it into the struct directly. In this case for example it was a bunch of 2¹⁶=65536 sized arrays of 3d vectors.

The workaround here would be to manually initialize the required fields, but that is tedious and error prone, since I need to manually take care about all the default field values.

@IntegratedQuantum IntegratedQuantum added the bug Observed behavior contradicts documented or intended behavior label Sep 26, 2023
IntegratedQuantum added a commit to PixelGuys/Cubyz that referenced this issue Sep 26, 2023
I also had to add some workarounds for a new regression in the zig compiler: ziglang/zig#17289
@xdBronch
Copy link
Contributor

xdBronch commented Sep 26, 2023

does the problem still occur if you change it to self.* = .{? edit: tested myself and this does fix it
cc @mlugg is this due to T{} not doing RLS anymore?

@IntegratedQuantum
Copy link
Contributor Author

Thanks @xdBronch that's the perfect solution.
I expected .{} and T{} would compile to the same thing, but apparently they don't.

@mlugg
Copy link
Member

mlugg commented Sep 26, 2023

Yeah, sorry, this is #17194. This is one unfortunate side effect of that proposal - I ran into it myself in the standard library tests, where I had to up the WASI stack size. #1639 would be desirable here.

In general, .{ ... } is a preferred syntax over S{ ... }. There's an open proposal to eliminate the latter, and it's really only necessary for Generic Things (tm) where the type isn't otherwise known. As of the implementation of #17194, only the former syntax performs RLS (to simplify the language), and you're implicitly relying on RLS here to not overflow the stack.

@Jarred-Sumner
Copy link
Contributor

Yeah, sorry, this is #17194. This is one unfortunate side effect of that proposal - I ran into it myself in the standard library tests, where I had to up the WASI stack size. #1639 would be desirable here.

In general, .{ ... } is a preferred syntax over S{ ... }. There's an open proposal to eliminate the latter, and it's really only necessary for Generic Things (tm) where the type isn't otherwise known. As of the implementation of #17194, only the former syntax performs RLS (to simplify the language), and you're implicitly relying on RLS here to not overflow the stack.

Why is it worth increasing the risk of stack overflow over what used to be a stylistic choice?

In Bun's codebase, we often use the explicit form S{ ... } because editor autocomplete/tooling works better with an explicit type. We assumed it would have no runtime difference when the resolved type is the same.

@mlugg
Copy link
Member

mlugg commented Sep 29, 2023

@Jarred-Sumner

Why is it worth increasing the risk of stack overflow over what used to be a stylistic choice?

Please read #17194 to understand the motivation. This change was not without reason: it simplifies the language and compiler design, and fixes several miscompilations.

It's not unlikely that #5038 is eventually accepted, in which case T{} is no longer a valid syntax. If ZLS can't handle .{} as well as alternatives then that's certainly unfortunate, but it's an issue that should be fixed in ZLS; not one the language should work around. I'm confident that it's possible for tooling like ZLS to provide high-quality completions in the presence of RLS / type inference.

I'm happy to personally collaborate with downstream (cc @Techatrix, @SuperAuguste) if this is an area I can potentially help to improve in the short-term. In addition, with the active work on incremental compilation, we'll hopefully be able to start hacking away at #615 soon to improve the ZLS experience across the board.

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

No branches or pull requests

4 participants