dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.22k stars 1.57k forks source link

Dead instance calls left in code #40402

Open askeksa-google opened 4 years ago

askeksa-google commented 4 years ago

Sometimes, the table dispatch transformation encounters instance calls for which no targets exist in the program. This happens when the TFA tree shaker has deemed all of those targets unreachable but hasn't removed the calls to them. Logically, such a call is itself either unreachable, or the receiver is always null.

In debug mode, the table dispatch transformation inserts a null check followed by a breakpoint with a comment saying Dead instance call executed.. Searching for that comment in the disassembly reveals the offending calls. For instance, running

DART_CONFIGURATION=DebugX64 pkg/vm/tool/precompiler2 --use-table-dispatch --disassemble pkg/compiler/bin/dart2js.dart dart2js.snapshot

reveals 7 such callsites.

alexmarkov commented 4 years ago

@askeksa-google I'm not able to reproduce this on the tip of the tree. There are no Dead instance call executed. strings in disassembly, and if I add UNREACHABLE()

diff --git a/runtime/vm/compiler/aot/aot_call_specializer.cc b/runtime/vm/compiler/aot/aot_call_specializer.cc
index 9e52a228ad..32a9f52e8c 100644
--- a/runtime/vm/compiler/aot/aot_call_specializer.cc
+++ b/runtime/vm/compiler/aot/aot_call_specializer.cc
@@ -1294,6 +1294,7 @@ void AotCallSpecializer::TryReplaceWithDispatchTableCall(
   if (selector == nullptr) {
     // Target functions were removed by tree shaking. This call is dead code,
     // or the receiver is always null.
+    UNREACHABLE();
 #if defined(DEBUG)
     AddCheckNull(receiver->CopyWithType(Z), call->function_name(),
                  DeoptId::kNone, call->env(), call);

It isn't hit when compiling dart2js.

askeksa-google commented 4 years ago

Right, the reproduction description was incomplete. The dead calls only show up once the unused table selector elimination is activated.

alexmarkov commented 3 years ago

There is currently 1 such dead instance call when compiling dart2js. It occurs inside closure in _HttpClient._findCredentials:

https://github.com/dart-lang/sdk/blob/fb5ffc827580ae49cd4611d3ddf0ad3c7c592c54/sdk/lib/_http/http_impl.dart#L2580-L2590

Here is the smaller repro:

// @dart=2.10

class A {
  bool get foo1 => true;
  int get foo2 => 42;
}

@pragma('vm:never-inline')
doTest(v) {
  (value) {
    A a = value as A;
    if (a.foo1) {
      print(a.foo2);
    }
  }.call(v);
}

void main() {
  doTest(null);
}

The problem is that in most cases TFA propagates types without specializing them. In the smaller repro, after value as A cast the type of a is (A+)? (nullable subtypes of A). After the first call a.foo1 this value is narrowed to A+ (non-nullable subtypes of A). It is in fact empty, as instances of A are not allocated, but analysis doesn't know that without calculating type specialization.

When analyzing a.foo2, in order to determine if it is reachable, analysis looks at types of arguments and checks if any of them is empty. At that point A+ is considered non-empty and call site is marked as reachable.