1286505 - Use MFBT Result<T, E> in IonBuilder.

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

Bug 1285217 added a simple ResultWithOOM, as a temporary solution for the mfbt equivalent.

We should replace ResultWithOOM with its MFBT equivalent, and use MOZ_TRY to handle the returned values.

Comment 1

[Nicolas B. Pierron [:nbp]]

In addition to ResultWithOOM which is already in place, we should also replace the abortReason_ from IonBuilder, and carry it as part of the returned value, as discussed in Bug 1303399 comment 7 (and before).

Comment 2

[Hannes Verschore [:h4writer]]

Merge has happened. Moving the bugs from P2 to P1.

Comment 3

[Hannes Verschore [:h4writer]]

Bug 1310155 has landed and seems to stick. I didn't hear Gary panic and the one issue I already got seems very benign. As a result I think this bug can go forward building on that big change to IonBuilder.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Convert AbortReason enum to an enum class.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Add Result<V,E>::fwdErr() function, to create a different Result type with the same error.

This patch adds the Result<V,E>::fwdErr() function, which extracts the error
to make it a generic error value, such that it can be reused implictly in
the constructor of Result<U,E> (for any U).

The current way of doing so involves doing:
  return ::mozilla::MakeGenericErrorResult(res.unwrapErr());

with the following function, we can do:
  return res.fwdErr();

In Rust, the equivalent would be either with a pattern matching:
  Err(e) => return Err(e)

or with an unwrap:
  return Err(res.unwrap_err())

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Use Result<V,E> to report errors within IonBuilder.

(not asking for feedback/review yet, I have to rebase and push to try first)

This patch replaces the 3 different error codes from MIRGenerator and
replaces them by one, which is for the moment only used for reporting
off-main-thread errors.

This patch wrap all error values with the "abort" function which is in
charge of making a generic error with an AbortReason, and an optional
message.

Most of the changes are in IonBuilder, and we can distinguish multiple
cases, that can be seen from the return type changes:
  * MOZ_MUST_USED bool --> AbortReasonOr<Ok>
    This is the most common, false means error, true means all-good.
  * MOZ_MUST_USED bool --> AbortReasonOr<bool>
    Testing function which can have alloc issues / errors.
  * MOZ_MUST_USED bool --> bool
    Testing function with no abort reasons.
  * Foo* --> AbortReason<Foo*>
    Some function which used nullptr as an error code, which also needs
    to carry an AbortReason as a result.

MIR.h had to be modified to make the Result<> type works on abstract
classes, to deduce the Alignment.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Use Result<V,E> to report errors within IonBuilder.

Delta:
 - Use AbortReason::Disable, for cases where it used to be abort() without
   specific codes.
 - Fix tryFoldInstanceOf, which is an emitted-like function, based on the
   caller uses (jsop_instanceof).
 - LoweringShared::errored query the state which is stored on the
   MIRGenerator instead of the new flag which got added to the
   LoweringShared.

This modification make all the jit-test (--ion) pass on my laptop with a
debug build.  Sending the changes to the try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f386c36938380d170f84f9a77b4887457476186e

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Use Result<V,E> to report errors within IonBuilder.

Delta:
 - Add missing MOZ_TRY on loadTypedObjectData and loadTypedObjectElements
   calls. (reported by the last try-push)
 - Add the js::jit::Err function, as a shorter name for
   mozilla::MakeGenericErrorResult, until a similar change is accepted in
   mfbt. (we no longer need the existing part 2, which is adding fwdErr())

Try push without the Err function:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54b8760054c29fd033747b51d8409d90dfbb904

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8819946 [details] [diff] [review]
part 1 - Convert AbortReason enum to an enum class.

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

Looking forward to the rest. Thanks!

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8820711 [details] [diff] [review]
part 3 - Use Result<V,E> to report errors within IonBuilder.

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

The patch contains mostly expected changes. Big patch though. For the sake of myself, can you update/discuss the raised issues and resubmit. I'll take a second look, before r+'ing

1) Can we keep the lowering changes out of this patch and a next patch that fixes it totally? It seems that the lowering part in this patch is not ready yet. Or finish all the todo's which are in the lowering part.
2) Nice work on adding all the missing ballast places.

::: js/src/jit/IonBuilder.cpp
@@ +858,4 @@
>  
> +    // :TODO: Remove these lines.
> +    //if (shouldForceAbort())
> +    //    return abort(AbortReason::Disable);

Yes indeed. And also the function shouldForceAbort and the variable.

@@ +1012,4 @@
>  
> +    // :TODO: Remove these lines.
> +    // if (shouldForceAbort())
> +    //     return abort(AbortReason::Disable);

ditto

@@ +3624,1 @@
>          }

Best to keep the removal for follow-up patch? Lets keep this patch neutral in behavioral change

@@ +3640,5 @@
>              const ObjectGroupVector& groups = inlineBuilder.abortedPreliminaryGroups();
>              MOZ_ASSERT(!groups.empty());
>              for (size_t i = 0; i < groups.length(); i++)
>                  addAbortedPreliminaryGroup(groups[i]);
> +            return Err(result.unwrapErr());

Why doesn't:
"return result.unwrapErr()" not work here?

return AbortReason::PreliminaryObjects; should work right?

@@ +3646,5 @@
> +
> +          case AbortReason::Alloc:
> +          case AbortReason::Inlining:
> +          case AbortReason::Error:
> +            return Err(result.unwrapErr());

ditto

