Closed
Bug 1063247
Opened 7 years ago
Closed 7 years ago
[jsdbg2] Debugger.Memory needs a mallocSizeOf function from the embedding to produce accurate byte sizes
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jimb, Assigned: jimb)
References
Details
Attachments
(3 files)
1.85 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
6.02 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
6.02 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Debugger.Memory needs the embedding to provide it with a function that can find the size of a malloc'd block, given the address of the block. Various Debugger.Memory functions want to return the byte sizes of various kinds of objects. Ideally, these sizes should include whatever internal padding malloc adds to the requested block sizes. about:memory accomplishes this by having all reporting functions require a pointer to a function that takes a 'void *' and returns the size of the block it points to, presumably by consulting malloc's internal data structures. (For example, the GNU C library provides a function, malloc_usable_size, that does this.) The code that implements the about:memory reports chooses the right function for the platform and passes it in at measurement time. But functions like Debugger.Memory.prototype.takeCensus need to be ready for use by JavaScript, which isn't in a position to supply pointers to random platform-specific C functions. So: the embedding should be able to set a malloc block sizing function on the JSRuntime, and Debugger should use that when it needs one.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8484649 -
Flags: review?(terrence)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8484651 -
Flags: review?(terrence)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8484652 -
Flags: review?(terrence)
Updated•7 years ago
|
Attachment #8484649 -
Flags: review?(terrence) → review+
Updated•7 years ago
|
Attachment #8484651 -
Flags: review?(terrence) → review+
Updated•7 years ago
|
Attachment #8484652 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 4•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd80b2984fe2 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ffa64956c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6033944c4c1
Assignee | ||
Updated•7 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla35
I backed this and bug 1063233 out for non-unified Windows build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/04be894027e6 https://tbpl.mozilla.org/php/getParsedLog.php?id=47507112&tree=Mozilla-Inbound
Flags: needinfo?(jimb)
![]() |
||
Comment 6•7 years ago
|
||
jimb, the third patch as landed has much more stuff in it than the reviewed version, which seems suspicious.
Also, before relanding, you should add this extra case to ShellMallocSizeOf (which I cribbed from moz_malloc_usable_size):
> #elif defined(XP_WIN)
> return _msize(ptr);
![]() |
||
Comment 7•7 years ago
|
||
Ah, I just saw bug 1063697, which mentions _msize().
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #7) > Ah, I just saw bug 1063697, which mentions _msize(). That whole thing was quite a trainwreck.
Flags: needinfo?(jimb)
Comment 9•7 years ago
|
||
For Firefox, you need to use mozmemory.h and malloc_usable_size for jemalloc-enabled builds and replace-malloc-enabled builds.
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #9) > For Firefox, you need to use mozmemory.h and malloc_usable_size for > jemalloc-enabled builds and replace-malloc-enabled builds. I can't use that header from js/src/shell/js.cpp, though.
Assignee | ||
Comment 11•7 years ago
|
||
Mike, I need a way to get malloc block sizes in the JS shell, js/src/shell/js.cpp. This can't use headers from memory/build, as far as I know.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 12•7 years ago
|
||
Here's a try push with lots of disunification: https://tbpl.mozilla.org/?tree=Try&rev=8b1340a4c337
![]() |
||
Comment 13•7 years ago
|
||
> Mike, I need a way to get malloc block sizes in the JS shell,
> js/src/shell/js.cpp. This can't use headers from memory/build, as far as I
> know.
Just copying the code from moz_malloc_usable_size into ShellMallocSizeOf seems reasonable to me.
Assignee | ||
Comment 14•7 years ago
|
||
That one looks good, so here's a try push with no disunification: https://tbpl.mozilla.org/?tree=Try&rev=d0c954be7700
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #13) > > Mike, I need a way to get malloc block sizes in the JS shell, > > js/src/shell/js.cpp. This can't use headers from memory/build, as far as I > > know. > > Just copying the code from moz_malloc_usable_size into ShellMallocSizeOf > seems reasonable to me. Yeah, that's what I've done --- with the concomitant configure.in grunge.
Assignee | ||
Comment 16•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cbc5079834 https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6d81ac0bba https://hg.mozilla.org/integration/mozilla-inbound/rev/ed38f85902f7
Status: NEW → ASSIGNED
Flags: needinfo?(mh+mozilla)
Flags: in-testsuite-
Flags: in-testsuite+
![]() |
||
Comment 17•7 years ago
|
||
Should the condition in ShellMallocSizeOf be this: > #elif defined(HAVE_MALLOC_USABLE_SIZE) || defined(MOZ_MEMORY) instead of this: > #elif defined(HAVE_MALLOC_USABLE_SIZE) jemalloc (i.e. MOZ_MEMORY) seems to be enabled for shell builds so it would be a shame to miss some cases. But maybe it's ok; I'm just speculating.
Comment 18•7 years ago
|
||
jemalloc isn't linked into shell. And according to bug 704045 mozalloc relies on specific behavior of mozjemalloc that may not be true for jemalloc3. In file included from js/src/shell/Unified_cpp_js_src_shell0.cpp:15: js/src/shell/js.cpp:5870:12: error: use of undeclared identifier 'malloc_usable_size'; did you mean 'malloc_good_size'? return malloc_usable_size(ptr); ^~~~~~~~~~~~~~~~~~ malloc_good_size /usr/include/malloc/malloc.h:101:15: note: 'malloc_good_size' declared here extern size_t malloc_good_size(size_t size); ^ In file included from js/src/shell/Unified_cpp_js_src_shell0.cpp:15: js/src/shell/js.cpp:5870:31: error: cannot initialize a parameter of type 'size_t' (aka 'unsigned long') with an lvalue of type 'void *' return malloc_usable_size(ptr); ^~~ /usr/include/malloc/malloc.h:101:39: note: passing argument to parameter 'size' here extern size_t malloc_good_size(size_t size); ^ 2 errors generated. or js/src/shell/tmpHMv2CG.list: Unified_cpp_js_src_shell0.o ../editline/Unified_c_js_src_editline0.o ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame Undefined symbols for architecture x86_64: "_malloc_usable_size", referenced from: ShellMallocSizeOf(void const*) in Unified_cpp_js_src_shell0.o ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) make[5]: *** [js] Error 1 make[4]: *** [js/src/shell/target] Error 2 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=828c171af981 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c67fea8f95eb
Comment 19•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1cbc5079834 https://hg.mozilla.org/mozilla-central/rev/ef6d81ac0bba https://hg.mozilla.org/mozilla-central/rev/ed38f85902f7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•