1278303 - Make it possible to use fallible TempAllocator with MIR Instructions.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Nicolas B. Pierron [:nbp]]

The goal of this bug is to pave the way towards idiomatic OOM handling, which means that any call to a fallible allocator should be syntactically different than a call to an infallible allocator.  Thus, a reviewer can catch a mistake without reading extra context, and have the type of the arguments enforced by the type system.

Thus, if the ::New call is fallible, then this should be mirrored at the caller level.

  MFoo* f = MFoo::New(alloc.fallible(), a, b);
  if (!f)
      return false;

  MQux* q = MQux::New(alloc, a, b);

For convenience, any infallible allocation can also be allocated with the fallible version of the allocator, which is equivalent of folding the ensureBallast call into the instruction allocation.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Prevent uses of TempObject new operator on MIR Instructions.

This patch ensures that we have to use MFoo::New or any other member to
allocate MFoo objects.

Unfortunately, I do not see a proper way to handle fallible allocations of
the init functions, nor to expose both arguments of the init function and
the constructor argument without adding too much of a burden.  So, let's at
least ensure that we only support one way of allocating MIR instructions.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Add MInstruction::New(TempAllocator::Fallible, ...) overload to all trivial MIR Instructions.

This patch adds a new macro used to introduced the New members of MIR
instructions.  These macro are used to define an infallible New allocator
member, and a fallible New allocator members.

This patch also replaces redundant lines, when a trivial New function exist
without any overloaded definition.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8760354 [details] [diff] [review]
part 1 - Prevent uses of TempObject new operator on MIR Instructions.

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

Nice

::: js/src/jit/MIR.h
@@ +1021,5 @@
>  
> +  protected:
> +    // All MInstructions are using the "MFoo::New(alloc)" notation instead of
> +    // the TempObject new operator. This code redefine the new operator as
> +    // protected, and delegate to the TempObject new operator. Thus, the

Nit: s/redefine/redefines/, s/delegate/delegates/

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Use a macro to define a list of getOperand accessors.

While I was replacing redundant lines with macros, I thought that I could go
a bit futher ...

Thus, here is another simple patch which adds a macro to define all the
getOperands accessors of MIR Instructions.

Note, this new syntax is also quite helpful to easily catch copy&pasta
errors (as highlighted by the upcoming part 4)

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Fix correctness issue of MCreateThis new-target operand index.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8760357 [details] [diff] [review]
part 2 - Add MInstruction::New(TempAllocator::Fallible, ...) overload to all trivial MIR Instructions.

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

Nice!

::: js/src/jit/MIR.h
@@ +5130,5 @@
>  class MToFloat32
>    : public MToFPInstruction
>  {
>    protected:
> +    MToFloat32(MDefinition* def, ConversionKind conversion = NonStringPrimitives)

Nit: this now needs `explicit`

@@ +5406,5 @@
>  {
>      bool canBeNegativeZero_;
>      MacroAssembler::IntConversionInputKind conversion_;
>  
> +    MToInt32(MDefinition* def, MacroAssembler::IntConversionInputKind conversion =

And here.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8760825 [details] [diff] [review]
part 3 - Use a macro to define a list of getOperand accessors.

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

Cool.

::: js/src/jit/MIR.h
@@ +1122,5 @@
>  
> +
> +// These macros are used as a syntactic sugar for writting getOperand
> +// accessors. They are meant to be used in the body of MIR Instructions as
> +// follow:

Nit: s/follow/follows/

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8760825 [details] [diff] [review]
part 3 - Use a macro to define a list of getOperand accessors.

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

(drive by nit)

::: js/src/jit/MIR.h
@@ +7504,5 @@
>  
>    public:
>      INSTRUCTION_HEADER(LexicalCheck)
>      TRIVIAL_NEW_WRAPPERS
> +    NAMED_OPERANDS((0, input))

Actually this could be removed: MLexicalCheck is a MUnaryInstruction, which already defines input() as getOperand(0).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d0a284f6ff
part 1 - Prevent uses of TempObject new operator on MIR Instructions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c255a40804be
part 2 - Add MInstruction::New(TempAllocator::Fallible, ...) overload to all trivial MIR Instructions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9adef81e8d
part 3 - Use a macro to define a list of getOperand accessors. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/09359e9505b1
part 4 - Fix correctness issue of MCreateThis new-target operand index. r=efaust

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1264948 and bug 1278303 for Static failures in Ion.cpp and undefined oomTest in bug1269756.js:

bug 1264948:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b44d6438cf58c12c198161654ab0048b7c3bb29
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77e4ad223d4a46b6fa85c45347ce96891206b4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d72428c4f435d1dabcdcdb0a4b64f4c21f9a3fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2527718052423d722383ca11059332a1cb42ac

bug 1278303:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4b01b4e40764d1468575bd1e5e65cccaec0461
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c1a37d417ef991f2f0649da657e9796f242289
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb6b1e8a6eeef6685cc82120f7e38efb8c78423
https://hg.mozilla.org/integration/mozilla-inbound/rev/a071095ecfdc80533cd602309516d9a048a31255

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=09359e9505b1fbc25003046fa5ed213bb5b5863b

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec710e102b1
part 1 - Prevent uses of TempObject new operator on MIR Instructions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d1cf0313a3
part 2 - Add MInstruction::New(TempAllocator::Fallible, ...) overload to all trivial MIR Instructions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42076de2370
part 3 - Use a macro to define a list of getOperand accessors. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1f56dfff60
part 4 - Fix correctness issue of MCreateThis new-target operand index. r=efaust

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ec710e102b1
https://hg.mozilla.org/mozilla-central/rev/b2d1cf0313a3
https://hg.mozilla.org/mozilla-central/rev/e42076de2370
https://hg.mozilla.org/mozilla-central/rev/fd1f56dfff60