MarkExactStackRoot doesn't play nicely with struct-based rooted kinds

RESOLVED FIXED in mozilla32

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: efaust, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
MarkExactStackRoot was written with the original concept of Rooted in mind. It is meant to adequately mark pointers to movable GC things.

It has been pressed into service against its will as a marking mechanism for stack-based structs containing GC things. People (myself included) wanted to use the Rooted<Foo> idiom, even when Foo was not a GC thing.

This is bad, because MarkExactStackRoot expects it to be. The IsNullTaggedPointer() check operates on the deref of Rooted<void *>->address(). For rooteds as intended, this makes good sense. We don't want to mark a |nullptr| JSObject *, and what lives on the stack is a single word. For stack-based rooted structs, this makes no sense at all. It grabs the first word of the struct, and compares it against an arbitrary series of checks, and decides whether to trace the *whole struct* based on the results of those checks.

This is an enormous and intolerable footgun.
Seems to me like efaust is right. IsNullTaggedPointer really isn't valid for anything that isn't a Cell*. Which means we shouldn't be calling it for Value, jsid, Bindings, JSPropertyDescriptor, or THING_ROOT_CUSTOM. And we should make it harder to get this wrong.
Summary: MarkExactStackRoot doesn't play nicely with stack-based rooted kinds → MarkExactStackRoot doesn't play nicely with struct-based rooted kinds
Oh, and types::Type.
(Reporter)

Comment 3

4 years ago
Created attachment 8434617 [details] [diff] [review]
Proposed Fix
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8434617 - Flags: review?(sphink)
(Assignee)

Comment 4

4 years ago
(In reply to Eric Faust [:efaust] from comment #0)
Nice catch, this is just wrong.  We really should be using typed Rooted rather than Rooted<void*> where possible.
Comment on attachment 8434617 [details] [diff] [review]
Proposed Fix

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

I would probably spell this a little differently, by making a ShouldNotMarkRooter(kind, rooter) that handles both IsNullTaggedPointer and the TaggedProto::LazyProto thing internally.

But either way, it sounded like on IRC that jonco has a patch that avoids casting away the type info here, so I'm going to transfer review to him.
Attachment #8434617 - Flags: review?(sphink) → review?(jcoppeard)
(Assignee)

Comment 6

4 years ago
Created attachment 8435054 [details] [diff] [review]
type-root-marking

Here's a patch to type the marking code for Rooters.

This also removes the current RootedGeneric template and reimplements it as a subclass of CustomAutoRooter, which means we can get rid of the pointer arithmetic necessary to find the vtable.  I don't think this should be a problem.

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=fc80317acc72
Assignee: efaustbmo → jcoppeard
Attachment #8435054 - Flags: review?(sphink)
(Assignee)

Updated

4 years ago
Attachment #8434617 - Flags: review?(jcoppeard)
Comment on attachment 8435054 [details] [diff] [review]
type-root-marking

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

::: js/src/gc/RootMarking.cpp
@@ +72,5 @@
> +{
> +    return IsNullTaggedPointer(*thingp) || *thingp == TaggedProto::LazyProto;
> +}
> +
> +template <class T, void (*MarkFunc)(JSTracer *trc, T *ref, const char *name), class Source>

I think you should templatize on the function, not the function pointer, for MarkFunc.

@@ +109,5 @@
> +    MarkExactStackRootsForType<jsid, MarkIdRoot>(trc, "exact-id");
> +    MarkExactStackRootsForType<Value, MarkValueRoot>(trc, "exact-value");
> +    MarkExactStackRootsForType<types::Type, MarkTypeRoot>(trc, "exact-type");
> +    MarkExactStackRootsForType<Bindings, MarkBindingsRoot>(trc);
> +    MarkExactStackRootsForType<JSPropertyDescriptor, MarkPropertyDescriptorRoot>(trc);

Why no name for these last two?

::: js/src/jscntxt.cpp
@@ +265,5 @@
>  }
>  
> +#if defined(JSGC_USE_EXACT_ROOTING) && defined(DEBUG)
> +void
> +ContextFriendFields::checkNoGCRooters() {

I'd prefer the #if goop to only show up once. Can you always define checkNoGCRooters(), and #ifdef the body?
Attachment #8435054 - Flags: review?(sphink) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8435101 [details] [diff] [review]
type-root-marking v2

Patch with review comments applied.
Attachment #8434617 - Attachment is obsolete: true
Attachment #8435054 - Attachment is obsolete: true
Attachment #8435101 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

4 years ago
(In reply to Steve Fink [:sfink] from comment #7)
> > +    MarkExactStackRootsForType<Bindings, MarkBindingsRoot>(trc);
> > +    MarkExactStackRootsForType<JSPropertyDescriptor, MarkPropertyDescriptorRoot>(trc);
> 
> Why no name for these last two?

The existing marking mechanisms for these don't use the name (and it made the lines wrap) so I got rid of it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/846ee7c7debf

sfink pointed me to the Try run for this over IRC. In the future, please include the link at the time of requesting checkin.
Keywords: checkin-needed
Backed out for bustage that sfink says has his name all over it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e6b3caa5a2

https://tbpl.mozilla.org/php/getParsedLog.php?id=41133777&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41134045&tree=Mozilla-Inbound
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Backed out for bustage that sfink says has his name all over it.

Actually, now I'm unsure of the exact problem, since it's only failing the ICS emulator build. The problem is

static void foo() { ... }
template <void (F)()> { ... }

it doesn't want to instantiate that template because F doesn't have external linkage. If it were template <void (*F)()>, that would sort of make sense. Maybe. Well, not really. But I really don't understand why it has a problem with what's in the actual push, unless the extra parens around (F) are being interpreted as an implicit (*F) or something.

Anyway, I'm looking into it. Probably ought to figure out how to do the ics emulator builds locally. I've been doing some emulator-jb builds recently, but it looks like those are succeeding. :(
Ugh. I think the external linkage thing may be a red herring. I think the problem is that gcc 4.4 doesn't allow

template <typename T, void F(T&)> ...

(variadic template arguments?) where one template parameter T is part of the type of another template parameter F.
Attachment #8435101 - Flags: checkin+
Re-pushed, with template functions switched to non-static to appease gcc 4.4.

http://hg.mozilla.org/integration/mozilla-inbound/rev/7ffbe6899aed
(Assignee)

Comment 15

4 years ago
(In reply to Steve Fink [:sfink] from comment #14)
Thanks for sorting that out!
https://hg.mozilla.org/mozilla-central/rev/7ffbe6899aed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Updated

3 years ago
Blocks: 1132042
You need to log in before you can comment on or make changes to this bug.