[vm] Align entry point verification and the precompiler.

For entry-point pragma annotations, most of the time they are
used with either no argument or with an argument that evaluates
to either

* false to denote the annotation should not take effect, or
* null or true to denote the annotation should take effect.

However, the user can also specify that only part of the operations
on a member should be accessed from native code by using a string
argument that is either 'call', 'set', or 'get'.

The entry point verification in Invoke/InvokeGetter/InvokeSetter
assumes that for getters and setters, the only valid string argument
is 'get' or 'set', respectively. This is because those methods are
called via `Dart_GetField`[0] and `Dart_SetField`, respectively, as if
they were the getter or setter of a defined field.

However, the precompiler previously assumed that the string
argument 'call' was the only string argument that meant the link
to a function's code object should be saved. Similarly, it assumed the
string argument 'get' for functions meant that their implicit closure
function should be saved, which ends up including getters. Furthermore,
it did not do anything with setters annotated with the string argument
'set'. This means that the code link would not be saved for getters or
setters that were annotated with the string argument expected by the
entry point verifier.

This CL aligns the precompiler to match the expectations of other
parts of the codebase. It also changes TFA to report an error
if a getter or setter is marked with the string argument 'call'.

[0] `Dart_Invoke` can be called with the name of a getter that
returns a closure, but doing so is semantically equivalent to
calling `Dart_GetField` followed by `Dart_InvokeClosure`.

TEST=vm/dart/entrypoint_verification_test

Fixes: #59920
Change-Id: Ia2768bbaf9058bb14a1cdfb331eb85fa082a0e90
Cq-Include-Trybots: luci.dart.try:vm-aot-dwarf-linux-product-x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-product-x64-try,vm-aot-mac-product-arm64-try,vm-aot-obfuscate-linux-release-x64-try,vm-linux-debug-x64-try,vm-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404823
Reviewed-by: Alexander Markov <[email protected]>
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: sstrickl

pkg/vm/lib/transformations/type_flow/native_code.dart

@@ -76,14 +76,19 @@ class PragmaEntryPointsVisitor extends RecursiveVisitor {
     return types ?? const [];
   }
 
+  static const _referenceToDocumentation =
+      "See https://github.com/dart-lang/sdk/blob/master/runtime/docs/compiler/"
+      "aot/entry_point_pragma.md.";
+
   @override
   visitLibrary(Library library) {
     for (final type in entryPointTypesFromPragmas(library.annotations)) {
       if (type == PragmaEntryPointType.Default) {
         nativeCodeOracle.addLibraryReferencedFromNativeCode(library);
       } else {
-        throw "Error: pragma entry-point definition on a library must evaluate "
-            "to null. See entry_points_pragma.md.";
+        throw "Error: The argument to an entry-point pragma annotation "
+            "on a library must evaluate to null, true, or false.\n"
+            "$_referenceToDocumentation";
       }
     }
     library.visitChildren(this);
@@ -101,8 +106,9 @@ class PragmaEntryPointsVisitor extends RecursiveVisitor {
         entryPoints.addDynamicallyExtendableClass(klass);
         nativeCodeOracle.addClassReferencedFromNativeCode(klass);
       } else {
-        throw "Error: pragma entry-point definition on a class must evaluate "
-            "to null, true or false. See entry_points_pragma.md.";
+        throw "Error: The argument to an entry-point pragma annotation "
+            "on a class must evaluate to null, true, or false.\n"
+            "$_referenceToDocumentation";
       }
     }
     klass.visitChildren(this);
@@ -119,34 +125,49 @@ class PragmaEntryPointsVisitor extends RecursiveVisitor {
           : new DirectSelector(proc, callKind: ck));
     }
 
