[torque] Get rid of @noVerifier annotation

As one small step toward reducing annotations, I propose that all
classes get generated verifiers unless they've opted out of C++ class
generation via @doNotGenerateCppClass, and that generated verifiers
always verify every Torque-defined field. If a generated verifier is
incorrect, such as for JSFunction or DataHandler, we can just avoid
calling it and hand-code the verification.

Bug: v8:7793
Change-Id: I7c0edb660574d0c688a59c7e90c41ee7ad464b42
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3171758Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#77145}

src/builtins/base.tq

@@ -554,7 +554,6 @@ extern class Filler extends HeapObject generates 'TNode<HeapObject>';
 // Like JSObject, but created from API function.
 @apiExposedInstanceTypeValue(0x422)
 @doNotGenerateCast
-@noVerifier
 extern class JSApiObject extends JSObject generates 'TNode<JSObject>';
 
 // TODO(gsathya): This only exists to make JSApiObject instance type into a
@@ -562,7 +561,6 @@ extern class JSApiObject extends JSObject generates 'TNode<JSObject>';
 @apiExposedInstanceTypeValue(0x80A)
 @doNotGenerateCast
 @highestInstanceTypeWithinParentClassRange
-@noVerifier
 extern class JSLastDummyApiObject extends JSApiObject
     generates 'TNode<JSObject>';
 

src/diagnostics/objects-debug.cc

