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

std.mem.copy does not work if slices overlap #382

Closed
andrewrk opened this issue May 29, 2017 · 5 comments
Closed

std.mem.copy does not work if slices overlap #382

andrewrk opened this issue May 29, 2017 · 5 comments
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase.
Milestone

Comments

@andrewrk
Copy link
Member

Zig's current implementation of std.mem.copy is this:

/// Copy all of source into dest at position 0.
/// dest.len must be >= source.len.
/// The memory areas must not overlap.
pub fn copy(comptime T: type, dest: []T, source: []const T) {
    for (source) |s, i| dest[i] = s;
}

"The memory areas must not overlap" is a pretty lame restriction, especially since the single branch it takes to make it work either way is a low cost. So this could be:

/// Copy all of source into dest at position 0.
/// dest.len must be >= source.len.
pub fn copy(comptime T: type, dest: []T, source: []const T) {
    if (usize(dest.ptr) < usize(source.ptr) or
        usize(source.ptr) + source.len <= usize(dest.ptr))
    {
        for (source) |s, i| dest[i] = s;
    } else {
        for (source) |s, i| dest[source.len - i] = source[source.len - i];
    }
}

If we want to have another function, copyAssumeNoOverlap with this branch optimized away, that's fine. We'd still need to add that assert though:

/// Copy all of source into dest at position 0.
/// dest.len must be >= source.len.
/// The memory areas must not overlap.
pub fn copyAssumeNoOverlap(comptime T: type, dest: []T, source: []const T) {
    assert(usize(dest.ptr) < usize(source.ptr) or usize(source.ptr) + source.len <= usize(dest.ptr));
    for (source) |s, i| dest[i] = s;
}

Now here's the problem:

  • usize(slice.ptr) does not work at compile time
  • std.mem.copy should work at compile time

At compile time we still have this concept of overlapping memory, but the way we want to check for overlapping memory doesn't work.

@andrewrk andrewrk added the enhancement Solving this issue will likely involve adding new logic or components to the codebase. label May 29, 2017
@andrewrk andrewrk added this to the 0.2.0 milestone May 29, 2017
@raulgrell
Copy link
Contributor

raulgrell commented Jun 24, 2017

The implementation you provided above has a couple of bugs that hadn't surfaced due to my limited testing - it's based off the snippet I sent in IRC, right?

This is what I have for now:

pub fn copy(comptime T: type, dest: []T, source: []const T) -> []T {
    @setDebugSafety(this, false);
    assert(dest.len >= source.len);
    for (source) |s, i| dest[i] = s;
    return dest;
}

pub fn move(comptime T: type, dest: []T, src: []const T) -> []T {
    assert(dest.len >= source.len);
    // Same address
    if (dest.ptr == src.ptr) return dest;
    
    // No overlap
    if (usize(src.ptr) + (src.len * @sizeOf(T)) <= usize(dest.ptr)
            or usize(dest.ptr) + (dest.len * @sizeOf(T)) <= usize(src.ptr)) {
        // Safe to @memcopy, mem.copy or whatever
        return copy(T, dest, src);
    }

    // Overlapping buffers, determine direction
    if (usize(dest.ptr) < usize(src.ptr)) {
        for (src) |s, i| dest[i] = s;
    } else {
        for (src) |s, i| dest[src.len - i - 1] = src[src.len - i - 1]; 
    }
    return dest;
}

@kyle-github
Copy link

The C standard (or is it POSIX?) specifications note that it is forbidden for the ranges to overlap in memcpy and they can in memmove. This is highly annoying so I am happy to see that Zig is attempting to do this right.

@andrewrk
Copy link
Member Author

I think this may actually be solved by #476 - add noalias for slices. Then the compiler does the assertion, and the code works at compile-time.

@andrewrk andrewrk modified the milestones: 0.2.0, 0.3.0 Feb 28, 2018
@andrewrk andrewrk modified the milestones: 0.3.0, 0.4.0 Aug 10, 2018
@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Feb 19, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 20, 2019
@andrewrk andrewrk removed the enhancement Solving this issue will likely involve adding new logic or components to the codebase. label Feb 10, 2020
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Feb 10, 2020
@shawnl
Copy link
Contributor

shawnl commented Jun 7, 2020

The root problem is described in #5553. This is just a symptom.

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 9, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Nov 6, 2020
@SpexGuy SpexGuy added the enhancement Solving this issue will likely involve adding new logic or components to the codebase. label Feb 12, 2021
@andrewrk andrewrk removed this from the 0.9.0 milestone May 19, 2021
@andrewrk andrewrk added this to the 0.10.0 milestone May 19, 2021
@andrewrk andrewrk modified the milestones: 0.13.0, 0.11.0 May 5, 2023
@andrewrk
Copy link
Member Author

andrewrk commented May 5, 2023

This was solved by #14040. The new semantics of @memcpy assert the slices do not overlap, and it works at compile-time. Meanwhile, std.mem.copy is renamed to std.mem.copyForwards and is defined to do exactly that, with no other restrictions.

@andrewrk andrewrk closed this as completed May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase.
Projects
None yet
Development

No branches or pull requests

5 participants