Dart Compiler removes declaration of Finalizable class

[BenoitOutman]

Hi, from dart 3.0 I have a problem with compiler and class who implements Finalizable

import 'dart:ffi';

class A implements Finalizable {}

void main() {
  A a;

  a = A();
}

this code launch on Android in release mode result to a crash:
E/flutter (24865): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Attempt to execute code removed by Dart AOT compiler (TFA)
E/flutter (24865): #0 TextBox.hashCode (dart:ui/text.dart)
E/flutter (24865): #1 _runMain. (dart:ui/hooks.dart:131)
E/flutter (24865): #2 _delayEntrypointInvocation. (dart:isolate-patch/isolate_patch.dart:296)
E/flutter (24865): #3 _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189)

The dart compiler as deleted the line "A a;"
an easy fix is to add late before "late A a;"

I have try on ios, and no problem.

Thank you.

[alexmarkov]

FFI/Finalizable transformer converts this code into the following kernel:

  static method main() → void {
    foo::A a;
    a = block {
      final synthesized foo::A :expressionValueWrappedFinalizable = new foo::A::•();
      _in::reachabilityFence(a);
    } =>:expressionValueWrappedFinalizable;
    _in::reachabilityFence(a);
  }

The first reachabilityFence(a) reads non-nullable variable a before it is assigned, which doesn't look correct. As a result, TFA concludes that this code is unreachable and removes it (replaces with throwing an error). We should fix the FFI/Finalizable transformer to avoid adding reachability fences until the variable is assigned if it has a non-nullable type (it is allowed to read a nullable variable before it is assigned, but front-end statically guarantees that non-nullable variable should not be read before assigned).

/cc @dcharkes

[dcharkes]

The first reachabilityFence(a) reads non-nullable variable a before it is assigned, which doesn't look correct.

This looks suspiciously similar to reading late variables before assigned. That we solved by making a second variable that is nullable.

We should fix the FFI/Finalizable transformer to avoid adding reachability fences until the variable is assigned if it has a non-nullable type (it is allowed to read a nullable variable before it is assigned, but front-end statically guarantees that non-nullable variable should not be read before assigned).

I'm not sure if that is possible, what about code such as:

MyFinalizable a;
if (someCondition) {
  a = MyFinalizable();
}
if (someOtherCondition) {
  somethingThatNeedsAReachabilityFence(); // some FFI call, or something that can throw, or ...
  // What do we do here?
}
a = MyFinalizable();
// ...

Maybe we can't have such cases, but I can't convince myself why.

[alexmarkov]

Tried the following code:

void main() {
  A a;
  if (int.parse('1') == 1) {
    a = A();
  }
  if (int.parse('2') == 2) {
    print('hi');
    return;
  }
  a = A();
}

This results in the following kernel:

  static method main() → void {
    foo::A a;
    if(core::int::parse("1") =={core::num::==}{(core::Object) → core::bool} 1) {
      a = block {
        final synthesized foo::A :expressionValueWrappedFinalizable = new foo::A::•();
        _in::reachabilityFence(a);
      } =>:expressionValueWrappedFinalizable;
    }
    if(core::int::parse("2") =={core::num::==}{(core::Object) → core::bool} 2) {
      core::print("hi");
      {
        _in::reachabilityFence(a);
        return;
      }
    }
    a = block {
      final synthesized foo::A :expressionValueWrappedFinalizable = new foo::A::•();
      _in::reachabilityFence(a);
    } =>:expressionValueWrappedFinalizable;
    _in::reachabilityFence(a);
  }

So, it looks like FFI/Finalizer transformer may need to add reachabilityFence at an arbitrary place, even where the variable is not used and statically it is not guaranteed to be initialized (but may be initialized).

In such cases we can resort to a nullable shadow copy of the variable, much like late. Or maybe we should admit that kernel AST/transformation is not suited for adding reachability fences and move this whole logic to the flow graph (where it would probably be much simpler).

[dcharkes]

Or maybe we should admit that kernel AST/transformation is not suited for adding reachability fences and move this whole logic to the flow graph (where it would probably be much simpler).

That was @mraleph's idea also, but I had a hard time proving too myself we wouldn't do premature optimizations in kernel that would violate reachability.

In such cases we can resort to a nullable shadow copy of the variable, much like late.

It's a single pass transform, so we don't see those cases up front, unless we make the transform more advanced.

We can apply the nullable shadow copy always, that would work. (Though we end up in the same conversation as last time, whether this would increase register pressure because we now get two variables.) At least it would keep everything consistent.

[mraleph]

Users are starting to hit this, we need to look at fixing it.

[dcharkes]

Or maybe we should admit that kernel AST/transformation is not suited for adding reachability fences and move this whole logic to the flow graph (where it would probably be much simpler).

That was @mraleph's idea also, but I had a hard time proving too myself we wouldn't do premature optimizations in kernel that would violate reachability.

At least one such situation I can come up with is

void main() {
  MyFinalizable1 m1;
  MyFinalizable1 m2 = ...;
  if (int.parse('1') == 1) {
    m1 = ...;
    m2.m1 = m1;
  }
  if (int.parse('2') == 2) {
    // someFfiCall using m2, but also using m1 if m2 happens to point to it.
  }
}

It is a bit of a contrived example, because now you have both m2 holding on to m1 natively and Dart holding on to m1 from Dart. However, deleting the code in this case surely doesn't conform to our specification:

Any local variable with a static type that includes Finalizable is guaranteed to be alive until execution exits the code block where the variable is in scope.

https://api.flutter.dev/flutter/dart-ffi/Finalizable-class.html

Any optimization we do before the flow graph would break the notion of "variable in scope" that users see in the editor.

I believe it would be a bad idea to start to make exceptions to that, and change the documentation in Finalizable to "guaranteed to be alive except for if it's provably dead code". It would require users to start reasoning about compiler optimizations instead of only their code.

CL for a fix using the current approach: https://dart-review.googlesource.com/c/sdk/+/312909

[mkustermann]

@dcharkes can this be closed now? Does it need a cherry-pick?

[dcharkes]

@dcharkes can this be closed now?

Yes

Does it need a cherry-pick?

We could do a cherry-pick, though the workaround is trivial. Do you think it's worth the churn?