Skip to content

Conversation

jacobly0
Copy link
Member

@jacobly0 jacobly0 commented Oct 31, 2023

In Zig, loads and stores affect @sizeOf(T) bytes of memory. The old implementations of these functions were pointer casting a buffer of @divExact(@bitSizeOf(T), 8) bytes which is not guaranteed to be large enough to perform a load or store of T directly. This only ever worked because llvm has weird store size rules, and in fact, after this change, some non-llvm backends mysteriously started passing these tests.

By making use of inline, the complicated nest of variant functions could all be reduced to four functions without compromising functionality. After removing some incorrect uses of two of these, there were no references to them remaining, so I deleted them too.


Migration guide

These apply to both the functions in std.mem and the methods on std.io.GenericReader, std.io.AnyReader, and std.io.Writer.

  • readInt(...)readInt(...)
  • readIntLittle(...)readInt(..., .little)
  • readIntBig(...)std.mem.readInt(..., .big)
  • readIntNative(...)std.mem.readInt(..., @import("builtin").cpu.arch.endian()) or just @as(T, @bitCast(buffer.*)) for the std.mem version
  • readIntForeign(...)@byteSwap(std.mem.readInt(..., @import("builtin").cpu.arch.endian())) or just @byteSwap(@as(T, @bitCast(buffer.*))) for the std.mem version
  • writeInt(...)std.mem.writeInt(...)
  • writeIntLittle(...)std.mem.writeInt(..., .little)
  • writeIntBig(...)std.mem.writeInt(..., .big)
  • writeIntNative(...)std.mem.writeInt(..., @import("builtin").cpu.arch.endian()) or just buffer.* = @bitCast(value) for the std.mem version
  • writeIntForeign(...)std.mem.writeInt(..., @byteSwap(value), @import("builtin").cpu.arch.endian()) or just buffer.* = @bitCast(@byteSwap(value)) for the std.mem version

@Luukdegram unfortunately, this new pattern regressed the self-hosted wasm backend, so just letting you know that there are three new .stage2_wasm checks in lib/test_runner.zig, test/behavior/bugs/1851.zig, and test/behavior/struct.zig.

@jacobly0 jacobly0 requested a review from squeek502 as a code owner October 31, 2023 10:31
@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels Oct 31, 2023
@andrewrk andrewrk enabled auto-merge October 31, 2023 17:29
@andrewrk andrewrk disabled auto-merge October 31, 2023 19:17
@andrewrk andrewrk enabled auto-merge October 31, 2023 20:04
@andrewrk
Copy link
Member

These warnings are new when bootstrapping:

[10/13] Building C object CMakeFiles/zig2.dir/zig2.c.o
/home/andy/Downloads/zig/build-release/zig2.c: In function ‘value_Value_readFromMemory__22826’:
/home/andy/Downloads/zig/build-release/zig2.c:1503723: warning: ‘memcpy’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
1503723 |      memcpy(&t47, &t46, sizeof(zig_u128));
        | 
/home/andy/Downloads/zig/build-release/zig2.c:1503582: note: source object ‘t46’ of size 10
1503582 |  uint8_t t46[10];
        | 
/home/andy/Downloads/zig/build-release/zig2.c: In function ‘mem_readPackedIntLittle__anon_233795__233795’:
/home/andy/Downloads/zig/build-release/zig2.c:2732095: warning: ‘memcpy’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
2732095 |  memcpy(&t11, &t10, sizeof(zig_u128));
        | 
/home/andy/Downloads/zig/build-release/zig2.c:2732072: note: source object ‘t10’ of size 10
2732072 |  uint8_t t10[10];
        | 
/home/andy/Downloads/zig/build-release/zig2.c: In function ‘mem_readPackedIntBig__anon_233796__233796’:
/home/andy/Downloads/zig/build-release/zig2.c:2732169: warning: ‘memcpy’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
2732169 |  memcpy(&t13, &t12, sizeof(zig_u128));
        | 
/home/andy/Downloads/zig/build-release/zig2.c:2732131: note: source object ‘t12’ of size 10
2732131 |  uint8_t t12[10];
        | 
[11/13] Building stage3

Copy link
Member

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to resinator look correct.

@andrewrk
Copy link
Member

andrewrk commented Nov 1, 2023

D:\a\zig\zig\lib\zig.h(2183): warning C4244: 'function': conversion from 'uint16_t' to 'uint8_t', possible loss of data
...
D:\a\zig\zig\lib\zig.h(3366): warning C4391: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect return type for intrinsic function, expected 'double'
D:\a\zig\zig\lib\zig.h(3366): warning C4392: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
test-x86_64-windows-msvc.c(53486): warning C4133: 'function': incompatible types - from 'A__9111 *' to 'A__9107 *'
test-x86_64-windows-msvc.c(53488): warning C4133: 'function': incompatible types - from 'A__9112 *' to 'A__9107 *'
test-x86_64-windows-msvc.c(65402): warning C4047: '=': 'void (__cdecl *)(void)' differs in levels of indirection from 'uint8_t (*)[4]'
test-x86_64-windows-msvc.c(123940): error C2167: '__fmax': too many actual parameters for intrinsic function
test-x86_64-windows-msvc.c(124357): error C2167: '__fmax': too many actual parameters for intrinsic function
test-x86_64-windows-msvc.c(263186): warning C4090: '=': different 'const' qualifiers
D:\a\zig\zig\lib\zig.h(2183): warning C4244: 'function': conversion from 'uint16_t' to 'uint8_t', possible loss of data
...
D:\a\zig\zig\lib\zig.h(3366): warning C4391: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect return type for intrinsic function, expected 'double'
D:\a\zig\zig\lib\zig.h(3366): warning C4392: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
compiler_rt-x86_64-windows-msvc.c(490): warning C4392: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
compiler_rt-x86_64-windows-msvc.c(19292): warning C4392: 'zig_f80 __fmax(const zig_f80,const zig_f80,const zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
compiler_rt-x86_64-windows-msvc.c(19292): error C2169: '__fmax': intrinsic function, cannot be defined

huh. I don't understand how that passed the CI before, or why it's passing on x86_64-windows-debug.

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 1, 2023

These warnings are new when bootstrapping:

Have a fix, but waiting for a CI run to complete.

huh. I don't understand how that passed the CI before, or why it's passing on x86_64-windows-debug.

It's obvious what happened (clash between f-max and fma-x), but like you said, who knows why it happened on this run in particular. I can only assume either a bad roll gave us a different image (different compiler version?) somehow, or an msvc bugs means the error is not consistent.

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 1, 2023

Well, not related to the changes in this PR at least: https://github.com/ziglang/zig/actions/runs/6710563026/job/18236031654

jacobly0 and others added 5 commits October 31, 2023 21:37
Use inline to vastly simplify the exposed API.  This allows a
comptime-known endian parameter to be propogated, making extra functions
for a specific endianness completely unnecessary.
After deleting all potentially incorrect usages, there were no more
remaining.
Let's take this breaking change opportunity to fix the style of this
enum.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants