dimitriv / Ziria

A domain-specific-language and compiler for low-level bitstream processing.
92 stars 18 forks source link

blink_copy undefined behavior #121

Open ghost opened 8 years ago

ghost commented 8 years ago

blink_copy uses memcpy which executes undefined behavior for overlapping memories [1] [2]. We do indeed have overlapping memories (in at least CCA). The tests pass on Windows, but that's not guaranteed to happen. The CCA test definitely fails on Linux. For now, I've added a commit (https://github.com/dimitriv/Ziria/commit/7e278869275fff55fcff386c4c5ef64cd4378dbb) to allow Linux to use memmove instead of memcpy. Historically, memmove has been slower than memcpy, and it probably still is, but both have a time complexity of O(N) so I would suggest using memmove for the Windows implementation as well. Alternatively the compiler could figure out when to use memmove instead of memcpy. And worst case scenario, we could just rely on undefined behavior in Windows.

[1] - https://msdn.microsoft.com/en-us/library/dswaw1wk.aspx [2] - http://man7.org/linux/man-pages/man3/memcpy.3.html

dimitriv commented 8 years ago

Agreed! In fact I may have (or had) at some point special cases in the code generator to tackle with precisely this issue. See function `cgAssignAliasing in CgCmdDom.hs, which is -- for efficiency reasons precisely -- commented out in CgExpr.hs :-)

dimitriv commented 8 years ago

But, my cgAssignAliasing trick I am realizing is not enough for functions, where arguments may alias:

var x; 
let fun f(y) {   x := y } 
-- call site
f(x)

So I think that the correct semantically fix is to actually use memmove as you say. But I'd probably emit code that just calls blink_copy for now, and still keep using memcopy for Windows (unless of course you observe that it does not make a difference -- I recall that it did make a difference). We can warn programmers if they call functions that have arguments that alias each other or even through errors. I hate aliasing, explicit (which we already avoid) or implicit (from those function arguments)

mainland commented 8 years ago

Sid and I discussed this this morning and agreed that he should commit a fix that uses memove on Linux but leaves the Windows code alone.