want better support for bound functions

[davepacheco]

Related to #87, when we ultimately found the function we were looking for, it was a bound function:

> bc2d8189::jsprint
function <anonymous> (as b)

> bc2d8189::jssource
file: native v8natives.js

 1577
 1578 function FunctionBind(a){
 1579 if(!(%_ClassOf(this)==='Function')){
 1580 throw new $TypeError('Bind must be called on a function');
 1581 }
 1582 var b=function(){
 1583
 1584 "use strict";
 1585
 1586
 1587 if(%_IsConstructCall()){
 1588 return %NewObjectFromBound(b);
 1589 }
 1590 var c=%BoundFunctionGetBindings(b);
 1591
 1592 var d=%_ArgumentsLength();
 1593 if(d==0){
 1594 return %Apply(c[0],c[1],c,2,c.length-2);
 1595 }
 1596 if(c.length===2){
 1597 return %Apply(c[0],c[1],arguments,0,d);
...

> bc2d8189::jsclosure
    "b": bc2d8189: function <anonymous> (as b)

But there's no great way (with mdb_v8) of finding the underlying function and the arguments that have been bound. According to @arekinath, the values are in the "literals_or_bindings" field:

> bc2d8189::v8print
bc2d8189 JSFunction {
    bc2d8189 JSObject {
        bc2d8189 JSReceiver {
            bc2d8189 HeapObject < Object  {
                bc2d8188 map = 9a2d9bd1 (Map)
            }
        }
        bc2d818c properties = bc2e19ad (FixedArray)
        bc2d8190 elements = af7080a1 (FixedArray)
    }
    bc2d8198 prototype_or_initial_map = 90e080a1 (Oddball: "hole")
    bc2d819c shared = 92469aa9 (SharedFunctionInfo)
    bc2d81a4 literals_or_bindings = bc2e19dd (FixedArray)
    bc2d81a8 next_function_link = 90e08091 (Oddball: "undefined")
}

> bc2e19dd::v8array | ::jsprint -ad0
9242cb41: function <anonymous> (as CueBallAgent.checkSocket)
bc2d8575: [...]
98d578ed: "manta.emy-10.joyent.us"

This is the actual binding (first the bound function, then this "this",
then the first argument that we bound: the string hostname).

You can see this in the code that called bind():

poolOpts.checker = this.checkSocket.bind(this, host);

[davepacheco]

Although this is simple in principle, this turned out to be a little nasty. Fetching the bindings is as straightforward as described. Reliably determining whether a function is bound in the first place is trickier (and it's important, since the bindings field is also used for other things). On Node versions 0.10.47, 0.12.16, and 4.6.0, you can check a particular bit of the "compiler_hints" field on the SharedFunctionInfo. However, the bit to check is different across all three versions, and we don't have postmortem metadata for it. On Node version 6.8.1, this is all completely different. There's no bit to determine if a JSFunction has bindings because there's a separate class, JSBoundFunction, that implements this behavior.

I've got changes that:

• add information about bound functions to the new ::jsfunction dcmd (see want "jsfunction" dcmd #44)
• add documentation to the user guide
• add an automated test for jsfunction, including the bound function case, that passes on both 32-bit and 64-bit Node 0.10.47, 0.12.16, 4.6.0, and 6.8.1. Depending how often the compiler hints changed in the various Node micro releases, it may not work on some intermediate versions.
• update ::jsprint to be able to print JSBoundFunctions on Node v6 and later.
• fix a bug with Node v6 where ::findjsobjects would mark an object as garbage (and not show it by default) if the object references a bound function (because we didn't grok the JSBoundFunction class)

As with all the other new work, I've extended the API in mdb_v8_dbg.h. The goal is to more cleanly separate reconstructing JS state from printing it out with mdb. This will make mdb_v8 much easier to fix and extend. In doing this, we have to decide how much callers should have to know about bound functions. I concluded that callers have to know that bound functions exist (as a first-class type, alongside regular functions, strings, regular expressions, and so on) because there's no other way to inspect them. (This is different than, say, node-once, which also wraps functions, but does so in a way that can be observed with existing tools (namely, jsclosure). Since V8 uses VM-specific magic to implement bound functions, we need a specific dcmd to expose the underlying details, which means callers have to know about them as first-class types. However, callers clearly shouldn't have to know about the drastic change in implementation in Node v6. To manage this, I've created a new v8boundfunction_t. In Node prior to v6, loading a v8boundfunction_t loads the underlying JSFunction as a v8function_t, checks the bit indicating if it's bound, and (if appropriate) fetches the bindings. In Node v6, we load the underlying JSBoundFunction. From the caller's perspective, they just pass a function value and v8boundfunction_load does the appropriate thing.

I don't think it's worth adding postmortem metadata to V8 for the bit because it's gone in modern versions anyway.

[davepacheco]

Draft documentation:
https://github.com/joyent/mdb_v8/compare/master...davepacheco:dev-issue-88?short_path=4be2078#diff-4be2078a125fe364f7198675d51bc688

[davepacheco]

Draft change: https://cr.joyent.us/#/c/2350/

This is make prepush clean with Node 0.10.26 (which I just happened to have as the default in my dev zone). Using the updated runtests_node, here are the test suite results:

Summary:
0.10.48 sunos x86: success
0.10.48 sunos x64: success
0.12.17 sunos x86: success
0.12.17 sunos x64: success
4.8.4   sunos x86: success
4.8.4   sunos x64: success
6.11.2  sunos x86: fail
6.11.2  sunos x64: fail

For 6.11.2, both 32-bit and 64-bit failed in tst.postmortem_jsstack.js because there's an extra stack frame present ("loadavg"). That's correct, though. We probably need to update this test to accept that frame.

Although it's basically ready for review, there are a few things I'd still like to fix in this change:

• Even though we'll likely never phrase the binding-related constants in Node v4 and earlier as postmortem metadata, I think it would still be cleaner to factor them out as postmortem constants with fallback values.
• I want to run several different commands and check for memory leaks with findleaks.

I'd also welcome input on which other Node versions are worth testing.

[davepacheco]

Memory leaks are probably less likely than I thought because although the v8function_.* API functions have alloc/free and such, in practice we use them with UM_GC, so even if we forgot to call free, they'd be freed at the end of the dcmd. That said, I did test this by loading mdb on the core file generated by tst.jsclosure.js and running a few success and failure cases:

$ UMEM_DEBUG=contents,guards,audit=60 mdb -S /var/tmp/node.83447 
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::load ./build/ia32/mdb_v8.so
mdb_v8 version: 1.1.5 (dev)
V8 version: 4.5.103.37
Autoconfigured V8 support from target
C++ symbol demangling enabled
> 1::jsprint
mdb: <couldn't read type>
> 0::jsprint
0
> 0::jsfunction
mdb: 0: not a heap object
> 1::jsfunction
mdb: failed to read type of 1: no mapping for address
mdb: 1: not a heap object
> ::findjsobjects -p __bind | ::findjsobjects | ::jsprint -a
bb06c479: {
    "__bind": 9e808165: uninitialized,
}
bb088145: {
    "__bind": 9e808115: true,
}
bb06c4e5: {
    "__bind": bb06c801: function <anonymous> (as <anon>),
}
> bb06c801::jsfunction
bound function that wraps: bb061acd
with "this" = bb06c529 (JSObject: Object)
      arg0  = 966271a5 (<unknown subclass>: String)
      arg1  = 966271bd (<unknown subclass>: String)
      arg2  = 966271d5 (<unknown subclass>: String)
      arg3  = 966271ed (<unknown subclass>: String)
> bb061acd::jsfunction
function: doStuff
defined at /home/dap/mdb_v8/test/standalone/tst.jsclosure.js position 772
> bb06c529::jsprint
{
    "thisObj": true,
}

Then I saved a core file of mdb and ran findleaks:

$ mdb -S core.21562 
Loading modules: [ libumem.so.1 libc.so.1 libproc.so.1 libumem.so libavl.so.1 libc.so ld.so ld.so.1 ]
> ::findleaks -v
findleaks:                maximum buffers => 94211
findleaks:                 actual buffers => 94034
findleaks: 
findleaks:             potential pointers => 3047643
findleaks:                     dismissals => 1104611       (36.2%)
findleaks:                         misses => 1562140       (51.2%)
findleaks:                           dups => 286860        ( 9.4%)
findleaks:                        follows => 94032         ( 3.0%)
findleaks: 
findleaks:              peak memory usage => 1160 kB
findleaks:               elapsed CPU time => 0.3 seconds
findleaks:              elapsed wall time => 1.0 seconds
findleaks: 
CACHE     LEAKED   BUFCTL CALLER
0810a010       2 0820fd30 mdb_alloc_align+0x43
------------------------------------------------------------------------
   Total       2 buffers, 9088 bytes
> 0820fd30::bufctl -v
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         820fd30          8210000    fa6b8b266aab2                1
                          810a010                0                0
                 libumem.so.1`umem_cache_alloc_debug+0x1fe
                 libumem.so.1`umem_cache_alloc+0x99
                 libumem.so.1`umem_alloc+0x50
                 libumem.so.1`umem_malloc+0x36
                 mdb_alloc_align+0x43
                 mdb_alloc+0x13
                 mdb_v8.so`findjsobjects_range+0x53
                 mdb_v8.so`findjsobjects_mapping+0x64
                 libproc.so.1`i_Pmapping_iter+0x66
                 libproc.so.1`Pmapping_iter+0x16
                 mdb_v8.so`findjsobjects_run+0x127
                 mdb_v8.so`dcmd_findjsobjects+0x1a5
                 dcmd_invoke+0x40
                 mdb_call_idcmd+0x128
                 mdb_call+0x16f
                 yyparse+0x3f7
                 mdb_run+0x26d
                 main+0x154c
                 _start+0x83

This looks like a potentially legit leak in findjsobjects_range() when we fail to read from the range. But it's not related to this change.

[davepacheco]

Having addressed the items I mentioned above, patchset 2 is now ready for review.

[davepacheco]

Although I'm running into #38 a lot, after a few trials, I got the same runtests results on patchset 2:

Summary:
0.10.48 sunos x86: success
0.10.48 sunos x64: success
0.12.17 sunos x86: success
0.12.17 sunos x64: success
4.8.4   sunos x86: success
4.8.4   sunos x64: success
6.11.2  sunos x86: fail
6.11.2  sunos x64: fail

And the v6 failures are the same as before.

[davepacheco]

Patchset 3 includes a few changes from code review. I've run make prepush from a clean build using Node 0.10.46 without issue.