Packed struct padding bits can cause incorrect values to be observed #13480
Comments
|
@SpexGuy poked at the source for a while and simplified it a bit further; the modified repro case is attached. The repro steps are basically the same.
|
|
Cannot reproduce on latest master. |
|
I just tested on |
|
I think both cdwfs and I are using Windows, it's possible that the bug only repros on that system. (though that would be somewhat strange since the program shouldn't use any OS APIs aside from VirtualAlloc). Given the instability of the bug, it's also possible that recent compiler changes have jostled things enough to mask it without fixing it. |
SpexGuy
added
the
miscompilation
|
Can't reproduce on
Can you try using a fixed buffer allocator? |
|
@Vexu To clarify, when you say "can't reproduce", do you mean you don't see the assert firing at all? Or that it's firing consistently, whether any other changes are made to the file?
and this does indeed seem to stop the assert from firing. But on the other hand, on my machine, so does defining an unrelated I feel like the appropriate next step is to look at the generated code for the |
|
This is really freaky. I just reproed the inconsistency you were mentioning. This is all the same executable: Console log, collapsedAFAIK the only things that could cause randomness in this program are
So somehow the high bits of a pointer must be influencing the result. I've attached the executable in case anyone wants to mess with it, I wonder if it repros under wine. |
|
Figured it out. The problem is the top bit of the u7. Here's the relevant part of the disassembly: ; var mask_a = std.bit_set.IntegerBitSet(7){
; .mask = display.digit_masks[digit_index_for_numeral[7]].mask ^ mask_cf.mask,
; };
; ...
mov cl, byte ptr [rsp+429h]
and cl, 80h
or al, cl
mov byte ptr [rsp+429h], al
; ...
; assert(mask_a.mask == 0b0001000);
mov al, byte ptr [rsp+429h]
sub al, 8
sete cl
call day08!assert (7ff7942b38c0)When the u7 is written to memory, the write preserves the top bit for some reason. Then later when it's read, the top bit isn't masked off. So it gets whatever happened to be on the stack in the top bit, which I guess in this particular program happens to be a pointer, and memory space randomization causes it to sometimes be 1 and sometimes 0. I'll see if I can come up with a more consistent repro. |
|
Here's the smallest repro I can find. It seems to require indexing an array of packed structs with padding. const BitSet = packed struct {
mask: u7,
};
const Display = struct {
// This array has to be large. Using separate padding
// plus a one item array doesn't work.
digit_masks: [10]BitSet = undefined,
};
fn testPart2() void {
var display = Display{};
display.digit_masks[9] = .{ .mask = 0b0001000 };
smash();
stackUp(display);
}
fn smash() void {
var stack: [0x500]u8 = undefined;
@memset(&stack, 0xFF, stack.len);
}
fn stackUp(display: Display) void {
var space: [0x100]u8 = undefined;
part2(display);
_ = space;
}
fn part2(display: Display) void {
// var mask_a = display.digit_masks[9] works without problems
var mask_a = BitSet{
.mask = display.digit_masks[9].mask,
};
if (@popCount(mask_a.mask) != 1) unreachable; // this passes
if (@ctz(mask_a.mask) != 3) unreachable; // this passes
if (mask_a.mask != 0b0001000) unreachable; // but this fails
}
pub fn main() void {
testPart2();
}Repros on godbolt: https://zig.godbolt.org/z/9sY3WjdWs Edit: It only repros sometimes on godbolt. If it doesn't work, reload the page a few times. |
SpexGuy
changed the title
|
|
Initializing packed structs with result location is problematic since storing to a packed struct field pointer involves loading the field first which can cause loading uninitialized memory unless specifically zeroed first. Since packed structs are integer backed now using aggregate init makes more sense and avoids this issue. Closes ziglang#13480
Initializing packed structs with result location is problematic since storing to a packed struct field pointer involves loading the field first which can cause loading uninitialized memory unless specifically zeroed first. Since packed structs are integer backed now using aggregate init makes more sense and avoids this issue. Closes ziglang#13480
|
Not sure if this is the same issue @Vexu but I just ran into this: const std = @import("std");
const Node = packed struct {
const @"c/d" = packed struct(u40) {
c: u32 = 0,
d: u8 = 0,
};
const @"a/b" = packed struct(u40) {
a: u32 = 0,
b: u8 = 0,
};
@"a/b": @"a/b" = .{},
@"c/d": @"c/d" = .{},
pub fn init(d: u8) Node {
return Node{
// These fields work okay
.@"a/b" = .{ .a = d, .b = d + 1 },
// Writing to either of these fields will not work:
.@"c/d" = .{ .c = d, .d = d + 1 },
};
}
};
pub fn main() !void {
const node = Node.init(9);
std.debug.print("{any}\n", .{node});
} |
|
Closing as a duplicate of the newer #14200 because I just so happened to fix that one first. @SpexGuy's explanation perfectly matches the issue I fixed and that reduction crashes without my fix and succeeds with my fix. @Validark |
Zig Version
0.10.0
Steps to Reproduce and Observed Behavior
I ran into this issue while converting my 2021 Advent of Code solutions from Zig 0.9.1 to 0.10.0. I originally reported the problem on Discord; see the thread for additional details.
Build the attached
day08.zigsource usingzig build-exe day08.zig, and run the resultingday08.exe. The assert on line 108 will fire, indicating that the loop directly above the assert did not run correctly.Now make nearly any change to the surrounding code, such as uncommenting the
load_bearing_booldefinition directly above the loop on lines 95-96. Build and run again, and note that the loop now runs correctly; the assert does not fire.Note that if
-fstage1is passed at build time to opt into the Bootstrap Compiler, the original unmodified code works as expected.day08.zip
Expected Behavior
The assert on line 108 should not trigger, regardless of whether unrelated/unused variables are defined, or
print()is called to inspect variable values, etc.The text was updated successfully, but these errors were encountered: