Closed Bug 1063247 Opened 6 years ago Closed 6 years ago

[jsdbg2] Debugger.Memory needs a mallocSizeOf function from the embedding to produce accurate byte sizes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(3 files)

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.
Blocks: 1011300
Attachment #8484649 - Flags: review?(terrence) → review+
Attachment #8484651 - Flags: review?(terrence) → review+
Attachment #8484652 - Flags: review?(terrence) → review+
Flags: in-testsuite-
Target Milestone: --- → mozilla35
Depends on: 1063726
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);
Ah, I just saw bug 1063697, which mentions _msize().
(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)
For Firefox, you need to use mozmemory.h and malloc_usable_size for jemalloc-enabled builds and replace-malloc-enabled builds.
(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.
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)
Here's a try push with lots of disunification:
https://tbpl.mozilla.org/?tree=Try&rev=8b1340a4c337
> 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.
That one looks good, so here's a try push with no disunification:
https://tbpl.mozilla.org/?tree=Try&rev=d0c954be7700
(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.
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.
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
You need to log in before you can comment on or make changes to this bug.