WeakReference tests fail on precompiled enviromnent

[sgrekhov]

The following co19 tests fails on dartkp
https://github.com/dart-lang/co19/blob/master/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t01.dart
https://github.com/dart-lang/co19/blob/master/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t02.dart
https://github.com/dart-lang/co19/blob/master/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t03.dart
https://github.com/dart-lang/co19/blob/master/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t04.dart
https://github.com/dart-lang/co19/blob/master/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t07.dart
https://github.com/dart-lang/co19/blob/master/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t08.dart

For example

import "../../gc_utils_lib.dart";
import "../../../../Utils/expect.dart";

class C {
  int id;
  C(this.id);
}

main() {
  C? c = C(42);
  WeakReference<C> wr = WeakReference(c);
  Expect.equals(c, wr.target);
  triggerGc();
  Expect.equals(c, wr.target);
  c = null;
  triggerGc();
  Expect.isNull(wr.target); // Fails here
}

/========================================================================================================================================================================\
| dartkp-strong-linux-release-x64:co19/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t01 is new and failed (RuntimeError, expected Pass) |
\========================================================================================================================================================================/

--- Command "vm_compile_to_kernel" (took 05.000913s):
DART_CONFIGURATION=ReleaseX64 /b/s/w/ir/pkg/vm/tool/gen_kernel --aot --platform=out/ReleaseX64/vm_platform_strong.dill -o /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.dill /b/s/w/ir/tests/co19/src/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t01.dart -Dtest_runner.configuration=dartkp-strong-linux-release-x64 --packages=/b/s/w/ir/.packages -Ddart.vm.product=false --sound-null-safety

exit code:
0

--- Command "precompiler" (took 05.000311s):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/gen_snapshot --snapshot-kind=app-aot-assembly --assembly=/b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.S -Dtest_runner.configuration=dartkp-strong-linux-release-x64 --ignore-unrecognized-flags --packages=/b/s/w/ir/.packages /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.dill

exit code:
0

--- Command "remove_kernel_file" (took 21ms):
DART_CONFIGURATION=ReleaseX64 rm /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.dill

exit code:
0

--- Command "assemble" (took 01.000228s):
DART_CONFIGURATION=ReleaseX64 gcc -m64 -shared -o /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.aotsnapshot /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.S

exit code:
0

--- Command "remove_assembly" (took 8ms):
DART_CONFIGURATION=ReleaseX64 rm /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.S

exit code:
0

--- Command "vm" (took 01.000059s):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart_precompiled_runtime --sound-null-safety -Dtest_runner.configuration=dartkp-strong-linux-release-x64 --ignore-unrecognized-flags --packages=/b/s/w/ir/.packages /b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp-strong-linux-release-x64/tests_co19_src_LanguageFeatures_FinalizationRegistry_ffi_WeakReference_weak_reference_target_t01/out.aotsnapshot

exit code:
255

stderr:
Unhandled exception:
Expect.isNull(actual: <Instance of 'C'>) fails.
#0      _fail (file:///b/s/w/ir/tests/co19/src/Utils/expect.dart:18)
#1      Expect.isNull (file:///b/s/w/ir/tests/co19/src/Utils/expect_common.dart:38)
#2      main (file:///b/s/w/ir/tests/co19/src/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t01.dart:33)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297)
#4      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192)

--- Re-run this test:
python3 tools/test.py -n dartkp-strong-linux-release-x64 co19/LanguageFeatures/FinalizationRegistry/ffi/WeakReference/weak_reference_target_t01

cc @dcharkes

[dcharkes]

Thanks for reporting @sgrekhov. I ran into this as well.

main() {
  C? c = C(42);
  WeakReference<C> wr = WeakReference(c);
  Expect.equals(c, wr.target);
  triggerGc();
  Expect.equals(c, wr.target);
  c = null;
  triggerGc();
  Expect.isNull(wr.target); // Fails here
}

The compiler is too smart 😄 It figures out that c is not read after c = null, so it optimizes it away.

A more reliable way to lose a reference to an object is to create it in a separate function call and insures it never gets inlined:

@pragma('vm:never-inline')
WeakReference<C> fabricateWeakReferece() {
  C? c = C(42);
  WeakReference<C> wr = WeakReference(c);
  Expect.equals(c, wr.target);
  triggerGc();
  Expect.equals(c, wr.target);
  return wr;
}

