wasmtime / issue #6421 Add `EntityList::copy_from` · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #6421 Add `EntityList::copy_from`

Wasmtime GitHub notifications bot (May 22 2023 at 19:44):

I'll leave detailed review to @elliottt but I took a look and I'm having a difficult time convincing myself that this implementation is correct. Some things I'm unclear on:

What happens if the new size is in a larger size class so the list is reallocated by grow_at?

What happens if you're copying within a single list and the source overlaps the destination?

Also, I'd love to hear more about what motivated adding this.

Wasmtime GitHub notifications bot (May 22 2023 at 20:00):

PROMETHIA-27 commented on issue #6421:

I don't believe other will ever move when reallocating, but I may be wrong about that. This could be a real bug.

Vec::copy_within uses memmove, not memcpy, so this is a perfectly valid operation.

This ended up getting refactored out for now, but the original need was that I had an iterator of EntityList and wanted to produce a new EntityList from their concatenated values (without having an intermediate allocation). This caused borrowck issues with .extend and from_iter because I can't borrow the slice of values and mutate at the same time.

Wasmtime GitHub notifications bot (May 22 2023 at 22:55):

jameysharp commented on issue #6421:

Yes, other shouldn't move, but self may. What I missed was that normally self.index is only read after grow_at returns, so it reflects the result of any reallocation.

However that's not actually true if other is None. If self gets reallocated _and_ we're moving within the same list, then the source range will point into the list which was just freed. That still gets the correct result in the current implementation since the old list's contents don't get overwritten immediately, but it's questionable.

I have a similar concern about moving within the same list with regard to the documentation for grow_at, which claims the newly-inserted elements have no specified value. So if the source overlaps with the destination, where the destination was allocated with grow_at, then some portion of the source is copied from unspecified values. Again I _think_ this gets the correct result in the current implementation, because grow_at actually leaves behind the old values in those elements, but it's hard for me to convince myself that it's right and that future changes elsewhere won't break it.

Wasmtime GitHub notifications bot (May 22 2023 at 23:34):

PROMETHIA-27 commented on issue #6421:

That's a very good point, the reallocation when moving within self demonstrably breaks- I ended up with an Inst0 in a vec that had only Inst1-4, on top of otherwise being incorrect.

I can add extra logic to handle this case, or just ban the "copy within one list" case. The logic would be somewhat verbose and I'm not sure if anyone will ever need that functionality, but I might do it just for completeness' sake.

For now I'm going to push the "ban" option along with some extra tests and if it's agreed that the extra complexity isn't worth supporting the other case then I'm fine with it being merged in that state, I certainly don't need that functionality.

Wasmtime GitHub notifications bot (May 22 2023 at 23:39):

PROMETHIA-27 edited a comment on issue #6421:

That's a very good point, the reallocation when moving within self demonstrably breaks- I ended up with an Inst0 in a vec that had only Inst1-4, on top of otherwise being incorrect.

I can add extra logic to handle this case, or just ban the "copy within one list" case (which I forgot implicitly happens because of borrowck without an Option). The logic would be somewhat verbose and I'm not sure if anyone will ever need that functionality, but I might do it just for completeness' sake.

For now I'm going to push the "ban" option along with some extra tests and if it's agreed that the extra complexity isn't worth supporting the other case then I'm fine with it being merged in that state, I certainly don't need that functionality.

Last updated: Nov 22 2024 at 17:03 UTC