zig icon indicating copy to clipboard operation
zig copied to clipboard

LLVM backend cannot lower pointers in packed struct constants

Open alexrp opened this issue 1 year ago • 12 comments

Zig Version

c6ad452

Steps to Reproduce and Observed Behavior

This test regressed with LLVM 19 due to this change: https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179

Our lowering was changed in a27f407 to deal with this change for the majority of cases, but this isn't enough for this particular test. The test was disabled in bdae7d9 (see commit message for failure details).

Expected Behavior

No failure.

alexrp avatar Sep 20 '24 05:09 alexrp

Another similar case that has regressed

test {
    const A = packed struct(u64) {
        c: *allowzero u64,
    };

    const a: A = @bitCast(@as(u64, 0));
    _ = a;
}

The stack trace is slightly different

Stack Trace
/home/dev/git/ziglang/zig/src/Value.zig:172:17: 0xabc93f0 in intFromEnum (zig)
        else => unreachable,
                ^
/home/dev/git/ziglang/zig/src/Value.zig:494:55: 0xb015093 in writeToPackedMemory (zig)
            switch (ip.indexToKey((try val.intFromEnum(ty, pt)).toIntern()).int.storage) {
                                                      ^
/home/dev/git/ziglang/zig/src/Value.zig:568:43: 0xb014d30 in writeToPackedMemory (zig)
            return val.writeToPackedMemory(Type.usize, pt, buffer, bit_offset);
                                          ^
/home/dev/git/ziglang/zig/src/Value.zig:544:50: 0xb0147e3 in writeToPackedMemory (zig)
                try field_val.writeToPackedMemory(field_ty, pt, buffer, bit_offset + bits);
                                                 ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:3976:32: 0xabdd43e in lowerValueToInt (zig)
        val.writeToPackedMemory(ty, pt, std.mem.sliceAsBytes(limbs)[0..bytes], 0) catch unreachable;
                               ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:4353:49: 0xa8f7514 in lowerValue (zig)
                        return o.lowerValueToInt(llvm_int_ty, arg_val);
                                                ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:5125:42: 0xbc251f7 in resolveValue (zig)
        const llvm_val = try o.lowerValue(val.toIntern());
                                         ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:5116:47: 0xbc24f83 in resolveInst (zig)
        const llvm_val = try self.resolveValue((try self.air.value(inst, self.ng.object.pt)).?);
                                              ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:7479:45: 0xbc8ffb4 in airDbgVarVal (zig)
        const operand = try self.resolveInst(pl_op.operand);
                                            ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:5400:54: 0xb7ee635 in genBody (zig)
                .dbg_var_val => try self.airDbgVarVal(inst, false),
                                                     ^
/home/dev/git/ziglang/zig/src/codegen/llvm.zig:1860:19: 0xb7e84c8 in updateFunc (zig)
        fg.genBody(air.getMainBody(), .poi) catch |err| switch (err) {
                  ^
/home/dev/git/ziglang/zig/src/link/Elf.zig:2336:70: 0xbc11145 in updateFunc (zig)
    if (self.llvm_object) |llvm_object| return llvm_object.updateFunc(pt, func_index, air, liveness);
                                                                     ^
/home/dev/git/ziglang/zig/src/link.zig:725:82: 0xb7f4ba5 in updateFunc (zig)
                return @as(*tag.Type(), @fieldParentPtr("base", base)).updateFunc(pt, func_index, air, liveness);
                                                                                 ^
/home/dev/git/ziglang/zig/src/Zcu/PerThread.zig:917:22: 0xb3c8f2c in linkerUpdateFunc (zig)
        lf.updateFunc(pt, func_index, air, liveness) catch |err| switch (err) {
                     ^
/home/dev/git/ziglang/zig/src/link.zig:1548:32: 0xaf54741 in doTask (zig)
            pt.linkerUpdateFunc(func.func, func.air) catch |err| switch (err) {
                               ^
/home/dev/git/ziglang/zig/src/Compilation.zig:3869:20: 0xab65b19 in dispatchCodegenTask (zig)
        link.doTask(comp, tid, link_task);
                   ^
/home/dev/git/ziglang/zig/src/Compilation.zig:3646:37: 0xa8d8a9e in processOneJob (zig)
            comp.dispatchCodegenTask(tid, .{ .codegen_func = func });
                                    ^
/home/dev/git/ziglang/zig/src/Compilation.zig:3603:30: 0xa6b1b6c in performAllTheWorkInner (zig)
            try processOneJob(@intFromEnum(Zcu.PerThread.Id.main), comp, job, main_progress_node);
                             ^
/home/dev/git/ziglang/zig/src/Compilation.zig:3463:36: 0xa516c80 in performAllTheWork (zig)
    try comp.performAllTheWorkInner(main_progress_node);
                                   ^
/home/dev/git/ziglang/zig/src/Compilation.zig:2232:31: 0xa512291 in update (zig)
    try comp.performAllTheWork(main_progress_node);
                              ^
/home/dev/git/ziglang/zig/src/main.zig:4414:20: 0xa55482d in updateModule (zig)
    try comp.update(prog_node);
                   ^
/home/dev/git/ziglang/zig/src/main.zig:3606:21: 0xa5bf664 in buildOutputType (zig)
        updateModule(comp, color, root_prog_node) catch |err| switch (err) {
                    ^
/home/dev/git/ziglang/zig/src/main.zig:268:31: 0xa41b884 in mainArgs (zig)
        return buildOutputType(gpa, arena, args, .zig_test);
                              ^
/home/dev/git/ziglang/zig/src/main.zig:200:20: 0xa418685 in main (zig)
    return mainArgs(gpa, arena, args);
                   ^
/home/dev/git/ziglang/zig/lib/std/start.zig:617:37: 0xa41818e in main (zig)
            const result = root.main() catch |err| {

gooncreeper avatar Nov 03 '24 20:11 gooncreeper

Note that in ReleaseFast builds of Zig this miscomps rather than crashes (#24166) -- which makes perfect sense, but is nonetheless unfortunate. It would probably be a good idea to at least make the backend emit an error.

mlugg avatar Jun 13 '25 17:06 mlugg

I have an Intel-based mac that I haven't been using. So I tried itlso on zig-x86_64-macos-0.15.0-dev.822+dd75e7bcb and it works properly without haveing to do anything special (like no-llvm switch). But not on aarch64 same version. It may work on earlier x86-64 builds, but the previous one I had was 0.12, and I didn't try the intervening ones.

dvmason avatar Jun 15 '25 23:06 dvmason

Recent builds of master use the self-hosted (-fno-llvm) backend by default on x86_64 for Linux and macOS, so the case worked for you because it didn't actually use LLVM.

mlugg avatar Jun 15 '25 23:06 mlugg

Thanks, @mlugg for confirming my guess as to why it worked.

Sigh... and now I get:

 unable to perform tail call: compiler backend 'stage2_x86_64' does not support tail calls on target architecture 'x86_64' with the selected CPU feature flags

I presume the .native value in zig targets describes the current features, but I don't know which features might prevent tail-call optimization (which is integral to my system) or how to disable said feature.

My native target looks like:

    .native = .{
        .triple = "x86_64-macos.15.5...15.5-none",
        .cpu = .{
            .arch = "x86_64",
            .name = "skylake",
            .features = .{
                "64bit",
                "adx",
                "aes",
                "allow_light_256_bit",
                "avx",
                "avx2",
                "bmi",
                "bmi2",
                "clflushopt",
                "cmov",
                "crc32",
                "cx16",
                "cx8",
                "ermsb",
                "f16c",
                "false_deps_popcnt",
                "fast_15bytenop",
                "fast_gather",
                "fast_scalar_fsqrt",
                "fast_shld_rotate",
                "fast_variable_crosslane_shuffle",
                "fast_variable_perlane_shuffle",
                "fast_vector_fsqrt",
                "fma",
                "fsgsbase",
                "fxsr",
                "idivq_to_divl",
                "invpcid",
                "lzcnt",
                "macrofusion",
                "mmx",
                "movbe",
                "no_bypass_delay_blend",
                "no_bypass_delay_mov",
                "no_bypass_delay_shuffle",
                "nopl",
                "pclmul",
                "popcnt",
                "prfchw",
                "rdrnd",
                "rdseed",
                "sahf",
                "sgx",
                "slow_3ops_lea",
                "smap",
                "smep",
                "sse",
                "sse2",
                "sse3",
                "sse4_1",
                "sse4_2",
                "ssse3",
                "vzeroupper",
                "x87",
                "xsave",
                "xsavec",
                "xsaveopt",
                "xsaves",
            },
        },
        .os = "macos",
        .abi = "none",
    }

dvmason avatar Jun 16 '25 13:06 dvmason

#24044

alexrp avatar Jun 16 '25 13:06 alexrp

(...meaning right now, you unfortunately have to pick your poison -- either use the self-hosted backend and lose tail calls, or use the LLVM backend and lose out on constant packed structs containing pointers.)

mlugg avatar Jun 16 '25 13:06 mlugg

Tail calls are an absolute requirement for this project. So I'm not a happy camper (I've had to change (uglify) 100s of lines of code to move things from comptime to runtime - and one of the reasons I so love(d) zig is that I can do so much at compile time - or at least could). I do hope someone can fix this soon. (preferably with LLVM as I want this to be multi-targt - at least x86_64 and AArch64 and eventually wasm).

Thanks to all of you who work on the compiler... it's awesome (see... I've almost forgotten the pain already :-)

dvmason avatar Jun 20 '25 03:06 dvmason

This is going to be resolved with the implementation of #24657. We're disallowing pointers in packed structs largely because LLVM is reflecting reality here: binary formats do not support relocation types which could make this work.

mlugg avatar Nov 25 '25 13:11 mlugg

What about when an @intFromPtr is introduced?

const std = @import("std");

var global_var: u32 = 10;

const PackedStruct = packed struct(u66) {
    trash: u1 = 1,
    addr: usize,
    more_trash: u1 = 1,
};

test "foo" {
    const instance: PackedStruct = .{ .addr = @intFromPtr(&global_var) };

    var copy: PackedStruct = undefined;
    copy = instance;
    try std.testing.expectEqual(@intFromPtr(&global_var), copy.addr);
}

Seems to work fine today:

andy@bark ~/d/z/build-release (master)> stage4/bin/zig test test.zig -fno-llvm
All 1 tests passed.
andy@bark ~/d/z/build-release (master)> stage4/bin/zig test test.zig -fllvm
All 1 tests passed.

Not sure why tho because it's subject to the same relocation problem as having a pointer in there, unless there's some logic (in Sema?) that figures out to mark instance as a runtime-known value.

test.zig:12:56: error: unable to evaluate comptime expression
    const instance: PackedStruct = comptime .{ .addr = @intFromPtr(&global_var) };
                                                       ^~~~~~~~~~~~~~~~~~~~~~~~
test.zig:12:68: note: operation is runtime due to this operand
    const instance: PackedStruct = comptime .{ .addr = @intFromPtr(&global_var) };
                                                                   ^~~~~~~~~~~
test.zig:12:36: note: 'comptime' keyword forces comptime evaluation
    const instance: PackedStruct = comptime .{ .addr = @intFromPtr(&global_var) };
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

🤔 when did pointers to globals become non-comptime values?

andrewrk avatar Nov 25 '25 13:11 andrewrk

@intFromPtr on a comptime-known pointer is usually runtime-known (the exception is if the pointer came from @ptrFromInt). This makes sense because while we conceptually know the pointer (i.e. we know what it's referring to), the address isn't comptime-known; it depends on linking and runtime relocation.

mlugg avatar Nov 25 '25 13:11 mlugg

Ah that's right, it's the @intFromPtr that does it. Thanks :)

andrewrk avatar Nov 25 '25 13:11 andrewrk