main() {
  WeakReference<C> wr = fabricateWeakReferece();
  triggerGc();
  Expect.isNull(wr.target); // Fails here
}

Please fix the tests in this way.

[lrhn]

Proves that testing GC, which has no specification, is not going to be easy. 😁

There is no promise that a weak reference will ever become null. Tests like this are necessarily heuristic, and may be invalidated by later changes to implementation.

[sgrekhov]

@dcharkes could you please help me to figure out why the following test fails on dartkp?

class C {
  int id;
  C(this.id);
}

main() {
  List<C> refs = List.filled(5, C(42), growable: true);

  WeakReference<C> wr = WeakReference(refs[0]);
  Expect.equals(refs[0], wr.target);
  triggerGc();
  Expect.equals(refs[1], wr.target);
  refs.clear();
  Expect.equals(0, refs.length);
  triggerGc();
  Expect.isNull(wr.target); // fails here
}

Is this failure the result of the optimization or an issue? List is cleared, there must be no references to the instances of C.

[sgrekhov]

Proves that testing GC, which has no specification, is not going to be easy. 😁

There is no promise that a weak reference will ever become null. Tests like this are necessarily heuristic, and may be invalidated by later changes to implementation.

@lrhn but how we then can be sure that we have a good test coverage? I'm adding tests according to a @dcharkes advice (using a separate function with @pragma('vm:never-inline') annotation) but may be it makes sense to have tests that don't use a separate function to lost the reference to the objects well (as most probably will do the user)? For example, if compiler is too smart and optimizes variable that is never reads then why the following test fails?

class C {
  int id;
  C(this.id);

  String toString() => id.toString();
}

main() {
  C? c = C(42);
  WeakReference<C> wr = WeakReference(c);
  Expect.equals(c, wr.target);
  triggerGc();
  Expect.equals(c, wr.target);
  c = C(-1);
  print(c); // prints -1
  triggerGc();
  Expect.isNull(wr.target); // Expect.isNull(actual: <42>) fails.
}

Why it failing? There is no references to the object with id=42. It looks surprising. I understand that WeakReference object doesn't do any promises, but user, who writes cashe using WeakReferences may be surprised as well when found that his cashe is never invalidated by VM.

If failure of the test above is a result of the optimisation I'd be nice to understand what's going on. If I'd have such unrerstanding I'd try to add some set of tests that decieves optimizer and works on dartkp. Later, if implementation changes, the person who makes this changes will simply approve the failures or will say "Ok, if you want to go on cheating do it in the following way...". Does it makes sense?

[dcharkes]

Why it failing?

The Dart compiler uses single static assignment (SSA). This means that in variable c are two values v1 and v2, they are different, and they might up in different stack slots or registers which means that v1 can be reachable after v2 has been created.

pkg/vm/tool/precompiler2 --packages=.packages --print-flow-graph --print-flow-graph-filter=main --compiler-passes=* tests/ffi/regress_48391_test.dart tests/ffi/regress_48391_test.dart.bin &> log.tx

    v2 <- AllocateObject:12(cls=C)
    // ...
    v18 <- AllocateObject:38(cls=C)

So, in general trying to overwrite a value means nothing for for the compiler.

As background, the reason the compiler does this is to optimize code such as:

var a = C('1');
var b = a;
var a = C('2');
use(a);
use(b);

There's no need to move any values around to different stack locations or registers.

I understand that WeakReference object doesn't do any promises, but user, who writes cache using WeakReferences may be surprised as well when found that his cache is never invalidated by VM.

We might get a rather long conversation, with many internal compiler details.

As @lrhn mentioned there's a reason why this is not part of the language specification. And there is a reason why the documentation on WeakReference is specified as that it may be cleaned up.

[sgrekhov]

Thank you, Daco. Sounds reasonable. I'll rewrite the tests

[lrhn]

Being heuristic is an acceptable choice, as long as we are aware of it. A lot of our tests checks how things are "right now", and fail if we change something unrelated (and then we need to update the expectation files/golden files).
As long as we think of the tests as tripwires, a failure telling us that something changed, not necessarily that the change is wrong, then it can still be useful. We just need to be ready to rewrite the tests if things change validly.

So, I guess it counts as a (currently valid) heuristic that if the function declaring the variable exits, then it's very unlikely that the variable will keep the value alive. Overwriting doesn't (necessarily) work.