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

change semantics of @memcpy and @memset #15278

Merged
merged 21 commits into from Apr 26, 2023
Merged

change semantics of @memcpy and @memset #15278

merged 21 commits into from Apr 26, 2023

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Apr 14, 2023

Now they use slices or array pointers with any element type instead of requiring byte pointers.

This is a breaking enhancement to the language.

  • std.mem.copy will be used for only possibly-overlapping slices, and thus renamed to copyForwards.
  • std.mem.set will be deleted in favor of the builtin.

closes #14040
closes #14094

Merge Checklist

  • the TODO in llvm.zig airStore
  • @memset undefined not optimized out @memset undefined not optimized out #14094
  • llvm: store undef if safety is disabled llvm: store undef if safety is disabled #15003
  • add test: runtime safety check for memcpy_alias
  • add test: runtime safety check for memcpy_len_mismatch
  • add test: compile error for unknown memcpy length
  • add test: compile error for non-matching memcpy lengths
  • add test: runtime safety for memset array to undefined, ABI size == 1
  • add test: runtime safety for memset array to undefined, ABI size > 1
  • add safety test: memset slice to undefined, ABI size == 1
  • add safety test: memset slice to undefined, ABI size > 1
  • Sema: implement memcpy used with tuples implement @memcpy and @memset for tuples #15479

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. labels Apr 14, 2023
const value = try func.resolveInst(bin_op.rhs);
const len = switch (ptr_ty.ptrSize()) {
.Slice => try func.sliceLen(ptr),
.One => @as(WValue, .{ .imm64 = ptr_ty.childType().arrayLen() }),
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

All Wasm instructions are typed. Considering the length is of type usize, this should use imm32 on the wasm32 target, rather than imm64. (Same in airMemcpy).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Now they use slices or array pointers with any element type instead of
requiring byte pointers.

This is a breaking enhancement to the language.

The safety check for overlapping pointers will be implemented in a
future commit.

closes #14040
 * Sema: upgrade operands to array pointers if possible when emitting
   AIR.
 * Implement safety checks for length mismatch and aliasing.
 * AIR: make ptrtoint support slice operands. Implement in LLVM backend.
 * C backend: implement new `@memset` semantics. `@memcpy` is not done
   yet.
Also introduce memset_safe AIR tag and support it in C backend and LLVM
backend.
Previously, this code casted the array pointer to u8 pointer, but I
removed that in a different commit. This commit restores the cast, but
instead of hard-coding u8, it uses the destination element pointer,
since memset now supports arbitrary element types.
and avoid new language feature in std.ArrayList for now, until x86_64
self-hosted backend can implement it.
 * make memset and memset_safe guarantee that if the length is
   comptime-known then it will be nonzero.
Needed due to the compiler depending on standard library APIs such as
ArrayList that contain `@memset` and `@memcpy` calls in them. The number
of parameters changed, so this is necessary for the compiler to build.
It does the same thing but has fewer bytes in the output.
store:
The value to store may be undefined, in which case the destination
memory region has undefined bytes after this instruction is
evaluated. In such case ignoring this instruction is legal
lowering.

store_safe:
Same as `store`, except if the value to store is undefined, the
memory region should be filled with 0xaa bytes, and any other
safety metadata such as Valgrind integrations should be notified of
this memory region being undefined.
Previously it was not multiplying by the element ABI size. Now, it uses
ptr_add instructions which do math based on the element type.
Pointer comparisons were triggering `-Wcompare-distinct-pointer-types`
before this fix, which adds `(void*)` casts if the lhs type and rhs type
do not match pointer sizeness.
@andrewrk andrewrk merged commit 3c66850 into master Apr 26, 2023
10 checks passed
@andrewrk andrewrk deleted the memcpy-memset branch April 26, 2023 17:01
truemedian added a commit to truemedian/zig that referenced this pull request Apr 27, 2023
sagehane added a commit to sagehane/zls that referenced this pull request Apr 29, 2023
Requires the minimum version to support ziglang/zig#15278
@frmdstryr
Copy link
Contributor

frmdstryr commented May 10, 2023

Unfortunately, due to #13530 now any freestanding arm builds are completely unusable. Any undefined array causes a hard fault.

Release mode does not work either because compiler optimizes anything similar to a memset into the broken functions.

Jarred-Sumner added a commit to oven-sh/bun that referenced this pull request May 13, 2023
Jarred-Sumner added a commit to oven-sh/bun that referenced this pull request May 14, 2023
* Add LIEF

* Compile LIEF

* Implement support for embedding files on macOS

* proof of concept

* Add zstd

* Implement runtime support

* Move some code around

* Update .gitmodules

* Upgrade zig

ziglang/zig#15278

* leftover

* leftover

* delete dead code

* Fix extname

* Revert "Upgrade zig"

This reverts commit dd968f3.

* Revert "leftover"

This reverts commit 7664de7.

* Revert "leftover"

This reverts commit 498005b.

* various fixes

* it works!

* leftover

* Make `zig build` a little faster

* give up on code signing support

* Support Linux & macOS

* Finish removing LIEF

* few more

* Add zstd to list of deps

* make it pretty

---------

Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>
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. enhancement Solving this issue will likely involve adding new logic or components to the codebase.
Projects
None yet
3 participants