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

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

Projects

None yet

2 participants

@mransan
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
Contributor
bobzhang commented Sep 9, 2016 edited

@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 bobzhang referenced this issue in ocsigen/js_of_ocaml Sep 9, 2016
Closed

Bytes.blit implementation has different semantic #521

@bobzhang bobzhang removed the PRIORITY:HIGH label Sep 9, 2016
@bobzhang
Contributor
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
Contributor
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
Contributor
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 bobzhang pushed a commit that referenced this issue Sep 9, 2016
Hongbo Zhang bug fix for #743 13e6546
@bobzhang
Contributor
bobzhang commented Sep 9, 2016

see #744, will make a minor release today

@bobzhang bobzhang pushed a commit that referenced this issue Sep 9, 2016
Hongbo Zhang bug fix for #743 ca02cd2
@bobzhang bobzhang pushed a commit that referenced this issue Sep 9, 2016
Hongbo Zhang bug fix for #743 6a87e0c
@bobzhang bobzhang closed this Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment