737258 - GC: pass real addresses to the trace function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Currently all Value and jsid's have to pass through the stack before getting passed to the tracer function.  This is because we need to send a raw void* as the target of the callback.  This cannot be a tagged pointer: every trace function doesn't want to have to care if its looking at a Value, id, or just an Object*.  Generally they only want to know the target that is being pointed at.

For our write buffer verifier, we do care about these details.  Passing all values and ids through the stack means that we do not get correct origin information (e.g. the pointer we store in the write buffer) and so cannot easily match against the pointers in the write buffer.

The easiest way to fix this is to make the pointers: void *target and void *location, which encode the target Cell and the location of the thing storing the pointer to the Cell.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0

This turned out to be a bit longer than I had intended, but I think it's fairly sane overall.

The key change is that MarkInternal no longer updates the T* it gets passed, it just returns a new T*, if updated.  Since MarkKind is called by MarkValueInternal I made it have the same interface, so that we can do value updates correctly as well.  I did the same for MarkGCThingRoot, since everywhere we call it we need it to not move and this makes it easier to assert.

There is also one problem I uncovered that we're going to have down the road with the current interface to MarkValue.  Most callers of MarkValue want to just call MarkValue(&obj->foo) and don't care if the value changes.  For Values stored as hash table keys, the move has to be done by the HashTable.  Right now we would just use a temp Value on the stack, but this prevents us from having the location reported correctly to the remembered set verifier.  I think we need a new 2-argument version of MarkValue that does not update the key inline or one that takes the location separately.  I will probably end up folding this into the rekey bug, but I would appreciate your thoughts on the interface.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 608048 [details] [diff] [review]
v0

I know we talked about changing the marking interface, but now that I see it, I don't really like it.

I do have an idea though. The location parameter is only going to be used in debug builds, since it's only needed by the verifier. So rather than passing it to the tracing callback via a parameter, we could add an extra field to JSTracer for the location. Most of the time it would be null, which would mean that the location from the parameter is correct. But if it's non-null, then the verifier should treat that as the real location.

I think this would be less confusing to people since it allows them to ignore the location unless they really need it.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v1: Much more reasonable.

3 files changed, 18 insertions(+), 1 deletions(-)

I considered adding a RAII class for this, but there are only... well, you can see the diffstat.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 612621 [details] [diff] [review]
v1: Much more reasonable.

Review of attachment 612621 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is very close.

::: js/src/jsapi.h
@@ +3239,5 @@
>      size_t              debugPrintIndex;
>      JSBool              eagerlyTraceWeakMaps;
> +#ifdef __cplusplus
> +    js::DebugOnly<void *> realLocation;
> +#endif

I think this will break anyone who uses a tracer from C code. We don't care too much about space here, so let's just make it a normal field.

::: js/src/jsgc.cpp
@@ +1051,5 @@
>      JS_snprintf(nameBuf, sizeof(nameBuf), pattern, thing);
>      JS_SET_TRACING_NAME(trc, nameBuf);
>  #endif
>      void *tmp = thing;
> +    trc->realLocation = (void *)w;

Let's add an inline function somewhere to set this, but only #ifdef DEBUG.

@@ +1056,2 @@
>      MarkKind(trc, &tmp, traceKind);
> +    trc->realLocation = NULL;

Can you have MarkInternal null out realLocation at the end, like it does for debugPrinter and debugPrintArg? Then you can get rid of the other null assignments.

@@ +1104,5 @@
> +            void *tmp = root->thing;
> +            trc->realLocation = &root->thing;
> +            MarkKind(trc, &tmp, root->kind);
> +            trc->realLocation = NULL;
> +            JS_ASSERT(tmp == root->thing);

I don't think we'll be using this code with the verifier, so there's no need to set the real location here AFAIK.

Comment 5

[Terrence Cole [:terrence]]

Attachment: v2: A bit less ambitious.

Uglier, but I suppose it matches the rest of JSTracer functionality better this way.

Comment 6

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5916e57c2f

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0c5916e57c2f