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

Bytes.blit implementation has different semantic in BS (bug) #743

Closed
mransan opened this issue Sep 9, 2016 · 5 comments
Closed

Bytes.blit implementation has different semantic in BS (bug) #743

mransan opened this issue Sep 9, 2016 · 5 comments
Labels

Comments

@mransan
Copy link
Contributor

mransan commented Sep 9, 2016

Bytes.blit should work "correctly even if [src] and [dst] are the same byte sequence, and the source and destination intervals overlap".

The BuckleScript implementation does not respect this semantic and therefore creates a bug in perfectly valid OCaml code.

The example below works well with OCaml while fail with BS:

let () = 
  let b = Bytes.create 3 in 
  Bytes.set b 0 'a';
  Bytes.set b 1 'b';
  Bytes.set b 2 'c';
  Bytes.blit b 0 b 1 2; 
  assert('a' = Bytes.get b 0);
  assert('a' = Bytes.get b 1);
  assert('b' = Bytes.get b 2);
  () 
@bobzhang bobzhang added the bug label Sep 9, 2016
@bobzhang
Copy link
Member

bobzhang commented Sep 9, 2016

@mransan curious how did you find the cause? will fix it soon.
This is the bug of runtime, seems jsoo has the same bug

@bobzhang
Copy link
Member

bobzhang commented Sep 9, 2016

@mransan actually it is quite hard to implement memmove semantics since there is no such thing called pointer in JS. however, the good thing is that ocaml does not have pointer either, so we only need do a special handling when src == dst.
ES6 already provide such functional called copyWithin(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/copyWithin)
we will use the polyfill here

@bobzhang
Copy link
Member

bobzhang commented Sep 9, 2016

note this will only affect caml_blit_bytes (not caml_blit_string) since it is impossible for string and bytes to have memory overlap

@mransan
Copy link
Contributor Author

mransan commented Sep 9, 2016

@bobzhang thanks for the quick reply! My little side project heavily rely on Bytes.blit and my unit tests were failing only in JavaScript. The investigation showed that a byte array was getting corrupted and eventually i traced it back to this function!

I agree that a src == dst check is the way to go ... this is currently my work around!

bobzhang pushed a commit that referenced this issue Sep 9, 2016
@bobzhang
Copy link
Member

bobzhang commented Sep 9, 2016

see #744, will make a minor release today

bobzhang pushed a commit that referenced this issue Sep 9, 2016
bobzhang pushed a commit that referenced this issue Sep 9, 2016
bobzhang added a commit that referenced this issue Sep 9, 2016
@bobzhang bobzhang closed this as completed Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants