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

Multiple stores generated for writes to volatile packed struct(u32) #17119

Closed
Lambosaurus opened this issue Sep 11, 2023 · 1 comment · Fixed by #18230
Closed

Multiple stores generated for writes to volatile packed struct(u32) #17119

Lambosaurus opened this issue Sep 11, 2023 · 1 comment · Fixed by #18230
Labels
backend-llvm The LLVM backend outputs an LLVM IR Module. bug Observed behavior contradicts documented or intended behavior miscompilation The compiler reports success but produces semantically incorrect code.
Milestone

Comments

@Lambosaurus
Copy link

Zig Version

0.11.0

Steps to Reproduce and Observed Behavior

When compiling with -O ReleaseSmall -target thumb-freestanding-none, strange stores are generated when writing to a volatile packed struct. This behavior seems to be related to -fstrip somehow, because the expected output is correctly generated with -fno-strip or -O ReleaseFast

The output can be seen in godbolt here.

Given the following source:

pub const Peripheral = packed struct {
    register: packed struct(u32) {
        bit0: u1 = 0,
        bit1: u1 = 0,
        bit2: u1 = 0,
        bit3: u1 = 0,
        bit4: u1 = 0,
        bit5: u2 = 0,
        bit7: u1 = 0,
        bit8: u24 = 0,
    }
};

pub const peripheral: *volatile Peripheral = @ptrFromInt(0x40021000);

export fn store_by_struct() void {
    peripheral.register = .{ .bit1 = 1 };
}

The resulting assembly is:

