findjsobjects -r should cleanup referents tree when appropriate

[misterdjules]

When marking objects as referents with ::findjsobjects -m and then performing a search for references on a given referent with the addr::findjsobjects -r command form, mdb_v8 aborts due to an assertion:

root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# rm -f ~/cores/core.node.*
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# node --version
v4.5.0
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# ls -lt ~/cores/
total 0
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# node -e 'process.abort()'
Abort (core dumped)
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# ls -lt ~/cores/
total 47008
-rw------- 1 root root 48037605 Sep 21 00:02 core.node.7512
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# mdb `which node` ~/cores/core.node.7512 
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::load /root/mdb_v8/build/amd64/mdb_v8.so
mdb_v8 version: 1.1.4 (dev)
V8 version: 4.5.103.37
Autoconfigured V8 support from target
C++ symbol demangling enabled
> ::findjsobjects -c Object ! tail -1
328cfc0516e1
> 328cfc0516e1::findjsobjects -m
findjsobjects: marked 328cfc0516e1
> 328cfc0516e1::findjsobjects -r 
Assertion failed: fjs->fjs_marking, file src/mdb_v8.c, line 5079

*** mdb: received signal ABRT at:
    [1] libc.so.1`_lwp_kill+0xa()
    [2] libc.so.1`raise+0x20()
    [3] libc.so.1`abort+0x60()
    [4] libc.so.1`__assert+0x8a()
    [5] mdb_v8.so`findjsobjects_referent+0x51()
    [6] mdb_v8.so`dcmd_findjsobjects+0x43f()
    [7] mdb`dcmd_invoke+0x7c()
    [8] mdb`mdb_call_idcmd+0x112()
    [9] mdb`mdb_call+0x3e1()
    [10] mdb`yyparse+0xdb4()
    [11] mdb`mdb_run+0x2cd()
    [12] mdb`main+0xc9d()
    [13] mdb`_start+0x6c()

mdb: (c)ore dump, (q)uit, (r)ecover, or (s)top for debugger [cqrs]? 
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]#

The reason is that, when executing the addr::findjsobjects -r form, mdb_v8 does not destroy the tree of referents that may have been built during a previous run of ::findjsobjects -m before adding new referents.

My proposed solution is that if -m is not passed to ::findjsobjects, we can destroy the tree of referents before adding new referents.

Successive runs of ::findjsobjects -m can still accumulate referents, but as soon as a non-marking ::findjsobjects command is used, the tree of referents is cleared.

[misterdjules]

Does the following approach:

diff --git a/src/mdb_v8.c b/src/mdb_v8.c
index 1227c85..f65d114 100644
--- a/src/mdb_v8.c
+++ b/src/mdb_v8.c
@@ -5099,13 +5099,25 @@ findjsobjects_referent(findjsobjects_state_t *fjs, uintptr_t addr)
 }

 static void
+findjsobjects_referents_destroy(findjsobjects_state_t *fjs)
+{
+   avl_tree_t *referents = &fjs->fjs_referents;
+   findjsobjects_referent_t *referent;
+   void *cookie = NULL;
+
+   while ((referent = avl_destroy_nodes(referents, &cookie)) != NULL)
+       mdb_free(referent, sizeof (findjsobjects_referent_t));
+
+   fjs->fjs_head = NULL;
+   fjs->fjs_tail = NULL;
+}
+
+static void
 findjsobjects_references(findjsobjects_state_t *fjs)
 {
    findjsobjects_reference_t *reference;
    findjsobjects_referent_t *referent;
-   avl_tree_t *referents = &fjs->fjs_referents;
    findjsobjects_obj_t *obj;
-   void *cookie = NULL;
    uintptr_t addr;

    fjs->fjs_referred = B_FALSE;
@@ -5164,11 +5176,7 @@ findjsobjects_references(findjsobjects_state_t *fjs)
    /*
     * Finally, destroy our referent nodes.
     */
-   while ((referent = avl_destroy_nodes(referents, &cookie)) != NULL)
-       mdb_free(referent, sizeof (findjsobjects_referent_t));
-
-   fjs->fjs_head = NULL;
-   fjs->fjs_tail = NULL;
+   findjsobjects_referents_destroy(fjs);
 }

 static findjsobjects_instance_t *
@@ -5560,6 +5568,15 @@ dcmd_findjsobjects(uintptr_t addr,
            return (DCMD_OK);
        }

+       if (!fjs->fjs_marking) {
+           /*
+            * Destroy all referents added by previous marking
+            * commands before adding new referents for this
+            * non-marking command invocation.
+            */
+           findjsobjects_referents_destroy(fjs);
+       }
+
        if (!listlike) {
            findjsobjects_referent(fjs, inst->fjsi_addr);
        } else {

make sense? If so I'll submit a PR asap with tests and maybe a few extra assertions.

[davepacheco]

I did not know that "findjsobjects -m" existed, and had trouble understanding what it does. If I'm understanding correctly, it's intended as a way to do "ADDR::findjsobjects -r" for several addresses in the same search. So the idea is that if you would run:

ADDR1::findjsobjects -r
ADDR2::findjsobjects -r
ADDR3::findjsobjects -r

then this would be equivalent, but require only one scan:

ADDR1::findjsobjects -m
ADDR2::findjsobjects -m
ADDR3::findjsobjects -m
::findjsobjects -r

If I'm understanding that correctly, then it's working as intended (the referents tree is supposed to stick around), and the proposed solution would break that behavior. The fact that there's no way to list marked objects, remove them, or even clear them makes me wonder if I'm misunderstanding, though.

Maybe you're supposed to use ::findjsobjects -mr to do the scan of marked objects? In that case, I think your proposal makes sense as implemented (where you only clear the tree if an address was specified). I wonder if we should print a message indicating that we're removing existing markings (if there are any).

[misterdjules]

I did not know that "findjsobjects -m" existed, and had trouble understanding what it does. If I'm understanding correctly, it's intended as a way to do "ADDR::findjsobjects -r" for several addresses in the same search. So the idea is that if you would run:

ADDR1::findjsobjects -r
ADDR2::findjsobjects -r
ADDR3::findjsobjects -r

then this would be equivalent, but require only one scan:

ADDR1::findjsobjects -m
ADDR2::findjsobjects -m
ADDR3::findjsobjects -m
::findjsobjects -r

I believe that is correct. It is (and was) my understanding as well.

If I'm understanding that correctly, then it's working as intended (the referents tree is supposed to stick around) and the proposed solution would break that behavior.

I don't think the proposal would break the behavior described above. The proposal clears the tree of referents only when the explicit (when specifying an address) form of addr::findjsobjects -r is used, not when ::findjsobjects -r is used without specifying the address of an object.

Here's an example that illustrates the behavior of a version of mdb_v8 built locally with the proposed changes:

[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# cat /var/tmp/test-referents.js 
function Foo() {
  this.bar = 42;
}

var fooList = [];

fooList.push(new Foo());
fooList.push(new Foo());
fooList.push(new Foo());

process.abort();
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~]# node /var/tmp/test-referents.js 
Abort (core dumped)
[root@0c933410-7c9b-4bea-96b6-2db35946bc95 ~/mdb_v8]# mdb `which node` ~/cores/core.node.34492 
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::load /root/mdb_v8/build/amd64/mdb_v8.so
mdb_v8 version: 1.1.4 (dev)
V8 version: 4.5.103.37
Autoconfigured V8 support from target
C++ symbol demangling enabled
> ::findjsobjects -c Foo | ::findjsobjects
2c384c088131
2c384c0882e1
2c384c088261
> 2c384c088131::findjsobjects -r
2c384c088131 referred to by 2c384c087f69[0]
> 2c384c0882e1::findjsobjects -r
2c384c0882e1 referred to by 2c384c087f69[2]
> 2c384c088261::findjsobjects -r
2c384c088261 referred to by 2c384c087f69[1]
> ::findjsobjects -c Foo | ::findjsobjects | ::findjsobjects -m
> 2c384c088131::findjsobjects -m
findjsobjects: marked 2c384c088131
> 2c384c0882e1::findjsobjects -m
findjsobjects: marked 2c384c0882e1
> 2c384c088261::findjsobjects -m
findjsobjects: marked 2c384c088261
> ::findjsobjects -r
2c384c088131 referred to by 2c384c087f69[0]
2c384c0882e1 referred to by 2c384c087f69[2]
2c384c088261 referred to by 2c384c087f69[1]
> 

The resulting behavior is what we expected, and a subsequent addr::findjsobjects -r command doesn't abort and outputs the correct result:

> 2c384c088261::findjsobjects -r
2c384c088261 referred to by 2c384c087f69[1]
>

The fact that there's no way to list marked objects, remove them, or even clear them makes me wonder if I'm misunderstanding, though.

The problem that I'm trying to solve is that, when running the following two commands in sequence:

> addr::findjsobjects -m
> addr::findjsobjects -r

mdb_v8 currently aborts, which is not the expected and desired behavior. The proposed changes solve this problem by assuming that, if the user specifies an address when running addr::findjsobjects -r, they mean that they want to find the references to addr only, not to any referents that may have been marked before with ::findjsobject -m.

This is one approach, there are other different approaches possible, including:

1. just adding addr to the list of referents and providing a separate command to clear all referents
2. requiring users to specify both -m and -r options to ::findjsobjects to find references to previously marked referents, and providing a separate command to clear all marked referents.

What is the approach that seems the most reasonable to you?

[davepacheco]

I think your proposal is fine. Just to clarify, we only clear the tree if you explicitly specify an address to "::findjsobjects -r". As we discussed, I think it would be worth printing a message when we remove anything in this case.