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

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

5 years ago
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

Updated

5 years ago
Blocks: 1011300
Attachment #8484649 - Flags: review?(terrence) → review+
Attachment #8484651 - Flags: review?(terrence) → review+
Attachment #8484652 - Flags: review?(terrence) → review+
Assignee

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla35

Updated

5 years ago
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().
Assignee

Comment 8

5 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)
For Firefox, you need to use mozmemory.h and malloc_usable_size for jemalloc-enabled builds and replace-malloc-enabled builds.
Assignee

Comment 10

5 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

5 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

5 years ago
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.
Assignee

Comment 14

5 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

5 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.
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

5 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
You need to log in before you can comment on or make changes to this bug.