Last Comment Bug 717762 - Proliferate js_memcpy and PodCopy
: Proliferate js_memcpy and PodCopy
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on: 933149
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 14:37 PST by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2013-10-31 01:56 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make js_memcpy and use it in a bunch of places. (37.72 KB, patch)
2012-01-12 14:37 PST, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Review
Make js_memcpy and use it in a bunch of places. (40.93 KB, patch)
2012-01-12 16:22 PST, Chris Leary [:cdleary] (not checking bugmail)
luke: review+
Details | Diff | Review

Description Chris Leary [:cdleary] (not checking bugmail) 2012-01-12 14:37:58 PST
Created attachment 588188 [details] [diff] [review]
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] 2012-01-12 14:48:54 PST
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) 2012-01-12 16:22:25 PST
Created attachment 588229 [details] [diff] [review]
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] 2012-01-12 16:30:57 PST
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) 2012-01-13 17:15:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b36175bbda47
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2012-01-13 17:48:02 PST
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd329d75054
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2012-01-18 11:02:40 PST
Take 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/7736d47f8fab
Comment 7 Marco Bonardo [::mak] 2012-01-19 02:46:05 PST
https://hg.mozilla.org/mozilla-central/rev/7736d47f8fab

Note You need to log in before you can comment on or make changes to this bug.