717762 - Proliferate js_memcpy and PodCopy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Make js_memcpy and use it in a bunch of places.

I think it'd be nice to have a js_memcpy(dst, src, len) that asserts that src does not fall in the range [dst, dst + len) and use it throughout the engine.

This passes locally, pushing to try.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 588188 [details] [diff] [review]
Make js_memcpy and use it in a bunch of places.

>+static JS_ALWAYS_INLINE void *
>+js_memcpy(void *dst, const void *src, size_t len)
>+{
>+    /* Check that src is not in [dst, dst + len). */
>+    JS_ASSERT(!(dst <= src && src < (char *) dst + len));
>+    return memcpy(dst, src, len);
>+}

I think non-overlapping-ness wants an even stronger assert, say, like the one in PodCopy:

>@@ -316,17 +324,17 @@ PodCopy(T *dst, const T *src, size_t nel
>     /* Cannot find portable word-sized abs(). */
>     JS_ASSERT_IF(dst >= src, size_t(dst - src) >= nelem);
>     JS_ASSERT_IF(src >= dst, size_t(src - dst) >= nelem);

Second, it looks like a lot (but not all) of these memcpy uses could be PodCopy instead (avoiding the sizeof(T) computation).  Could you switch them since you are touching all these lines anyway?

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Make js_memcpy and use it in a bunch of places.

Now with PodAssign and a bunch of PodCopy's!

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 588229 [details] [diff] [review]
Make js_memcpy and use it in a bunch of places.

r+ with more PodCopy/PodAssign in TradeGuts and JSScript::NewScriptFromEmitter (if possible) and with verification that the generated asm isn't any worse.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b36175bbda47

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd329d75054

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Take 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/7736d47f8fab

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7736d47f8fab