::: js/src/jit/MIR.h
@@ +14271,5 @@
> +
> +template <>
> +class AlignmentFinder<js::jit::MInstruction> : public AlignmentFinder<js::jit::MStart>
> +{
> +};

This isn't used yet? Please keep this out of this patch.

::: js/src/jit/MIRGenerator.h
@@ -171,5 @@
>      const CompileInfo* info_;
>      const OptimizationInfo* optimizationInfo_;
>      TempAllocator* alloc_;
>      MIRGraph* graph_;
> -    AbortReason abortReason_;

https://www.youtube.com/watch?v=mxD-5z_xHBU

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #10)
> 1) Can we keep the lowering changes out of this patch and a next patch that
> fixes it totally? It seems that the lowering part in this patch is not ready
> yet. Or finish all the todo's which are in the lowering part.

Previously, the MIRGenerator::abort function was setting the error_ flag, which was used to carry the fact that off-thread-compilation failed.  With this patch, I replaced the abort function to return this value instead of setting any flag.

To avoid making an even larger patch, I decided that I would rename the flag and only use it for the Lowering-shared.h, which explains why we have these new abort functions in the LoweringShared class, and why I had to go over the gen->abort functions to make them use the new LoweringShared::abort which replicate the old behaviour.

The TODOs are indeed for a follow-up patch similar to the current one, but dedicated to the Lowering.
What I can do, is open a follow-up bug and remove these TODO while keeping the current modifications.

> 2) Nice work on adding all the missing ballast places.

Unfortunately, I do not think this patch added all the missing ballast, but this would prevent us from ignoring OOM, and make it easier to not forget any caller.

As part of getting rid of these ignored OOM, I will have to change the way our MInstruction::New are working such that fallible New constructor returns this AbortReasonOr<MFoo*>, this way the type system will enforce that we do not forget any, but this would be due in a follow-up patch as well.

> ::: js/src/jit/IonBuilder.cpp
> @@ +858,4 @@
> >  
> > +    // :TODO: Remove these lines.
> > +    //if (shouldForceAbort())
> > +    //    return abort(AbortReason::Disable);
> 
> Yes indeed. And also the function shouldForceAbort and the variable.

I will.
I already removed the setForceAbort and the shouldForceAbort function in the current patch.

> @@ +3624,1 @@
> >          }
> 
> Best to keep the removal for follow-up patch? Lets keep this patch neutral
> in behavioral change
> 
> @@ +3640,5 @@
> >              const ObjectGroupVector& groups = inlineBuilder.abortedPreliminaryGroups();
> >              MOZ_ASSERT(!groups.empty());
> >              for (size_t i = 0; i < groups.length(); i++)
> >                  addAbortedPreliminaryGroup(groups[i]);
> > +            return Err(result.unwrapErr());
> 
> Why doesn't:
> "return result.unwrapErr()" not work here?
> 
> return AbortReason::PreliminaryObjects; should work right?

In both case this does not work because the Result has an implicit constructor for the Value, but it has an explicit constructor for the error value.

On the other hand, it does have an implicit constructor for GenericErrorResult<AbortReason>, which is what the "Err" function converts its input into.  This is less verbose and mimic what Rust code would do.

On a side note, I usually prefer to go with the original error code when possible, unless a new error arise.  This avoid making typos, and copy & pasta errors.

> ::: js/src/jit/MIR.h
> @@ +14271,5 @@
> > +
> > +template <>
> > +class AlignmentFinder<js::jit::MInstruction> : public AlignmentFinder<js::jit::MStart>
> > +{
> > +};
> 
> This isn't used yet? Please keep this out of this patch.

This is needed because Result<MInstruction*, AbortReason> use the mozilla::detail::HasFreeLSB class which relies on the MOZ_ALIGNOF macro which is converted into the AlignmentFinder class.  As MInstruction* and MDefinition* are abstract classes (some virtual pure methods), this is a temporary work-around to teach the alignment finder how to deal with these abstract classes.

The reason this fails with the AlignmentFinder is that it uses an union and splat the given pointy-type into the union.  Thus, the C++ compiler does not know how to instantiate this class.

> ::: js/src/jit/MIRGenerator.h
> @@ -171,5 @@
> >      MIRGraph* graph_;
> > -    AbortReason abortReason_;
> 
> https://www.youtube.com/watch?v=mxD-5z_xHBU

I don't know if I should call my self The Doctor, or just a Dalek?

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Use Result<V,E> to report errors within IonBuilder.

Delta:
 - Remove TODOs
 - open follow-up bugs: Bug 1325384, Bug 1325386

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - IonBuilder::jsop_getname, remove useless push & pop.

Minor clean-up that I noticed while doing this big rewrite.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8821178 [details] [diff] [review]
part 3 - IonBuilder::jsop_getname, remove useless push & pop.

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

Hehe, good find.

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8821177 [details] [diff] [review]
part 2 - Use Result<V,E> to report errors within IonBuilder.

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

Thanks for doing this!

Comment 16

[Nicolas B. Pierron [:nbp]]

Rebased changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d70d183cd3f7b866c3c516c624f19a4d442ef76

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed792a02180
part 1 - Convert AbortReason enum to an enum class. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eca45c6fb2d
part 2 - Use Result<V,E> to report errors within IonBuilder. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/254a2e98cc64
part 3 - IonBuilder::jsop_getname, remove useless push & pop. r=h4writer

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8ed792a02180
https://hg.mozilla.org/mozilla-central/rev/0eca45c6fb2d
https://hg.mozilla.org/mozilla-central/rev/254a2e98cc64