Closed Bug 1013663 Opened 10 years ago Closed 10 years ago

Fix bad implicit conversion constructors in the JS engine

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files, 2 obsolete files)

Fix bad implicit conversion constructors in the JS engine
543.20 KB, patch
jandem
: review+
luke
: feedback-
Details | Diff | Splinter Review
Fix bad implicit conversion constructors in the JS engine
453.02 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Assignee: nobody → ehsan
Blocks: explicit
Comment on attachment 8425906 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

I'm so sorry about the size of the patch... There was no great way of splitting this up.  This moves the entire JS engine to make it compile with the analysis in bug 1009631 running.

The double-parenthesis pattern that I've used in parts of this patch is to disambiguate the syntax with that of function declarations.  Thanks for that, C++. :/

I know that this is a gigantic patch but I'd appreciate fast reviews, it already bitrotted once as I was working on it, and currently running it against try again.
Attachment #8425906 - Flags: review?(jorendorff)
Comment on attachment 8425906 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

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

::: js/src/jsapi.cpp
@@ +4666,5 @@
>  
>  JSScript *
>  JS::Compile(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options, FILE *fp)
>  {
> +    FileContents buffer((TempAllocPolicy(cx)));

This is kind of nonsense, if it's going to happen everywhere.  The very *point* of the AllocPolicy pattern is largely to allow passing an argument directly along to the AllocPolicy under the hood.  Is there a good reason we can't have constructors taking an AllocPolicy do perfect forwarding of the argument into the underlying policy's constructor, to get rid of this implicitness?  Really really don't want to see this nonsense showing up everywhere AllocPolicys are used.
Ehsan, thank you so much for doing this.

I will probably be asking for MOZ_IMPLICIT exceptions for AllocPolicy classes. Full review tomorrow.
Comment on attachment 8425906 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

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

At Jason's request, the GC bits look fine.

::: js/src/gc/Barrier.h
@@ +467,5 @@
>  class PreBarriered : public BarrieredBase<T>
>  {
>    public:
>      PreBarriered() : BarrieredBase<T>(GCMethods<T>::initial()) {}
> +    // Needs to be MOZ_IMPLICIT in order to make DebuggerWeakMap::markKeys() compile.

In general we have to be able to mark HashTable keys generically. Perhaps a better comment would be:

/* Allow implicit construction for use in generic contexts, such as DebuggerWeakMap::markKeys. */

::: js/src/jsapi.cpp
@@ +4666,5 @@
>  
>  JSScript *
>  JS::Compile(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options, FILE *fp)
>  {
> +    FileContents buffer((TempAllocPolicy(cx)));

Second what Jeff said. The fact that we construct an AllocPolicy from a MallocProvider is a bit deceptive, but this sort of verbosity is not the right solution.
Attachment #8425906 - Flags: review+
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Ehsan, thank you so much for doing this.

NP, and again, sorry that this looks like a patch bomb!  This was the first piece of the code base I touched here and didn't realize how extensive the changes will be until it was too late.

(In reply to Terrence Cole [:terrence] from comment #5)
> ::: js/src/jsapi.cpp
> @@ +4666,5 @@
> >  
> >  JSScript *
> >  JS::Compile(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options, FILE *fp)
> >  {
> > +    FileContents buffer((TempAllocPolicy(cx)));
> 
> Second what Jeff said. The fact that we construct an AllocPolicy from a
> MallocProvider is a bit deceptive, but this sort of verbosity is not the
> right solution.

OK, I can make the following MOZ_IMPLICIT: TempAllocPolicy, IonAllocPolicy, and RuntimeAllocPolicy.  If I'm missing any, please let me know!
D'oh, and also LifoAllocPolicy.
Comment on attachment 8425906 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

Working on a new patch...
Attachment #8425906 - Attachment is obsolete: true
Attachment #8425906 - Flags: review?(jorendorff)
And ContextAllocPolicy!
Attachment #8426230 - Flags: review?(jorendorff)
Comment on attachment 8426230 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

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

Thanks again, ehsan. r=me but with lots of comments. I think the biggest change I'm asking for is not adding `js::` where it'd be redundant with a `using namespace js;` declaration.

In class HandleValueArray:
>   public:
>-    HandleValueArray(const RootedValue& value) : length_(1), elements_(value.address()) {}
>+    MOZ_IMPLICIT HandleValueArray(const RootedValue& value) : length_(1), elements_(value.address()) {}

Please make this one signature explicit. It means adding an explicit
HandleValueArray() call in a bunch of places, but that's good pain.

It's fine for the other ones to be MOZ_IMPLICIT.

In jsarray.cpp, in js_InitArrayClass:
>-    RootedTypeObject type(cx, cx->getNewType(&ArrayObject::class_, proto.get()));
>+    Rooted<TaggedProto> taggedProto(cx, TaggedProto(proto));
>+    RootedTypeObject type(cx, cx->getNewType(&ArrayObject::class_, taggedProto.get()));

The extra Rooted here doesn't buy us anything. The type of the argument
to getNewType is just TaggedProto; that is, it the function takes a copy
of the pointer and doesn't root it. So just slap a TaggedProto() around
the argument and call it a day:

    RootedTypeObject type(cx, cx->getNewType(&ArrayObject::class_, TaggedProto(proto)));

(Definitely drop the .get() if C++ lets you.)

In NewArray:
>-    RootedTypeObject type(cxArg, cxArg->getNewType(&ArrayObject::class_, proto.get()));
>+    Rooted<TaggedProto> taggedProto(cxArg, TaggedProto(proto));
>+    RootedTypeObject type(cxArg, cxArg->getNewType(&ArrayObject::class_, taggedProto));

Same here.

In js/src/jscompartmentinlines.h:
>-    global_ = &global;
>+    global_ = js::ReadBarriered<js::GlobalObject*>(&global);

Instead, write:  global_.set(&global);

The same thing happens in a bunch of other places, all involving ReadBarriered.

In js/src/jsinfer.cpp:
>         /* Make a new type to use for future arrays with the same elements. */
>         RootedObject objProto(cx, obj->getProto());
>-        TypeObject *objType = newTypeObject(cx, &ArrayObject::class_, objProto);
>+        Rooted<js::TaggedProto> taggedProto(cx, js::TaggedProto(objProto));
>+        TypeObject *objType = newTypeObject(cx, &ArrayObject::class_, taggedProto);

Rooting a TaggedProto unnecessarily again. But also: almost all cpp
files under js/src do `using namespace js;`. Please avoid using `js::` in
those files.

There are quite a few more similar places; no point mentioning them all.

In js/src/jswrapper.cpp:
>-    wcompartment->putWrapper(cx, ObjectValue(*newTarget), ObjectValue(*wobj));
>+    wcompartment->putWrapper(cx, CrossCompartmentKey(ObjectValue(*newTarget)), ObjectValue(*wobj));

CrossCompartmentKey has a constructor that takes a JSObject* directly,
so you can just `CrossCompartment(newTarget)`.

In js/src/jswrapper.h:
>     WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),
>-                                    proto_()
>+                                             proto_()

Hmm. This indentation-only change looks wrong to me.

In js/src/vm/Runtime.h:
>-        AutoLockForInterrupt(JSRuntime *rt MOZ_GUARD_OBJECT_NOTIFIER_PARAM) : rt(rt) {
>+        AutoLockForInterrupt(JSRuntime *rt MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
>+            : rt(rt) {

Please revert this or else put the brace on its own line.

In js/src/vm/Shape.cpp:
>-    Rooted<types::TypeObject *> type(cx, cx->getNewType(clasp, proto.get()));
>+    Rooted<types::TypeObject *> type(cx, cx->getNewType(clasp, js::TaggedProto(proto.get())));

See if you can skip the .get() here.

>-    entry.shape = shape.get();
>+    entry.shape = ReadBarrieredShape(shape.get());

And here.

>-                    InitialShapeEntry newKey(shape, proto);
>+                    InitialShapeEntry newKey((ReadBarrieredShape(shape)), js::TaggedProto(proto));

Wow.

Another way to avoid the warning, I guess, is to declare the
ReadBarrieredShape as a separate variable on the previous line... but I
guess both ways are at least a little gruesome.
Attachment #8426230 - Flags: review?(jorendorff) → review+
Oh - there's one other place where you're calling CrossCompartmentKey(ObjectValue(_)), and the ObjectValue can be dropped. It's in js/src/jsapi.cpp.
Comment on attachment 8426230 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

Oh yeah, one more important caveat --- I didn't look at anything in js/src/jit or js/src/assembler; jandem is going to look at those separately. This can't land ahead of his review.
Attachment #8426230 - Flags: review?(jdemooij)
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> In js/src/jsinfer.cpp:
> >         /* Make a new type to use for future arrays with the same elements. */
> >         RootedObject objProto(cx, obj->getProto());
> >-        TypeObject *objType = newTypeObject(cx, &ArrayObject::class_, objProto);
> >+        Rooted<js::TaggedProto> taggedProto(cx, js::TaggedProto(objProto));
> >+        TypeObject *objType = newTypeObject(cx, &ArrayObject::class_, taggedProto);
> 
> Rooting a TaggedProto unnecessarily again.

Well, newTypeObject() takes a Handle<TaggedProto>.  How can I avoid rooting here?  What you're suggesting here gives me |no viable conversion from 'js::TaggedProto' to 'Handle<js::TaggedProto>'|.
Flags: needinfo?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> In js/src/jswrapper.h:
> >     WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),
> >-                                    proto_()
> >+                                             proto_()
> 
> Hmm. This indentation-only change looks wrong to me.

Hmm, this is the diff:

-    WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),
-                                    proto_()
+    explicit WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),
+                                             proto_()

I'm just preserving the indentation.  What would you like me to do with that?
Oh, and do you still want me to make AbstractFramePtr MOZ_IMPLICIT?
Attachment #8426230 - Attachment is obsolete: true
Attachment #8426230 - Flags: review?(jdemooij)
Attachment #8427462 - Flags: review?(jdemooij)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #16)
> Oh, and do you still want me to make AbstractFramePtr MOZ_IMPLICIT?

Not speaking for Jason, but I think that'd be good. I initially made its constructors explicit but Luke asked me to make them implicit because there's no potential risk and it simplifies the code that uses them.

I'll review this patch in a bit.
Comment on attachment 8427462 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

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

Jan asked me to review some parts of this patch.  Lots of good explicits here, but a couple that need to be reverted:

::: js/public/CharacterEncoding.h
@@ +150,5 @@
>  class ConstTwoByteChars : public mozilla::RangedPtr<const jschar>
>  {
>    public:
>      ConstTwoByteChars(const ConstTwoByteChars &s) : ConstCharPtr(s) {}
> +    explicit ConstTwoByteChars(const mozilla::RangedPtr<const jschar> &s) : ConstCharPtr(s) {}

Implicit

::: js/public/Value.h
@@ +1250,5 @@
>      jsval_layout data;
>  
>    private:
>  #if defined(JS_VALUE_IS_CONSTEXPR)
> +    explicit JS_VALUE_CONSTEXPR Value(jsval_layout layout) : data(layout) {}

Implicit

::: js/src/jit/AsmJS.cpp
@@ +386,5 @@
>      Which which_;
>  
>    public:
>      Type() : which_(Which(-1)) {}
> +    explicit Type(Which w) : which_(w) {}

Implicit

@@ +498,5 @@
>  
>    public:
>      RetType() : which_(Which(-1)) {}
> +    explicit RetType(Which w) : which_(w) {}
> +    explicit RetType(AsmJSCoercion coercion) {

and these

@@ +576,2 @@
>        : which_(w) {}
> +    explicit VarType(AsmJSCoercion coercion) {

and these

::: js/src/jit/AsmJSModule.h
@@ +298,5 @@
>      {
>          PropertyName *name_;
>        public:
>          Name() : name_(nullptr) {}
> +        explicit Name(PropertyName *name) : name_(name) {}

and this

::: js/src/vm/Stack.h
@@ +116,5 @@
>      AbstractFramePtr()
>        : ptr_(0)
>      {}
>  
> +    explicit AbstractFramePtr(InterpreterFrame *fp)

I think all the AbstractFramePtrs can stay implicit.

@@ +1599,5 @@
>      FrameIter(ThreadSafeContext *cx, ContextOption, SavedOption);
>      FrameIter(JSContext *cx, ContextOption, SavedOption, JSPrincipals *);
>      FrameIter(const FrameIter &iter);
> +    explicit FrameIter(const Data &data);
> +    explicit FrameIter(AbstractFramePtr frame);

These two can stay implicit
Attachment #8427462 - Flags: feedback-
Comment on attachment 8427462 [details] [diff] [review]
Fix bad implicit conversion constructors in the JS engine

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

r=me on the jit/ and assembler/ changes, with Luke's comments addressed too.

::: js/src/assembler/assembler/AbstractMacroAssembler.h
@@ +123,5 @@
>              , offset(0)
>          {
>          }
>  
> +        explicit ImplicitAddress(Address address)

This ImplicitAddress class is confusing, the implicit constructors here are by design, but I think it's a bad idea. Please make this constructor MOZ_IMPLICIT and let's leave the other constructor, ImplicitAddress(reg), explicit, because implicitly treating a register as an address is error prone.

The good news is that all this code is only used by Yarr, and Yarr is on its way out (will likely be removed in the next cycle), so there's no point removing this class now.

::: js/src/jit/RegisterSets.h
@@ +230,5 @@
>      {
>          dataValue() = value;
>      }
>  
> +    explicit ConstantOrRegister(TypedOrValueRegister reg)

Make the ConstantOrRegister and TypedOrValueRegister constructors implicit. It avoids some unnecessary verbosity.
Attachment #8427462 - Flags: review?(jdemooij) → review+
(In reply to Luke Wagner [:luke] from comment #19)
> Jan asked me to review some parts of this patch.  Lots of good explicits
> here, but a couple that need to be reverted:

I'd like to understand your reasoning better. The guideline we're applying here is that constructors should be explicit unless there's a reason for them to be implicit. So in particular:

> > +    explicit JS_VALUE_CONSTEXPR Value(jsval_layout layout) : data(layout) {}
> 
> Implicit

I don't think there are any implicit uses of this constructor in the codebase. Why make it implicit?

> ::: js/src/vm/Stack.h
> > +    explicit AbstractFramePtr(InterpreterFrame *fp)
> 
> I think all the AbstractFramePtrs can stay implicit.

Here I agree. Pointer conversions from concrete frame types to AbstractFramePtr are effectively Derived* to Base* conversions. It's totally sensible for those to be implicit.

Plus these conversions are actually used implicitly in something like 49 places.


> @@ +1599,5 @@
> >      FrameIter(ThreadSafeContext *cx, ContextOption, SavedOption);
> >      FrameIter(JSContext *cx, ContextOption, SavedOption, JSPrincipals *);
> >      FrameIter(const FrameIter &iter);
> > +    explicit FrameIter(const Data &data);
> > +    explicit FrameIter(AbstractFramePtr frame);
> 
> These two can stay implicit

Again, I don't think these are called implicitly anywhere. It seems like they should be explicit.

All your comments refer to code you understand way better than I do :) so I'm happy to go with your preference. I would like to understand better though.
Flags: needinfo?(jorendorff)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> > Rooting a TaggedProto unnecessarily again.
> 
> Well, newTypeObject() takes a Handle<TaggedProto>.  How can I avoid rooting
> here?

Ignore my comment.

> -    WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),
> -                                    proto_()
> +    explicit WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),
> +                                             proto_()
> 
> I'm just preserving the indentation.  What would you like me to do with that?

Definitely ignore my comment on this one.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #16)
> Oh, and do you still want me to make AbstractFramePtr MOZ_IMPLICIT?

Yes, definitely.
(In reply to Jason Orendorff [:jorendorff] from comment #22)
> > > +    explicit JS_VALUE_CONSTEXPR Value(jsval_layout layout) : data(layout) {}
> > 
> > Implicit
> 
> I don't think there are any implicit uses of this constructor in the
> codebase. Why make it implicit?

Yes, there aren't.

> > ::: js/src/vm/Stack.h
> > > +    explicit AbstractFramePtr(InterpreterFrame *fp)
> > 
> > I think all the AbstractFramePtrs can stay implicit.
> 
> Here I agree. Pointer conversions from concrete frame types to
> AbstractFramePtr are effectively Derived* to Base* conversions. It's totally
> sensible for those to be implicit.
> 
> Plus these conversions are actually used implicitly in something like 49
> places.

Yep.  I already made AbstractFramePtr's ctor implicit.

> > @@ +1599,5 @@
> > >      FrameIter(ThreadSafeContext *cx, ContextOption, SavedOption);
> > >      FrameIter(JSContext *cx, ContextOption, SavedOption, JSPrincipals *);
> > >      FrameIter(const FrameIter &iter);
> > > +    explicit FrameIter(const Data &data);
> > > +    explicit FrameIter(AbstractFramePtr frame);
> > 
> > These two can stay implicit
> 
> Again, I don't think these are called implicitly anywhere. It seems like
> they should be explicit.

Yes, these two are also never used implicitly.
I'm almost ready to land this, but will wait for Luke's response first...
Flags: needinfo?(luke)
Sorry, I landed the patch since I had to modify it a few times as people changed things over the weekend and today, and don't want to keep running this on try any more...  I promise to quickly address any follow-up work when Luke gets back to us in a follow-up though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5021d1337fa9
Landing it was the right thing. Thanks for all your work on this, Ehsan.
Looks good, Ehsan, thanks for addressing my comments!

(In reply to Jason Orendorff [:jorendorff] from comment #22)
> (In reply to Luke Wagner [:luke] from comment #19)
> > Jan asked me to review some parts of this patch.  Lots of good explicits
> > here, but a couple that need to be reverted:
> 
> I'd like to understand your reasoning better. The guideline we're applying
> here is that constructors should be explicit unless there's a reason for
> them to be implicit. So in particular:

The internal guideline I use is that implicit constructors express a subtyping relationship between the constructed type (the supertype) and the argument type (the subtype) and, if you start with the argument type, you shouldn't be surprised if it ends up with a subtype.  Both the Value(jsval_layout) and FrameIter(AbstractFramePtr) ctors seem to satisfy this categorization which is why I recommended their being left implicit.  It's not a big deal, though; this is all quite subjective.
Flags: needinfo?(luke)
https://hg.mozilla.org/mozilla-central/rev/5021d1337fa9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Thanks guys!  If I need to do any follow-up work, please let me know. :-)
(In reply to Luke Wagner [:luke] from comment #28)
> The internal guideline I use is that implicit constructors express a
> subtyping relationship between the constructed type (the supertype) and the
> argument type (the subtype) and, if you start with the argument type, you
> shouldn't be surprised if it ends up with a subtype.

Hmm. That's pretty much exactly the guideline I use, though I'd add a Third Law: "weird stuff shall not happen implicitly". We shouldn't have implicit constructors for conversions that aren't perfectly normal boring things. All weirdness is worth an explicit T() in the code.

> Both the
> Value(jsval_layout) and FrameIter(AbstractFramePtr) ctors seem to satisfy
> this categorization which is why I recommended their being left implicit.

Yeah. In the case of Value(jsval_layout) it would be weird--and surely a mistake, though not a bug--for someone to be making jsval_layouts and trying to pass them around where Values are expected.

The FrameIter(AbstractFramePtr) case is different; it just doesn't read like a subtyping relationship to me. Actually... I wonder if we could unify those two types somehow...

> It's not a big deal, though; this is all quite subjective.

Certainly true! Thanks for clarifying though.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: