The only place we currently add a Scope* to the GCThingList is EmitterScope::internEmptyGlobalScopeAsBody, where &bce->cx->global()->emptyGlobalScope() is added to the GCThingList.

The stencil format is going to be simpler if we instead store a special sentinal type in the GCThingList.

Then we can get that particular scope during instantiation.

I found out about this bug on https://phabricator.services.mozilla.com/D65931.

I'm interested in doing this, but I don't understand SpiderMonkey yet, so it might take some time.
Could you give me some advice, please?

Hi Kosuke,

So to provide a bit of context: A large undertaking that's happening within the SpiderMonkey team right now is a project called "Stencil". The goal of this project is to make our compiler front end (which take JS source text, and converts that to bytecode) produce a documented data format that doesn't require access to Garbage Collected (GC'd) objects.

The way this will then work is that we'll take this data-only format (the stencil), and we'll instantiate it, filling in all the objects later. There's some complication here with how offthread-parsing works, but it's more detail than you need right now.

While we are creating our bytecode, we create references to objects that need to be brought into existence, or need to be referenced. In the instance of this bug, when we call EmitterScope::internEmptyGlobalScopeAsBody we are adding a reference to a specific Scope, that already exists outside the frontend.

So what we'd like to do in this bug is replace that specific scope with a special value, that then implies when we do our instantiate step, that we grab the specific scope cx->global()->emptyGlobalScope().

So, with that background, here's what you're going to need to do: (normally I would put this all into searchfox links, but some things have moved around recently and as I write this searchfox isn't updated yet)

In js/src/frontend/Stencil.h, there is a type called ScriptThingsVariant:

// These types all end up being baked into GC things as part of stencil
// instantiation. Currently, GCCellPtr is part of this list while we complete
// Stencil, but eventually will be removed.
using ScriptThingVariant =
    mozilla::Variant<JS::GCCellPtr, BigIntIndex, ObjLiteralCreationData,
                     RegExpIndex, ScopeIndex>;

Before the definition of that type, you'll add a new empty class; we'll call it EmptyGlobalScopeType. This class is just a distinct type, which we need because we're using some C++ template metaprogramming tricks :)

Add that type to the ScriptThingsVariant (a Variant is a discriminated union, similar to a Rust enum.) This will allow us to store an instance of this type inside the ScriptThingsVector

Then you'll go to the Matcher in BytecodeSection.cpp here: this controls how things in the ScriptThingsVectorare converted to GC types. You'll add an overload ofbool operator()(const EmptyGlobalScopeType& emptyGlobalScope), which simply assignscx->global()->emptyGlobalScope()` to the output and returns true.

After that, you'll go to internEmptyGlobalScope and instead of putting the Scope into the list, you'll put an instance of EmptyGlobalScopeType. Now, there's complication here; we derive the value of hasEnvironment_ here from the scope; I'm going to argue for now that this is OK: How we access information from scopes that pre-exist our particular call to a frontend is a future work problem, so for now don't worry about the other reference to emptyGlobalScope there. We just don't want it in that list, because we eventually want the ScriptThingsVector to not contain any GC things.

After this, we should build, and test making sure we pass the test (../jit-test/jit_test.py ./dist/bin/js from your build directory. Tests run a lot fastr if you are doing this on an optimized-build with debug; ../configure --enable-warnings-as-errors --disable-tests --enable-optimize --enable-debug)

Thank you for your reply.

I have a question about internEmptyGlobalScope.
When I put EmptyGlobalScopeType instead of Scope to the list, it won't work because of getScope.

I think EmptyGlobalScopeType needs to be a GCCellPtr or ScopeIndex type.

What should I do?

Oh dear. I hadn't considered that. Could you post your interim patch, and let me know what test you're seeing fail? I'll try to think about how we can address this ASAP;

(My immediate giving-it-almost-no-time-to-think answer is that this means we need to create a special ScopeCreationData (which is what the ScopeIndex points to) that will be able to answer the relevant queries, but then point to emptyGlobalScope instead of allocating a new one here . This would be instead of creating the EmptyGlobalScopeType; but it may depend on what the actual source of the getScope call is)

This problem definitely means this bug is more challenging than I thought: However, if you're still up for continuing, I'm still up for mentoring you through it. I'll be back online Monday.

All tests fail.

Assertion failure: is<T>(), at /Users/u7693/workspace/mozilla-central/js/src/build_DBG.OBJ/dist/include/mozilla/Variant.h:734

is<T>() seems to be called by elem.as<ScopeIndex>() in the getScope function.

Ok.

In the sober light of a Monday, I realize now my ScopeCreationIdea is total overkill. The restriction we are trying to maintain is that the -output- of a parse doesn't have GC things in it; however, in our current state it's sort of inevitable that we have references to external scopes during parse.

So, with that in mind, what I think the right path is: Let's just create an AbstractScope with cx->global()->emptyGlobalScope() at the call to getScope:

AbstractScopePtr getScope(size_t index) const {
  auto& elem = vector[index].get();
  if (elem.is<JS::GCCellPtr>()) {
    return AbstractScopePtr(&elem.as<JS::GCCellPtr>().as<Scope>());
  } else if (elem.is<EmptyGlobalScopeType>()) {
    return AbstractScopePtr(&compilationInfo.cx->global()->emptyGlobalScope());
  }
  return AbstractScopePtr(compilationInfo, elem.as<ScopeIndex>());
}

You'll likely have to move that method into the .cpp file, and add an include for vm/GlobalObject.h.

Side note: Looking at the patch, you'll notice the difference between your append method, and the one for Scope*; I suspect you'll run into issues there because you're not recalling the first scope index. An easy fix.

Thanks so much for working on this, and if you have any questions about background or motivation, please feel free to ask.

https://hg.mozilla.org/mozilla-central/rev/c68e50528d6c