Skip to content

packed struct miscompilation in ReleaseFast #15715

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
Jarred-Sumner opened this issue May 15, 2023 · 1 comment
Closed

packed struct miscompilation in ReleaseFast #15715

Jarred-Sumner opened this issue May 15, 2023 · 1 comment

Comments

@Jarred-Sumner
Copy link
Contributor

Zig Version

0.11.0-dev.2777+b95cdf0ae

Steps to Reproduce and Observed Behavior

When the following structs are packed, ReleaseFast builds report a different value in some cases.

pub const Loc = packed struct {
    start: i32 = -1,
};

pub const Range = packed struct {
    loc: Loc = .{},
    len: i32 = 0,
};

In Bun, this causes a JavaScript parser bug in code like this:

// abcdefghijklmnopqrstuvwxyz abcdefghijklmnopqrstuvwxyz
export default () => {};

It produces an error like the following:

> bun /tmp/input.js

error: Expected scope (src.js_ast.Scope.Kind.function_args, 8) in /private/tmp/input.js, found scope (src.js_ast.Scope.Kind.function_args, 72)

export default () => {};
                        ^
/private/tmp/input.js:2:25 82

The correct number is 72, the incorrect number is 8.

Why I suspect a bug in Zig or LLVM and not a bug in Bun

When the only difference between the code is removing packed from the above two structs, the error no longer occurs. The error does not occur in debug builds. I have not tested if it occurs in ReleaseSafe or ReleaseSmall.

I've attached macOS aarch64 executables of Bun with and without packed, and no other code changes.

bun-miscompilation.zip
bun-not-packed.zip

Expected Behavior

packed or not packed structs given the same values should produce the same results in ReleaseFast and Debug

@Jarred-Sumner Jarred-Sumner added the bug Observed behavior contradicts documented or intended behavior label May 15, 2023
Jarred-Sumner added a commit to oven-sh/bun that referenced this issue May 15, 2023
@andrewrk
Copy link
Member

Regarding oven-sh/bun@9acf854, I'm not sure why you would use a packed struct for these types. For Loc it does not change the size or alignment, it only forces the compiler to use a well-defined memory layout, causing it to opt-out of potential future safety checks such as #2414. For Range, making it packed actually increases the alignment from 4 bytes to 8 bytes because now it's being stored as a 64-bit integer instead of two 32-bit integers.

That alignment difference could account for some undefined behavior elsewhere in your application being hidden by changing whether or not this struct is packed. Have you ensured that a debug build runs Valgrind-clean for this test case?

I'm sorry that I cannot offer you a "zig-reduce" tool yet to help produce a small test case confirming this as a bug/miscompilation here on the Zig issue tracker. However, I do consider this to be unconfirmed so far.

Here is what I suggest for the path forward:

  1. wait for zig to solve each issue in this search query
  2. then you can try this test case again, and maybe it is fixed.
  3. if the issue persists, you will need to reduce it and provide the reduction here. then I'll re-open this issue.

@andrewrk andrewrk closed this as not planned Won't fix, can't repro, duplicate, stale Jul 23, 2023
@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Jul 23, 2023
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

No branches or pull requests

2 participants