@@ -166,7 +166,9 @@ void TaggedIndex::TaggedIndexVerify(Isolate* isolate) {
 }
 
 void HeapObject::HeapObjectVerify(Isolate* isolate) {
-  TorqueGeneratedClassVerifiers::HeapObjectVerify(*this, isolate);
+  CHECK(IsHeapObject());
+  VerifyPointer(isolate, map(isolate));
+  CHECK(map(isolate).IsMap());
 
   switch (map().instance_type()) {
 #define STRING_TYPE_CASE(TYPE, size, name, CamelName) case TYPE:
@@ -827,7 +829,24 @@ void JSBoundFunction::JSBoundFunctionVerify(Isolate* isolate) {
 }
 
 void JSFunction::JSFunctionVerify(Isolate* isolate) {
-  TorqueGeneratedClassVerifiers::JSFunctionVerify(*this, isolate);
+  // Don't call TorqueGeneratedClassVerifiers::JSFunctionVerify here because the
+  // Torque class definition contains the field `prototype_or_initial_map` which
+  // may not be allocated.
+
+  // This assertion exists to encourage updating this verification function if
+  // new fields are added in the Torque class layout definition.
+  STATIC_ASSERT(JSFunction::TorqueGeneratedClass::kHeaderSize ==
+                8 * kTaggedSize);
+
+  JSFunctionOrBoundFunctionVerify(isolate);
+  CHECK(IsJSFunction());
+  VerifyPointer(isolate, shared(isolate));
+  CHECK(shared(isolate).IsSharedFunctionInfo());
+  VerifyPointer(isolate, context(isolate, kRelaxedLoad));
+  CHECK(context(isolate, kRelaxedLoad).IsContext());
+  VerifyPointer(isolate, raw_feedback_cell(isolate));
+  CHECK(raw_feedback_cell(isolate).IsFeedbackCell());
+  VerifyPointer(isolate, raw_code(isolate));
   CHECK(raw_code(isolate).IsCodeT());
   CHECK(map(isolate).is_callable());
   Handle<JSFunction> function(*this, isolate);
@@ -1230,8 +1249,9 @@ void SmallOrderedHashTable<Derived>::SmallOrderedHashTableVerify(
     }
   }
 }
+
 void SmallOrderedHashMap::SmallOrderedHashMapVerify(Isolate* isolate) {
-  TorqueGeneratedClassVerifiers::SmallOrderedHashMapVerify(*this, isolate);
+  CHECK(IsSmallOrderedHashMap());
   SmallOrderedHashTable<SmallOrderedHashMap>::SmallOrderedHashTableVerify(
       isolate);
   for (int entry = NumberOfElements(); entry < NumberOfDeletedElements();
@@ -1244,7 +1264,7 @@ void SmallOrderedHashMap::SmallOrderedHashMapVerify(Isolate* isolate) {
 }
 
 void SmallOrderedHashSet::SmallOrderedHashSetVerify(Isolate* isolate) {
-  TorqueGeneratedClassVerifiers::SmallOrderedHashSetVerify(*this, isolate);
+  CHECK(IsSmallOrderedHashSet());
   SmallOrderedHashTable<SmallOrderedHashSet>::SmallOrderedHashTableVerify(
       isolate);
   for (int entry = NumberOfElements(); entry < NumberOfDeletedElements();
@@ -1258,8 +1278,7 @@ void SmallOrderedHashSet::SmallOrderedHashSetVerify(Isolate* isolate) {
 
 void SmallOrderedNameDictionary::SmallOrderedNameDictionaryVerify(
     Isolate* isolate) {
-  TorqueGeneratedClassVerifiers::SmallOrderedNameDictionaryVerify(*this,
-                                                                  isolate);
+  CHECK(IsSmallOrderedNameDictionary());
   SmallOrderedHashTable<
       SmallOrderedNameDictionary>::SmallOrderedHashTableVerify(isolate);
   for (int entry = NumberOfElements(); entry < NumberOfDeletedElements();
@@ -1655,9 +1674,20 @@ void WasmExportedFunctionData::WasmExportedFunctionDataVerify(
 #endif  // V8_ENABLE_WEBASSEMBLY
 
 void DataHandler::DataHandlerVerify(Isolate* isolate) {
-  TorqueGeneratedClassVerifiers::DataHandlerVerify(*this, isolate);
+  // Don't call TorqueGeneratedClassVerifiers::DataHandlerVerify because the
+  // Torque definition of this class includes all of the optional fields.
+
+  // This assertion exists to encourage updating this verification function if
+  // new fields are added in the Torque class layout definition.
+  STATIC_ASSERT(DataHandler::kHeaderSize == 6 * kTaggedSize);
+
+  StructVerify(isolate);
+  CHECK(IsDataHandler());
+  VerifyPointer(isolate, smi_handler(isolate));
   CHECK_IMPLIES(!smi_handler().IsSmi(),
                 IsStoreHandler() && smi_handler().IsCodeT());
+  VerifyPointer(isolate, validity_cell(isolate));
+  CHECK(validity_cell().IsSmi() || validity_cell().IsCell());
   int data_count = data_field_count();
   if (data_count >= 1) {
     VerifyMaybeObjectField(isolate, kData1Offset);

src/objects/bigint.tq

@@ -9,7 +9,6 @@ extern class BigIntBase extends PrimitiveHeapObject
 
 type BigInt extends BigIntBase;
 
-@noVerifier
 @hasSameInstanceTypeAsParent
 @doNotGenerateCast
 extern class MutableBigInt extends BigIntBase generates 'TNode<BigInt>';

src/objects/data-handler.tq

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// This class does not use the generated verifier, so if you change anything
+// here, please also update DataHandlerVerify in objects-debug.cc.
 @abstract
 extern class DataHandler extends Struct {
   // [smi_handler]: A Smi which encodes a handler or Code object (we still
@@ -15,7 +17,7 @@ extern class DataHandler extends Struct {
   validity_cell: Smi|Cell;
 
   // Space for the following fields may or may not be allocated.
-  @noVerifier data1: MaybeObject;
-  @noVerifier data2: MaybeObject;
-  @noVerifier data3: MaybeObject;
+  data1: MaybeObject;
+  data2: MaybeObject;
+  data3: MaybeObject;
 }

src/objects/js-function.tq

@@ -17,6 +17,8 @@ extern class JSBoundFunction extends JSFunctionOrBoundFunction {
   bound_arguments: FixedArray;
 }
 
+// This class does not use the generated verifier, so if you change anything
+// here, please also update JSFunctionVerify in objects-debug.cc.
 @highestInstanceTypeWithinParentClassRange
 extern class JSFunction extends JSFunctionOrBoundFunction {
   shared_function_info: SharedFunctionInfo;
@@ -25,7 +27,7 @@ extern class JSFunction extends JSFunctionOrBoundFunction {
   @if(V8_EXTERNAL_CODE_SPACE) code: CodeDataContainer;
   @ifnot(V8_EXTERNAL_CODE_SPACE) code: Code;
   // Space for the following field may or may not be allocated.
-  @noVerifier prototype_or_initial_map: JSReceiver|Map;
+  prototype_or_initial_map: JSReceiver|Map;
 }
 
 // Class constructors are special, because they are callable, but [[Call]] will

src/objects/ordered-hash-table.tq

@@ -14,7 +14,6 @@ const kSmallOrderedHashTableNotFound: constexpr int31
 const kSmallOrderedHashTableLoadFactor: constexpr int31
     generates 'SmallOrderedHashTable<int>::kLoadFactor';
 
-@noVerifier
 @abstract
 @doNotGenerateCppClass
 extern class SmallOrderedHashTable extends HeapObject

src/objects/swiss-name-dictionary.tq

@@ -4,7 +4,6 @@
 
 #include 'src/objects/swiss-name-dictionary.h'
 
-@noVerifier
 @doNotGenerateCppClass
 extern class SwissNameDictionary extends HeapObject {
   hash: uint32;

src/torque/ast.h

@@ -939,7 +939,6 @@ struct ClassFieldExpression {
   std::vector<ConditionalAnnotation> conditions;
   bool weak;
   bool const_qualified;
-  bool generate_verify;
   FieldSynchronization read_synchronization;
   FieldSynchronization write_synchronization;
 };

src/torque/constants.h

@@ -84,7 +84,6 @@ static const char* const WEAK_HEAP_OBJECT = "WeakHeapObject";
 static const char* const STATIC_ASSERT_MACRO_STRING = "StaticAssert";
 
 static const char* const ANNOTATION_GENERATE_PRINT = "@generatePrint";
-static const char* const ANNOTATION_NO_VERIFIER = "@noVerifier";
 static const char* const ANNOTATION_ABSTRACT = "@abstract";
 static const char* const ANNOTATION_HAS_SAME_INSTANCE_TYPE_AS_PARENT =
     "@hasSameInstanceTypeAsParent";
@@ -145,20 +144,19 @@ enum class ClassFlag {
   kNone = 0,
   kExtern = 1 << 0,
   kGeneratePrint = 1 << 1,
-  kGenerateVerify = 1 << 2,
-  kTransient = 1 << 3,
-  kAbstract = 1 << 4,
-  kIsShape = 1 << 5,
-  kHasSameInstanceTypeAsParent = 1 << 6,
-  kGenerateCppClassDefinitions = 1 << 7,
-  kCustomCppClass = 1 << 8,
-  kHighestInstanceTypeWithinParent = 1 << 9,
-  kLowestInstanceTypeWithinParent = 1 << 10,
-  kUndefinedLayout = 1 << 11,
-  kGenerateBodyDescriptor = 1 << 12,
-  kExport = 1 << 13,
-  kDoNotGenerateCast = 1 << 14,
-  kCustomMap = 1 << 15,
+  kTransient = 1 << 2,
+  kAbstract = 1 << 3,
+  kIsShape = 1 << 4,
+  kHasSameInstanceTypeAsParent = 1 << 5,
+  kGenerateCppClassDefinitions = 1 << 6,
+  kCustomCppClass = 1 << 7,
+  kHighestInstanceTypeWithinParent = 1 << 8,
+  kLowestInstanceTypeWithinParent = 1 << 9,
+  kUndefinedLayout = 1 << 10,
+  kGenerateBodyDescriptor = 1 << 11,
+  kExport = 1 << 12,
+  kDoNotGenerateCast = 1 << 13,
+  kCustomMap = 1 << 14,
 };
 using ClassFlags = base::Flags<ClassFlag>;
 

src/torque/implementation-visitor.cc

@@ -77,7 +77,7 @@ const Type* ImplementationVisitor::Visit(Statement* stmt) {
 void ImplementationVisitor::BeginGeneratedFiles() {
   std::set<SourceId> contains_class_definitions;
   for (const ClassType* type : TypeOracle::GetClasses()) {
-    if (type->GenerateCppClassDefinitions()) {
+    if (type->ShouldGenerateCppClassDefinitions()) {
       contains_class_definitions.insert(type->AttributedToFile());
     }
   }
@@ -3818,7 +3818,8 @@ void ImplementationVisitor::GenerateClassFieldOffsets(
     for (const ClassType* type : TypeOracle::GetClasses()) {
       // TODO(danno): Remove this once all classes use ClassFieldOffsetGenerator
       // to generate field offsets without the use of macros.
-      if (!type->GenerateCppClassDefinitions() && !type->HasUndefinedLayout()) {
+      if (!type->ShouldGenerateCppClassDefinitions() &&
+          !type->HasUndefinedLayout()) {
         MacroFieldOffsetsGenerator g(header, type);
         for (auto f : type->fields()) {
           CurrentSourcePosition::Scope scope(f.pos);
@@ -4208,7 +4209,7 @@ void CppClassGenerator::GenerateClass() {
     // hand-written definition.
     base::Optional<const ClassType*> parent = type_->parent()->ClassSupertype();
     while (parent) {
-      if ((*parent)->GenerateCppClassDefinitions() &&
+      if ((*parent)->ShouldGenerateCppClassDefinitions() &&
           !(*parent)->ShouldGenerateFullClassDefinition() &&
           (*parent)->AttributedToFile() == type_->AttributedToFile()) {
         Error("Exported ", *type_,
@@ -4681,7 +4682,7 @@ void ImplementationVisitor::GenerateClassDefinitions(
       CurrentSourcePosition::Scope position_activator(type->GetPosition());
       auto& streams = GlobalContext::GeneratedPerFile(type->AttributedToFile());
       std::ostream& header = streams.class_definition_headerfile;
-      std::string name = type->GenerateCppClassDefinitions()
+      std::string name = type->ShouldGenerateCppClassDefinitions()
                              ? type->name()
                              : type->GetGeneratedTNodeTypeName();
       header << "class " << name << ";\n";
@@ -4695,7 +4696,7 @@ void ImplementationVisitor::GenerateClassDefinitions(
       std::ostream& inline_header = streams.class_definition_inline_headerfile;
       std::ostream& implementation = streams.class_definition_ccfile;
 
-      if (type->GenerateCppClassDefinitions()) {
+      if (type->ShouldGenerateCppClassDefinitions()) {
         CppClassGenerator g(type, header, inline_header, implementation);
         g.GenerateClass();
       }
@@ -4881,7 +4882,7 @@ void ImplementationVisitor::GeneratePrintDefinitions(
     for (const ClassType* type : TypeOracle::GetClasses()) {
       if (!type->ShouldGeneratePrint()) continue;
 
-      if (type->GenerateCppClassDefinitions()) {
+      if (type->ShouldGenerateCppClassDefinitions()) {
         const ClassType* super = type->GetSuperClass();
         std::string gen_name = "TorqueGenerated" + type->name();
         std::string gen_name_T =
@@ -5119,7 +5120,6 @@ void GenerateClassFieldVerifier(const std::string& class_name,
                                 const ClassType& class_type, const Field& f,
                                 std::ostream& h_contents,
                                 std::ostream& cc_contents) {
-  if (!f.generate_verify) return;
   const Type* field_type = f.name_and_type.type;
 
   // We only verify tagged types, not raw numbers or pointers. Structs
@@ -5236,16 +5236,7 @@ void ImplementationVisitor::GenerateClassVerifiers(
       }
       if (super_type) {
         std::string super_name = super_type->name();
-        if (super_name == "HeapObject") {
-          // Special case: HeapObjectVerify checks the Map type and dispatches
-          // to more specific types, so calling it here would cause infinite
-          // recursion. We could consider moving that behavior into a
-          // different method to make the contract of *Verify methods more
-          // consistent, but for now we'll just avoid the bad case.
-          cc_contents << "  " << super_name << "Verify(o, isolate);\n";
-        } else {
-          cc_contents << "  o." << super_name << "Verify(isolate);\n";
-        }
+        cc_contents << "  o." << super_name << "Verify(isolate);\n";
       }
 
       // Second, verify that this object is what it claims to be.

src/torque/torque-parser.cc

@@ -888,7 +888,7 @@ base::Optional<ParseResult> MakeClassDeclaration(
     ParseResultIterator* child_results) {
   AnnotationSet annotations(
       child_results,
-      {ANNOTATION_GENERATE_PRINT, ANNOTATION_NO_VERIFIER, ANNOTATION_ABSTRACT,
+      {ANNOTATION_GENERATE_PRINT, ANNOTATION_ABSTRACT,
        ANNOTATION_HAS_SAME_INSTANCE_TYPE_AS_PARENT,
        ANNOTATION_DO_NOT_GENERATE_CPP_CLASS, ANNOTATION_CUSTOM_CPP_CLASS,
        ANNOTATION_CUSTOM_MAP, ANNOTATION_GENERATE_BODY_DESCRIPTOR,
@@ -900,8 +900,6 @@ base::Optional<ParseResult> MakeClassDeclaration(
   ClassFlags flags = ClassFlag::kNone;
   bool generate_print = annotations.Contains(ANNOTATION_GENERATE_PRINT);
   if (generate_print) flags |= ClassFlag::kGeneratePrint;
-  bool generate_verify = !annotations.Contains(ANNOTATION_NO_VERIFIER);
-  if (generate_verify) flags |= ClassFlag::kGenerateVerify;
   if (annotations.Contains(ANNOTATION_ABSTRACT)) {
     flags |= ClassFlag::kAbstract;
   }
@@ -1971,11 +1969,9 @@ base::Optional<ParseResult> MakeAnnotation(ParseResultIterator* child_results) {
 base::Optional<ParseResult> MakeClassField(ParseResultIterator* child_results) {
   AnnotationSet annotations(
       child_results,
-      {ANNOTATION_NO_VERIFIER, ANNOTATION_CPP_RELAXED_STORE,
-       ANNOTATION_CPP_RELAXED_LOAD, ANNOTATION_CPP_RELEASE_STORE,
-       ANNOTATION_CPP_ACQUIRE_LOAD},
+      {ANNOTATION_CPP_RELAXED_STORE, ANNOTATION_CPP_RELAXED_LOAD,
+       ANNOTATION_CPP_RELEASE_STORE, ANNOTATION_CPP_ACQUIRE_LOAD},
       {ANNOTATION_IF, ANNOTATION_IFNOT});
-  bool generate_verify = !annotations.Contains(ANNOTATION_NO_VERIFIER);
   FieldSynchronization write_synchronization = FieldSynchronization::kNone;
   if (annotations.Contains(ANNOTATION_CPP_RELEASE_STORE)) {
     write_synchronization = FieldSynchronization::kAcquireRelease;
@@ -2027,7 +2023,6 @@ base::Optional<ParseResult> MakeClassField(ParseResultIterator* child_results) {
                                           std::move(conditions),
                                           weak,
                                           const_qualified,
-                                          generate_verify,
                                           read_synchronization,
                                           write_synchronization}};
 }

src/torque/type-visitor.cc

@@ -211,7 +211,6 @@ const StructType* TypeVisitor::ComputeType(
             offset.SingleValue(),
             false,
             field.const_qualified,
-            false,
             FieldSynchronization::kNone,
             FieldSynchronization::kNone};
     auto optional_size = SizeOf(f.name_and_type.type);
@@ -319,7 +318,7 @@ const ClassType* TypeVisitor::ComputeType(
         Error("non-external classes must have defined layouts");
       }
     }
-    flags = flags | ClassFlag::kGeneratePrint | ClassFlag::kGenerateVerify;
+    flags = flags | ClassFlag::kGeneratePrint;
   }
   if (!(flags & ClassFlag::kExtern) &&
       (flags & ClassFlag::kHasSameInstanceTypeAsParent)) {
@@ -442,7 +441,6 @@ void TypeVisitor::VisitClassFieldsAndMethods(
          class_offset.SingleValue(),
          field_expression.weak,
          field_expression.const_qualified,
-         field_expression.generate_verify,
          field_expression.read_synchronization,
          field_expression.write_synchronization});
     ResidueClass field_size = std::get<0>(field.GetFieldSizeInformation());

src/torque/types.h

@@ -228,7 +228,6 @@ struct Field {
 
   bool is_weak;
   bool const_qualified;
-  bool generate_verify;
   FieldSynchronization read_synchronization;
   FieldSynchronization write_synchronization;
 };
@@ -674,8 +673,8 @@ class ClassType final : public AggregateType {
            ((flags_ & ClassFlag::kGeneratePrint) && !HasUndefinedLayout());
   }
   bool ShouldGenerateVerify() const {
-    return !IsExtern() || ((flags_ & ClassFlag::kGenerateVerify) &&
-                           (!HasUndefinedLayout() && !IsShape()));
+    return !IsExtern() || (ShouldGenerateCppClassDefinitions() &&
+                           !HasUndefinedLayout() && !IsShape());
   }
   bool ShouldGenerateBodyDescriptor() const {
     return flags_ & ClassFlag::kGenerateBodyDescriptor ||
@@ -689,9 +688,8 @@ class ClassType final : public AggregateType {
   bool HasSameInstanceTypeAsParent() const {
     return flags_ & ClassFlag::kHasSameInstanceTypeAsParent;
   }
-  bool GenerateCppClassDefinitions() const {
-    return flags_ & ClassFlag::kGenerateCppClassDefinitions || !IsExtern() ||
-           ShouldGenerateBodyDescriptor();
+  bool ShouldGenerateCppClassDefinitions() const {
+    return (flags_ & ClassFlag::kGenerateCppClassDefinitions) || !IsExtern();
   }
   bool ShouldGenerateFullClassDefinition() const {
     return !IsExtern() && !(flags_ & ClassFlag::kCustomCppClass);