store_by_struct:
        ldr     r0, .LCPI0_0
        ldr     r1, [r0]
        orr     r1, r1, #2
        str     r1, [r0]
        ldr     r1, [r0]
        bic     r1, r1, #1
        str     r1, [r0]
        ldr     r1, [r0]
        bic     r1, r1, #4
        str     r1, [r0]
        ldr     r1, [r0]
        bic     r1, r1, #8
        str     r1, [r0]
        ldr     r1, [r0]
        bic     r1, r1, #16
        str     r1, [r0]
        ldr     r1, [r0]
        bic     r1, r1, #96
        str     r1, [r0]
        ldr     r1, [r0]
        bic     r1, r1, #128
        str     r1, [r0]
        ldr.w   r1, [r0, #1]
        uxtb    r1, r1
        str.w   r1, [r0, #1]
        bx      lr
.LCPI0_0:
        .long   1073876992

This seems to be generating a bit clear and a store for each field in the struct.

Expected Behavior

When compiling with -fno-strip the following assembly is generated:

store_by_struct:
        ldr     r0, .LCPI0_0
        movs    r1, #2
        str     r1, [r0]
        bx      lr
.LCPI0_0:
        .long   1073876992

This is the expected output.

@Lambosaurus Lambosaurus added the bug Observed behavior contradicts documented or intended behavior label Sep 11, 2023
@Vexu Vexu added the miscompilation The compiler reports success but produces semantically incorrect code. label Sep 11, 2023
@Vexu Vexu added this to the 0.13.0 milestone Sep 11, 2023
@Vexu Vexu added the backend-llvm The LLVM backend outputs an LLVM IR Module. label Sep 11, 2023
@cfillion
Copy link
Contributor

cfillion commented Dec 8, 2023

I'm running into a similar issue, however here with 0.11.0 neither -fstrip or -O ReleaseSmall were required to duplicate. The bad assembly was generated even in Debug or ReleaseFast builds.

Starting with commit 8b91611 onwards the issue is only triggered when -fstrip or -O ReleaseSmall is specified.

var value: u32 = 42;

const register: *volatile packed union {
  fields: packed struct(u32) {
    foo: bool = false,
    _1:  u3   = 0,
    _2:  u4   = 0,
    bar: bool = false,
    _3:  u3   = 0,
    baz: bool = false,
    _6:  u19  = 0,
  },
  value: u32,
} = @ptrCast(&value);

export fn _start() void {
  register.fields = .{ .foo = true, .bar = true };
}
$ git switch -d 8b916117~1
HEAD is now at b835fd90ce std.http.Server: use correct header for Transfer-Encoding
$ rm -r build zig-cache t t.o; cmake -B build -G Ninja && cmake --build build
$ build/stage3/bin/zig build-exe t.zig -target thumb-freestanding && arm-none-eabi-objdump -d t
00020104 <_start>:
   20104:	f240 114c 	movw	r1, #332	@ 0x14c
   20108:	f2c0 0103 	movt	r1, #3
   2010c:	6808      	ldr	r0, [r1, #0]
   2010e:	f040 0001 	orr.w	r0, r0, #1
   20112:	6008      	str	r0, [r1, #0]
   20114:	f8d1 0001 	ldr.w	r0, [r1, #1]
   20118:	f440 7080 	orr.w	r0, r0, #256	@ 0x100
   2011c:	f8c1 0001 	str.w	r0, [r1, #1]
   20120:	6808      	ldr	r0, [r1, #0]
   20122:	f020 000e 	bic.w	r0, r0, #14
   20126:	6008      	str	r0, [r1, #0]
   20128:	6808      	ldr	r0, [r1, #0]
   2012a:	f020 00f0 	bic.w	r0, r0, #240	@ 0xf0
   2012e:	6008      	str	r0, [r1, #0]
   20130:	6808      	ldr	r0, [r1, #0]
   20132:	f420 6060 	bic.w	r0, r0, #3584	@ 0xe00
   20136:	6008      	str	r0, [r1, #0]
   20138:	6808      	ldr	r0, [r1, #0]
   2013a:	f420 5080 	bic.w	r0, r0, #4096	@ 0x1000
   2013e:	6008      	str	r0, [r1, #0]
   20140:	6808      	ldr	r0, [r1, #0]
   20142:	f36f 305f 	bfc	r0, #13, #19
   20146:	6008      	str	r0, [r1, #0]
   20148:	4770      	bx	lr
$ git switch -d 8b916117
Previous HEAD position was b835fd90ce std.http.Server: use correct header for Transfer-Encoding
HEAD is now at 8b9161179d Sema: avoid deleting runtime side-effects in comptime initializers
$ rm -r build zig-cache t t.o; cmake -B build -G Ninja && cmake --build build
$ build/stage3/bin/zig build-exe t.zig -target thumb-freestanding && arm-none-eabi-objdump -d t
00020104 <_start>:
   20104:	f240 1114 	movw	r1, #276	@ 0x114
   20108:	f2c0 0103 	movt	r1, #3
   2010c:	f240 1001 	movw	r0, #257	@ 0x101
   20110:	6008      	str	r0, [r1, #0]
   20112:	4770      	bx	lr

Adding -fstrip reverts the output back to how it was before that commit.

Using a temporary variable does not trigger the issue:

export fn _start() void {
  const fields = .{ .foo = true, .bar = true };
  register.fields = fields;
}

cfillion added a commit to cfillion/zig that referenced this issue Dec 8, 2023
… with `-fstrip`

The first instruction in the block was never checked resulting in `struct_is_comptime` being incorrectly cleared if there are no instructions before the first field of the comptime struct.

Fixes ziglang#17119
cfillion added a commit to cfillion/zig that referenced this issue Dec 8, 2023
…th `-fstrip`

The first instruction in the block was never checked resulting in `struct_is_comptime` being incorrectly cleared if there are no instructions before the first field of the comptime struct.

Fixes ziglang#17119
Vexu pushed a commit that referenced this issue Dec 28, 2023
…th `-fstrip`

The first instruction in the block was never checked resulting in `struct_is_comptime` being incorrectly cleared if there are no instructions before the first field of the comptime struct.

Fixes #17119
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend-llvm The LLVM backend outputs an LLVM IR Module. bug Observed behavior contradicts documented or intended behavior miscompilation The compiler reports success but produces semantically incorrect code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants