1116591 - Actually give a few MIR opcodes a type policy where they clearly intended to have one but failed to say the magic words correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jeff Walden [:Waldo]]

Various MIR instructions clearly-inadvertently are lacking type policies

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Don't have testcase(s) yet, will try to scrounge some up before landing.  Not sure if this'll need backporting yet -- MSubstr is the only one that might be riding release trains outside nightly at this point.

I have more patches to make all TypePolicys final to prevent this mistake, and to force every MIR instruction to affirmatively say it doesn't want a type policy if indeed it doesn't, but those can wait a little past this immediate bug-fix.  (They indicated that these three are the only instructions mistakenly inheriting from TypePolicys instead of from ::Data, tho.)

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8542696 [details] [diff] [review]
Patch

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

::: js/src/jit/MIR.h
@@ +12049,4 @@
>  
>  class MCompareExchangeTypedArrayElement
>    : public MAryInstruction<4>,
> +    public MixPolicy< MixPolicy<ObjectPolicy<0>, IntPolicy<1> >, MixPolicy<IntPolicy<2>, IntPolicy<3> > >::Data

nit: Mix4Policy ?

::: js/src/jit/TypePolicy.cpp
@@ +993,4 @@
>      _(Mix3Policy<ObjectPolicy<0>, ObjectPolicy<1>, IntPolicy<2> >)      \
>      _(Mix3Policy<StringPolicy<0>, ObjectPolicy<1>, StringPolicy<2> >)   \
>      _(Mix3Policy<StringPolicy<0>, StringPolicy<1>, StringPolicy<2> >)   \
> +    _(Mix3Policy<StringPolicy<0>, IntPolicy<1>, IntPolicy<2>>)          \

nit: Keep this list sorted, by moving it up by 2 lines.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Add a Mix4Policy template class, and switch a MixPolicy of MixPolicys over to it

Delta patch for adding Mix4Policy, posted in case anyone wants to see it -- trivial enough I'm not going to bother asking for review on it.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Make all TypePolicy subclasses final to prevent the mistake of inheriting from one (rather than inheriting from its nested Data class)

With the next patch the finality here is kind of belt-and-suspenders, but it seems nice anyway.  The more safeguards against inheriting from the wrong thing, the better.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Make every MIR opcode's typePolicy() member function consult op::thisTypePolicy(), never the global thisTypePolicy() method

The sibling thisTypeSpecialization() looks like it'd want similar treatment, but it appears vastly rarer to actually call that, not immediately obvious what instructions would have it called -- and finally, if the global one's called we just crash.  Not great, but also not prone to badness.  If someone else wants to vacuum that up into the right fix, props to 'em, but I'm not touching it.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8542711 [details] [diff] [review]
Make all TypePolicy subclasses final to prevent the mistake of inheriting from one (rather than inheriting from its nested Data class)

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

::: js/src/jit/TypePolicy.h
@@ +68,1 @@
>      static MDefinition *alwaysBoxAt(TempAllocator &alloc, MInstruction *at, MDefinition *operand);

I think it would make sense to remove the BoxInputs class, and just define alwaysBoxAt outside of it to make it usable in IonAnalysis.cpp.  Otherwise, it can be made a static member of the TypePolicy base class.

@@ +68,2 @@
>      static MDefinition *alwaysBoxAt(TempAllocator &alloc, MInstruction *at, MDefinition *operand);
> +    static bool adjustInputs(TempAllocator &alloc, MInstruction *def);

Is this function still used?

@@ +73,3 @@
>  {
>    public:
>      SPECIALIZATION_DATA_;

to-fix: Use EMPTY_DATA_ instead of SPECIALIZATION_DATA_, as the adjustInputs function for this type policy does not on any specialization() field hold by the MIR instruction which has this type policy.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8542712 [details] [diff] [review]
Make every MIR opcode's typePolicy() member function consult op::thisTypePolicy(), never the global thisTypePolicy() method

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

::: js/src/jit/MIR.h
@@ +980,5 @@
>  };
>  
> +class MNullaryInstruction
> +  : public MAryInstruction<0>,
> +    public NoTypePolicy

Do you think it would make sense to have NoTypePolicy::Data instead, to make it match others TypePolicy definitions?

::: js/src/jit/TypePolicy.h
@@ +62,4 @@
>  
>  #define SPECIALIZATION_DATA_ INHERIT_DATA_(TypeSpecializationData)
>  
> +class NoTypePolicy

This is pure cosmetic, but I would expect NoTypePolicy to inherit from TypePolicy, but this might be me reading too many bad class inheritance :P  Still I think that "NoType" is a form of "TypePolicy".

@@ +65,5 @@
> +class NoTypePolicy
> +{
> +  public:
> +    static TypePolicy *thisTypePolicy() {
> +        return nullptr;

In which case it make sense to have this function defined in a Data structure.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59f8e01e7133
https://hg.mozilla.org/mozilla-central/rev/92c8c395677f
https://hg.mozilla.org/mozilla-central/rev/d3a71d1c2180
https://hg.mozilla.org/mozilla-central/rev/d9557e125ac3

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Do you think it would make sense to have NoTypePolicy::Data instead, to make
> it match others TypePolicy definitions?

I did it this way at first, then changed to no ::Data.  Still kinda waffly about it, so I switched to ::Data when landing.

> This is pure cosmetic, but I would expect NoTypePolicy to inherit from
> TypePolicy, but this might be me reading too many bad class inheritance :P 
> Still I think that "NoType" is a form of "TypePolicy".

It's explicitly claiming *not* to be a type policy, I think.  But yeah, there is a minor contention that could be had here.  Given NoTypePolicy should never be instantiated, seems worth not having the inheritance, and pointless extra vtable, &c.