-    final defaultCallKind = proc.isGetter
-        ? CallKind.PropertyGet
-        : (proc.isSetter ? CallKind.PropertySet : CallKind.Method);
-
     for (final type in types) {
       switch (type) {
         case PragmaEntryPointType.CallOnly:
-          addSelector(defaultCallKind);
+          if (proc.isGetter) {
+            throw "Error: The argument to an entry-point pragma annotation on "
+                "a getter ($proc) must evaluate to null, true, false, or "
+                "'get'.\n$_referenceToDocumentation";
+          }
+          if (proc.isSetter) {
+            throw "Error: The argument to an entry-point pragma annotation on "
+                "a setter ($proc) must evaluate to null, true, false, or "
+                "'set'.\n$_referenceToDocumentation";
+          }
+          addSelector(CallKind.Method);
           break;
         case PragmaEntryPointType.SetterOnly:
           if (!proc.isSetter) {
-            throw "Error: cannot generate a setter for a method or getter ($proc).";
+            throw "Error: cannot generate a setter for a method or getter "
+                "($proc).\n$_referenceToDocumentation";
           }
           addSelector(CallKind.PropertySet);
           break;
         case PragmaEntryPointType.GetterOnly:
           if (proc.isSetter) {
-            throw "Error: cannot closurize a setter ($proc).";
+            throw "Error: cannot closurize a setter ($proc).\n"
+                "$_referenceToDocumentation";
           }
           if (proc.isFactory) {
-            throw "Error: cannot closurize a factory ($proc).";
+            throw "Error: cannot closurize a factory ($proc).\n"
+                "$_referenceToDocumentation";
           }
           addSelector(CallKind.PropertyGet);
           break;
         case PragmaEntryPointType.Default:
-          addSelector(defaultCallKind);
-          if (!proc.isSetter && !proc.isGetter && !proc.isFactory) {
+          if (proc.isGetter) {
             addSelector(CallKind.PropertyGet);
+          } else if (proc.isSetter) {
+            addSelector(CallKind.PropertySet);
+          } else {
+            addSelector(CallKind.Method);
+            if (!proc.isFactory) {
+              addSelector(CallKind.PropertyGet);
+            }
           }
           break;
         case PragmaEntryPointType.Extendable:
@@ -165,8 +186,9 @@ class PragmaEntryPointsVisitor extends RecursiveVisitor {
     for (final type in entryPointTypesFromPragmas(ctor.annotations)) {
       if (type != PragmaEntryPointType.Default &&
           type != PragmaEntryPointType.CallOnly) {
-        throw "Error: pragma entry-point definition on a constructor ($ctor) must"
-            "evaluate to null, true, false or 'call'. See entry_points_pragma.md.";
+        throw "Error: The argument to an entry-point pragma annotation on a "
+            "constructor ($ctor) must evaluate to null, true, false or "
+            "'call'.\n$_referenceToDocumentation";
       }
       entryPoints
           .addRawCall(new DirectSelector(ctor, callKind: CallKind.Method));
@@ -192,21 +214,22 @@ class PragmaEntryPointsVisitor extends RecursiveVisitor {
           addSelector(CallKind.PropertyGet);
           break;
         case PragmaEntryPointType.SetterOnly:
-          if (field.isFinal) {
-            throw "Error: can't use 'set' in entry-point pragma for final field "
-                "$field";
+          if (!field.hasSetter) {
+            throw "Error: can't use 'set' in an entry-point pragma annotation "
+                "for a field that has no setter ($field).\n"
+                "$_referenceToDocumentation";
           }
           addSelector(CallKind.PropertySet);
           break;
         case PragmaEntryPointType.Default:
           addSelector(CallKind.PropertyGet);
-          if (!field.isFinal) {
+          if (field.hasSetter) {
             addSelector(CallKind.PropertySet);
           }
           break;
         case PragmaEntryPointType.CallOnly:
-          throw "Error: can't generate invocation dispatcher for field $field"
-              "through @pragma('vm:entry-point')";
+          throw "Error: 'call' is not a valid entry-point pragma annotation "
+              "argument for the field $field.\n$_referenceToDocumentation";
         case PragmaEntryPointType.Extendable:
           throw "Error: only class can be extendable";
         case PragmaEntryPointType.CanBeOverridden:

runtime/bin/entrypoints_verification_test.cc

@@ -168,6 +168,21 @@ bool IsTreeShaken(const char* name, Dart_Handle handle, const char* error) {
                         Dart_Null()));                                         \
   } while (false)
 
+#define TEST_GETTERS(target)                                                   \
+  do {                                                                         \
+    FAIL("get1", Dart_GetField(target, Dart_NewStringFromCString("get1")));    \
+    CHECK(Dart_GetField(target, Dart_NewStringFromCString("get2")));           \
+    CHECK(Dart_GetField(target, Dart_NewStringFromCString("get3")));           \
+  } while (false)
+
+#define TEST_SETTERS(target, value)                                            \
+  do {                                                                         \
+    FAIL("set1",                                                               \
+         Dart_SetField(target, Dart_NewStringFromCString("set1"), value));     \
+    CHECK(Dart_SetField(target, Dart_NewStringFromCString("set2"), value));    \
+    CHECK(Dart_SetField(target, Dart_NewStringFromCString("set3"), value));    \
+  } while (false)
+
 DART_EXPORT void RunTests() {
   is_dartaotruntime = Dart_IsPrecompiledRuntime();
 
@@ -261,4 +276,28 @@ DART_EXPORT void RunTests() {
 
   fprintf(stderr, "\n\nTesting fields with instance target\n\n\n");
   TEST_FIELDS(D);
+
+  //////// Test actions against getter and setter functions.
+
+  fprintf(stderr, "\n\nTesting getters with library target\n\n\n");
+  TEST_GETTERS(lib);
+
+  fprintf(stderr, "\n\nTesting getters with class target\n\n\n");
+  TEST_GETTERS(F_class);
+
+  fprintf(stderr, "\n\nTesting getters with instance target\n\n\n");
+  TEST_GETTERS(D);
+
+  Dart_Handle test_value =
+      Dart_GetField(lib, Dart_NewStringFromCString("testValue"));
+  CHECK(test_value);
+
+  fprintf(stderr, "\n\nTesting setters with library target\n\n\n");
+  TEST_SETTERS(lib, test_value);
+
+  fprintf(stderr, "\n\nTesting setters with class target\n\n\n");
+  TEST_SETTERS(F_class, test_value);
+
+  fprintf(stderr, "\n\nTesting setters with instance target\n\n\n");
+  TEST_SETTERS(D, test_value);
 }

runtime/docs/compiler/aot/entry_point_pragma.md

@@ -50,33 +50,92 @@ Note that `@pragma("vm:entry-point")` may be added to abstract classes -- in
 this case, their name will survive obfuscation, but they won't have any
 allocation stubs.
 
-### Procedures
+### Getters
 
-Any one of the following forms may be attached to a procedure (including
-getters, setters and constructors):
+Any one of the following forms may be attached to getters:
 
 ```dart
 @pragma("vm:entry-point")
 @pragma("vm:entry-point", true/false)
 @pragma("vm:entry-point", !const bool.fromEnvironment("dart.vm.product"))
 @pragma("vm:entry-point", "get")
+void get foo { ... }
+```
+
+The `"get"` annotation allows retrieval of the getter value via
+`Dart_GetField`. `Dart_Invoke` can only be used with getters that return a
+closure value, in which case it is the same as retrieving the closure via
+`Dart_GetField` and then invoking the closure using `Dart_InvokeClosure`, so
+the "get" annotation is also needed for such uses.
+
+If the second parameter is missing, `null` or `true`, it behaves the same
+as the `"get"` parameter.
+
+Getters cannot be closurized.
+
+### Setters
+
+Any one of the following forms may be attached to setters:
+
+```dart
+@pragma("vm:entry-point")
+@pragma("vm:entry-point", true/false)
+@pragma("vm:entry-point", !const bool.fromEnvironment("dart.vm.product"))
+@pragma("vm:entry-point", "set")
+void set foo(int value) { ... }
+```
+
+The `"set"` annotation allows setting the value via `Dart_SetField`.
+
+If the second parameter is missing, `null` or `true`, it behaves the same
+as the `"set"` parameter.
+
+Setters cannot be closurized.
+
+### Constructors
+
+Any one of the following forms may be attached to constructors:
+
+```dart
+@pragma("vm:entry-point")
+@pragma("vm:entry-point", true/false)
+@pragma("vm:entry-point", !const bool.fromEnvironment("dart.vm.product"))
 @pragma("vm:entry-point", "call")
-void foo() { ... }
+C(this.foo) { ... }
 ```
 
-If the second parameter is missing, `null` or `true`, the procedure (and its
-closurized form, excluding constructors and setters) will available for lookup
-and invocation directly from native or VM code.
+If the annotation is `"call"`, then the procedure is available for invocation
+(access via `Dart_Invoke`).
+
+If the second parameter is missing, `null` or `true`, it behaves the same
+as the `"call"` parameter.
+
+If the constructor is _generative_, the enclosing class must also be annotated
+for allocation from native or VM code.
+
+Constructors cannot be closurized.
+
+### Other Procedures
+
+Any one of the following forms may be attached to other types of procedures:
+
+```dart
+@pragma("vm:entry-point")
+@pragma("vm:entry-point", true/false)
+@pragma("vm:entry-point", !const bool.fromEnvironment("dart.vm.product"))
+@pragma("vm:entry-point", "get")
+@pragma("vm:entry-point", "call")
+void foo(int value) { ... }
+```
 
-If the procedure is a *generative* constructor, the enclosing class must also be
-annotated for allocation from native or VM code.
+If the annotation is `"get"`, then the procedure is only available for
+closurization (access via `Dart_GetField`).
 
-If the annotation is "get" or "call", the procedure will only be available for
-closurization (access via `Dart_GetField`) or invocation (access via
-`Dart_Invoke`).
+If the annotation is `"call"`, then the procedure is only available for
+invocation (access via `Dart_Invoke`).
 
-"@pragma("vm:entry-point", "get") against constructors or setters is disallowed
-since they cannot be closurized.
+If the second parameter is missing, `null` or `true`, the procedure is available
+for both closurization and invocation.
 
 ### Fields
 
@@ -95,11 +154,11 @@ int foo;
 If the second parameter is missing, `null` or `true`, the field is marked for
 native access and for non-static fields the corresponding getter and setter in
 the interface of the enclosing class are marked for native invocation. If the
-"get" or "set" parameter is used, only the getter or setter is marked. For
+`"get"` or `"set"` parameter is used, only the getter or setter is marked. For
 static fields, the implicit getter is always marked if the field is marked
 for native access.
 
-A field containing a closure may only be invoked using Dart_Invoke if the
+A field containing a closure may only be invoked using `Dart_Invoke` if the
 getter is marked, in which case it is the same as retrieving the closure from
-the field using Dart_GetField and then invoking the closure using
-Dart_InvokeClosure.
+the field using `Dart_GetField` and then invoking the closure using
+`Dart_InvokeClosure`.