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

assert that fromHost does not copy non-allocated memory #68

Open
exaexa opened this issue Jul 24, 2021 · 2 comments
Open

assert that fromHost does not copy non-allocated memory #68

exaexa opened this issue Jul 24, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@exaexa
Copy link

exaexa commented Jul 24, 2021

Hi! First, thanks for vuh, it makes lots of stuff much easier. :]

I noticed that in case of copying into host-inaccessible memory through the cached memory, fromHost does not check that the iterator range used to construct the buffer (ie. distance between begin and end) is really equal to the actual array size that gets transferred (which is size_bytes()).

The problem is around here, possibly also in other places:
https://github.com/Glavnokoman/vuh/blob/master/src/include/vuh/arr/deviceArray.hpp#L113

This causes really nasty failures (eg literal reboots on some certain linux drivers) and is IMO particularly surprising -- if given a smaller range, I'd expect that just the starting piece of memory is filled, as with std::copy.

I can make a PR to "fix" this, just wondering which fix would be best:

  • document this properly and leave the handling to the users?
  • assert() that iterator range matches the array size?
  • behave "as reasonably as possible" and just copy min(end-begin, size()) elements to avoid problems? (I guess this behavior is the least surprising thus most reasonable, asking mainly because I'm not sure whether it would break any other expectations.)

Thanks for suggestions!

@exaexa exaexa changed the title assert that fromHost does not copy unmapped memory assert that fromHost does not copy non-allocated memory Jul 24, 2021
@exaexa
Copy link
Author

exaexa commented Jul 24, 2021

(fixed the issue name, as "unmapped" has also different meanings here)

@Glavnokoman
Copy link
Owner

Thanks for catching this. I guess the third option (behave "as reasonably...") would be the right way to fix this (and this is actually how the device-to-host copy works in vuh2 branch which I did not have time/enough-motivation to release but I encourage you to use that instead).
Merge requests are welcome, although I may be slow to respond.

@Glavnokoman Glavnokoman added the bug Something isn't working label Jul 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants