[tools] Fix v8windbg behavior on Map's bit_field2

Bill kindly pointed out to me that v8windbg was not handling bit_field2
correctly. The issue was that the constexpr type for ElementsKind was,
somewhat unsurprisingly, "ElementsKind", but v8windbg expected a fully-
qualified type name like "v8::internal::ElementsKind". This change
addresses the problem in two ways:
1. Update v8windbg's type resolution logic to resolve type names as if
   they were used in the v8::internal namespace. This makes it more
   consistent with how those type names are used in other generated
   Torque code, reducing surprises and the number of times we have to
   write `v8::internal::` in .tq files.
2. Add compile-time verification that any constexpr type name used as a
   string in class-debug-readers-tq.cc can also resolve as a type name.

Bug: v8:9376
Change-Id: I349cd6ab586fd8345a1fa8bfc3989bb8e6376ab8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2063769
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#66633}

src/builtins/base.tq

@@ -161,7 +161,7 @@ type LayoutDescriptor extends ByteArray
     generates 'TNode<LayoutDescriptor>';
 extern class TransitionArray extends WeakFixedArray;
 
-type InstanceType extends uint16 constexpr 'v8::internal::InstanceType';
+type InstanceType extends uint16 constexpr 'InstanceType';
 
 type NoSharedNameSentinel extends Smi;
 

src/objects/api-callbacks.tq

@@ -30,9 +30,8 @@ extern class AccessCheckInfo extends Struct {
   data: Object;
 }
 
-type PropertyAttributes extends int32
-constexpr 'v8::internal::PropertyAttributes';
-type SideEffectType extends int32 constexpr 'v8::SideEffectType';
+type PropertyAttributes extends int32 constexpr 'PropertyAttributes';
+type SideEffectType extends int32 constexpr 'SideEffectType';
 
 bitfield struct AccessorInfoFlags extends uint31 {
   all_can_read: bool: 1 bit;

src/objects/intl-objects.tq

@@ -15,10 +15,8 @@
 #include 'src/objects/js-segment-iterator.h'
 #include 'src/objects/js-segmenter.h'
 
-type DateTimeStyle extends int32
-constexpr 'v8::internal::JSDateTimeFormat::DateTimeStyle';
-type HourCycle extends int32
-constexpr 'v8::internal::JSDateTimeFormat::HourCycle';
+type DateTimeStyle extends int32 constexpr 'JSDateTimeFormat::DateTimeStyle';
+type HourCycle extends int32 constexpr 'JSDateTimeFormat::HourCycle';
 bitfield struct JSDateTimeFormatFlags extends uint31 {
   hour_cycle: HourCycle: 3 bit;
   date_style: DateTimeStyle: 3 bit;
@@ -34,10 +32,9 @@ extern class JSDateTimeFormat extends JSObject {
   flags: SmiTagged<JSDateTimeFormatFlags>;
 }
 
-type JSDisplayNamesStyle extends int32
-constexpr 'v8::internal::JSDisplayNames::Style';
+type JSDisplayNamesStyle extends int32 constexpr 'JSDisplayNames::Style';
 type JSDisplayNamesFallback extends int32
-constexpr 'v8::internal::JSDisplayNames::Fallback';
+constexpr 'JSDisplayNames::Fallback';
 bitfield struct JSDisplayNamesFlags extends uint31 {
   style: JSDisplayNamesStyle: 2 bit;
   fallback: JSDisplayNamesFallback: 1 bit;
@@ -48,10 +45,8 @@ extern class JSDisplayNames extends JSObject {
   flags: SmiTagged<JSDisplayNamesFlags>;
 }
 
-type JSListFormatStyle extends int32
-constexpr 'v8::internal::JSListFormat::Style';
-type JSListFormatType extends int32
-constexpr 'v8::internal::JSListFormat::Type';
+type JSListFormatStyle extends int32 constexpr 'JSListFormat::Style';
+type JSListFormatType extends int32 constexpr 'JSListFormat::Type';
 bitfield struct JSListFormatFlags extends uint31 {
   style: JSListFormatStyle: 2 bit;
   Type: JSListFormatType: 2 bit;  // "type" is a reserved word.
@@ -70,8 +65,7 @@ extern class JSNumberFormat extends JSObject {
   bound_format: JSFunction|Undefined;
 }
 
-type JSPluralRulesType extends int32
-constexpr 'v8::internal::JSPluralRules::Type';
+type JSPluralRulesType extends int32 constexpr 'JSPluralRules::Type';
 bitfield struct JSPluralRulesFlags extends uint31 {
   Type: JSPluralRulesType: 1 bit;  // "type" is a reserved word.
 }
@@ -85,7 +79,7 @@ extern class JSPluralRules extends JSObject {
 }
 
 type JSRelativeTimeFormatNumeric extends int32
-constexpr 'v8::internal::JSRelativeTimeFormat::Numeric';
+constexpr 'JSRelativeTimeFormat::Numeric';
 bitfield struct JSRelativeTimeFormatFlags extends uint31 {
   numeric: JSRelativeTimeFormatNumeric: 1 bit;
 }
@@ -102,7 +96,7 @@ extern class JSLocale extends JSObject {
 }
 
 type JSSegmenterGranularity extends int32
-constexpr 'v8::internal::JSSegmenter::Granularity';
+constexpr 'JSSegmenter::Granularity';
 bitfield struct JSSegmenterFlags extends uint31 {
   granularity: JSSegmenterGranularity: 2 bit;
 }

src/objects/shared-function-info.tq

@@ -14,10 +14,9 @@ extern class InterpreterData extends Struct {
   interpreter_trampoline: Code;
 }
 
-type FunctionKind extends uint8 constexpr 'v8::internal::FunctionKind';
-type FunctionSyntaxKind extends uint8
-constexpr 'v8::internal::FunctionSyntaxKind';
-type BailoutReason extends uint8 constexpr 'v8::internal::BailoutReason';
+type FunctionKind extends uint8 constexpr 'FunctionKind';
+type FunctionSyntaxKind extends uint8 constexpr 'FunctionSyntaxKind';
+type BailoutReason extends uint8 constexpr 'BailoutReason';
 
 bitfield struct SharedFunctionInfoFlags extends uint32 {
   // Have FunctionKind first to make it cheaper to access.

src/torque/class-debug-reader-generator.cc

@@ -128,21 +128,19 @@ class DebugFieldType {
     if (IsTagged()) {
       return storage == kAsStoredInHeap ? "i::Tagged_t" : "uintptr_t";
     }
-    // Note that we need constexpr names to resolve correctly in the global
-    // namespace, because we're passing them as strings to a debugging
-    // extension. We can verify this during build of the debug helper, because
-    // we use this type for a local variable below, and generate this code in
-    // a disjoint namespace. However, we can't emit a useful error at this
-    // point. Instead we'll emit a comment that might be helpful.
+
+    // We can't emit a useful error at this point if the constexpr type name is
+    // wrong, but we can include a comment that might be helpful.
     return GetOriginalType(storage) +
-           " /*Failing? Ensure constexpr type name is fully qualified and "
-           "necessary #includes are in debug-helper-internal.h*/";
+           " /*Failing? Ensure constexpr type name is correct, and the "
+           "necessary #include is in any .tq file*/";
   }
 
   // Returns the type that should be used to represent a field's type to
   // debugging tools that have full V8 symbols. The types returned from this
-  // method are fully qualified and may refer to object types that are not
-  // included in the compilation of the debug helper library.
+  // method are resolveable in the v8::internal namespace and may refer to
+  // object types that are not included in the compilation of the debug helper
+  // library.
   std::string GetOriginalType(TypeStorage storage) const {
     if (name_and_type_.type->IsStructType()) {
       // There's no meaningful type we could use here, because the V8 symbols
@@ -164,6 +162,26 @@ class DebugFieldType {
     return name_and_type_.type->GetConstexprGeneratedTypeName();
   }
 
+  // Returns a C++ expression that evaluates to a string (type `const char*`)
+  // containing the name of the field's type. The types returned from this
+  // method are resolveable in the v8::internal namespace and may refer to
+  // object types that are not included in the compilation of the debug helper
+  // library.
+  std::string GetTypeString(TypeStorage storage) const {
+    if (IsTagged() || name_and_type_.type->IsStructType()) {
+      // Wrap up the original type in a string literal.
+      return "\"" + GetOriginalType(storage) + "\"";
+    }
+
+    // We require constexpr type names to be resolvable in the v8::internal
+    // namespace, according to the contract in debug-helper.h. In order to
+    // verify at compile time that constexpr type names are resolvable, we use
+    // the type name as a dummy template parameter to a function that just
+    // returns its parameter.
+    return "CheckTypeName<" + GetValueType(storage) + ">(\"" +
+           GetOriginalType(storage) + "\")";
+  }
+
   // Returns the field's size in bytes.
   size_t GetSize() const {
     auto opt_size = SizeOf(name_and_type_.type);
@@ -346,10 +364,9 @@ void GenerateGetPropsChunkForField(const Field& field,
                                      struct_field.pos);
     get_props_impl << "  " << struct_field_list
                    << ".push_back(std::make_unique<StructProperty>(\""
-                   << struct_field.name_and_type.name << "\", \""
-                   << struct_field_type.GetOriginalType(kAsStoredInHeap)
-                   << "\", \""
-                   << struct_field_type.GetOriginalType(kUncompressed) << "\", "
+                   << struct_field.name_and_type.name << "\", "
+                   << struct_field_type.GetTypeString(kAsStoredInHeap) << ", "
+                   << struct_field_type.GetTypeString(kUncompressed) << ", "
                    << struct_field.offset_bytes << ", " << struct_field.num_bits
                    << ", " << struct_field.shift_bits << "));\n";
   }
@@ -383,11 +400,11 @@ void GenerateGetPropsChunkForField(const Field& field,
   }
 
   get_props_impl << "  result.push_back(std::make_unique<ObjectProperty>(\""
-                 << field.name_and_type.name << "\", \""
-                 << debug_field_type.GetOriginalType(kAsStoredInHeap)
-                 << "\", \"" << debug_field_type.GetOriginalType(kUncompressed)
-                 << "\", " << debug_field_type.GetAddressGetter() << "(), "
-                 << count_value << ", " << debug_field_type.GetSize() << ", "
+                 << field.name_and_type.name << "\", "
+                 << debug_field_type.GetTypeString(kAsStoredInHeap) << ", "
+                 << debug_field_type.GetTypeString(kUncompressed) << ", "
+                 << debug_field_type.GetAddressGetter() << "(), " << count_value
+                 << ", " << debug_field_type.GetSize() << ", "
                  << struct_field_list << ", " << property_kind << "));\n";
 }
 
@@ -537,12 +554,17 @@ void ImplementationVisitor::GenerateClassDebugReaders(
     h_contents << "#undef GetBValue\n";
     h_contents << "#endif\n\n";
 
+    for (const std::string& include_path : GlobalContext::CppIncludes()) {
+      cc_contents << "#include " << StringLiteralQuote(include_path) << "\n";
+    }
     cc_contents << "#include \"torque-generated/" << file_name << ".h\"\n";
     cc_contents << "#include \"include/v8-internal.h\"\n\n";
     cc_contents << "namespace i = v8::internal;\n\n";
 
-    NamespaceScope h_namespaces(h_contents, {"v8_debug_helper_internal"});
-    NamespaceScope cc_namespaces(cc_contents, {"v8_debug_helper_internal"});
+    NamespaceScope h_namespaces(h_contents,
+                                {"v8", "internal", "debug_helper_internal"});
+    NamespaceScope cc_namespaces(cc_contents,
+                                 {"v8", "internal", "debug_helper_internal"});
 
     std::stringstream visitor;
     visitor << "\nclass TqObjectVisitor {\n";

src/wasm/wasm-objects.tq

@@ -74,7 +74,7 @@ extern class WasmMemoryObject extends JSObject {
   instances: WeakArrayList|Undefined;
 }
 
-type WasmValueType extends uint8 constexpr 'v8::internal::wasm::ValueType';
+type WasmValueType extends uint8 constexpr 'wasm::ValueType';
 bitfield struct WasmGlobalObjectFlags extends uint31 {
   Type: WasmValueType: 8 bit;  // "type" is a reserved word.
   is_mutable: bool: 1 bit;

test/cctest/test-debug-helper.cc

@@ -372,8 +372,8 @@ TEST(GetObjectProperties) {
       heap_addresses);
   const d::ObjectProperty& flags = FindProp(*props, "flags");
   CHECK_GE(flags.num_struct_fields, 3);
-  CheckStructProp(*flags.struct_fields[0], "v8::internal::FunctionKind",
-                  "function_kind", 0, 5, 0);
+  CheckStructProp(*flags.struct_fields[0], "FunctionKind", "function_kind", 0,
+                  5, 0);
   CheckStructProp(*flags.struct_fields[1], "bool", "is_native", 0, 1, 5);
   CheckStructProp(*flags.struct_fields[2], "bool", "is_strict", 0, 1, 6);
 

tools/debug_helper/debug-helper-internal.cc

@@ -8,7 +8,9 @@
 
 namespace i = v8::internal;
 
-namespace v8_debug_helper_internal {
+namespace v8 {
+namespace internal {
+namespace debug_helper_internal {
 
 bool IsPointerCompressed(uintptr_t address) {
 #if COMPRESS_POINTERS_BOOL
@@ -58,4 +60,6 @@ bool TqObject::IsSuperclassOf(const TqObject* other) const {
   return GetName() != other->GetName();
 }
 
-}  // namespace v8_debug_helper_internal
+}  // namespace debug_helper_internal
+}  // namespace internal
+}  // namespace v8

tools/debug_helper/debug-helper-internal.h

@@ -19,7 +19,9 @@
 
 namespace d = v8::debug_helper;
 
-namespace v8_debug_helper_internal {
+namespace v8 {
+namespace internal {
+namespace debug_helper_internal {
 
 // A value that was read from the debuggee's memory.
 template <typename TValue>
@@ -119,7 +121,7 @@ class ObjectProperty : public PropertyBase {
 class ObjectPropertiesResult;
 struct ObjectPropertiesResultExtended : public d::ObjectPropertiesResult {
   // Back reference for cleanup.
-  v8_debug_helper_internal::ObjectPropertiesResult* base;
+  debug_helper_internal::ObjectPropertiesResult* base;
 };
 
 // Internal version of API class v8::debug_helper::ObjectPropertiesResult.
@@ -189,6 +191,14 @@ class TqObject {
   uintptr_t address_;
 };
 
+// A helpful template so that generated code can be sure that a string type name
+// actually resolves to a type, by repeating the name as the template parameter
+// and the value.
+template <typename T>
+const char* CheckTypeName(const char* name) {
+  return name;
+}
+
 // In ptr-compr builds, returns whether the address looks like a compressed
 // pointer (zero-extended from 32 bits). Otherwise returns false because no
 // pointers can be compressed.
@@ -207,6 +217,8 @@ d::PropertyKind GetArrayKind(d::MemoryAccessResult mem_result);
 // Torque class definitions.
 extern const d::ClassList kObjectClassList;
 
-}  // namespace v8_debug_helper_internal
+}  // namespace debug_helper_internal
+}  // namespace internal
+}  // namespace v8
 
 #endif

tools/debug_helper/debug-helper.h

@@ -71,7 +71,12 @@ struct PropertyBase {
 
   // Statically-determined type, such as from .tq definition. Can be an empty
   // string if this property is itself a Torque-defined struct; in that case use
-  // |struct_fields| instead.
+  // |struct_fields| instead. This type should be treated as if it were used in
+  // the v8::internal namespace; that is, type "X::Y" can mean any of the
+  // following, in order of decreasing preference:
+  // - v8::internal::X::Y
+  // - v8::X::Y
+  // - X::Y
   const char* type;
 
   // In some cases, |type| may be a simple type representing a compressed

tools/debug_helper/gen-heap-constants.py

@@ -19,7 +19,9 @@ out = """
 #include "src/common/ptr-compr-inl.h"
 #include "tools/debug_helper/debug-helper-internal.h"
 
-namespace v8_debug_helper_internal {
+namespace v8 {
+namespace internal {
+namespace debug_helper_internal {
 """
 
 def iterate_objects(target_space, camel_space_name):
@@ -70,7 +72,7 @@ if (hasattr(v8heapconst, 'HEAP_FIRST_PAGES')):  # Only exists in ptr-compr build
       out = out + '  }\n'
 out = out + '}\n'
 
-out = out + '\n}\n'
+out = out + '\n}\n}\n}\n'
 
 try:
   with open(sys.argv[2], "r") as out_file:

tools/debug_helper/get-object-properties.cc

@@ -14,7 +14,9 @@
 
 namespace i = v8::internal;
 
-namespace v8_debug_helper_internal {
+namespace v8 {
+namespace internal {
+namespace debug_helper_internal {
 
 constexpr char kObject[] = "v8::internal::Object";
 constexpr char kTaggedValue[] = "v8::internal::TaggedValue";
@@ -591,9 +593,11 @@ std::unique_ptr<ObjectPropertiesResult> GetObjectProperties(
                                                   stream.str(), kSmi);
 }
 
-}  // namespace v8_debug_helper_internal
+}  // namespace debug_helper_internal
+}  // namespace internal
+}  // namespace v8
 
-namespace di = v8_debug_helper_internal;
+namespace di = v8::internal::debug_helper_internal;
 
 extern "C" {
 V8_DEBUG_HELPER_EXPORT d::ObjectPropertiesResult*

tools/debug_helper/heap-constants.cc

@@ -7,7 +7,9 @@
 
 namespace d = v8::debug_helper;
 
-namespace v8_debug_helper_internal {
+namespace v8 {
+namespace internal {
+namespace debug_helper_internal {
 
 std::string FindKnownObject(uintptr_t address,
                             const d::HeapAddresses& heap_addresses) {
@@ -82,4 +84,6 @@ KnownInstanceType FindKnownMapInstanceTypes(
   return result;
 }
 
-}  // namespace v8_debug_helper_internal
+}  // namespace debug_helper_internal
+}  // namespace internal
+}  // namespace v8

tools/debug_helper/heap-constants.h

@@ -14,7 +14,9 @@
 
 namespace d = v8::debug_helper;
 
-namespace v8_debug_helper_internal {
+namespace v8 {
+namespace internal {
+namespace debug_helper_internal {
 
 // ===== Functions generated by gen-heap-constants.py: =========================
 
@@ -62,6 +64,8 @@ struct KnownInstanceType {
 KnownInstanceType FindKnownMapInstanceTypes(
     uintptr_t address, const d::HeapAddresses& heap_addresses);
 
-}  // namespace v8_debug_helper_internal
+}  // namespace debug_helper_internal
+}  // namespace internal
+}  // namespace v8
 
 #endif

tools/debug_helper/list-object-classes.cc

@@ -5,7 +5,7 @@
 #include "debug-helper-internal.h"
 #include "torque-generated/class-debug-readers-tq.h"
 
-namespace di = v8_debug_helper_internal;
+namespace di = v8::internal::debug_helper_internal;
 
 extern "C" {
 V8_DEBUG_HELPER_EXPORT const d::ClassList*

tools/v8windbg/src/v8-debug-helper-interop.h

@@ -40,7 +40,12 @@ struct StructField {
 
   std::u16string name;
 
-  // Statically-determined type, such as from .tq definition.
+  // Statically-determined type, such as from .tq definition. This type should
+  // be treated as if it were used in the v8::internal namespace; that is, type
+  // "X::Y" can mean any of the following, in order of decreasing preference:
+  // - v8::internal::X::Y
+  // - v8::X::Y
+  // - X::Y
   std::u16string type_name;
 
   // In some cases, |type_name| may be a simple type representing a compressed
@@ -78,7 +83,12 @@ struct Property {
 
   // Statically-determined type, such as from .tq definition. Can be an empty
   // string if this property is itself a Torque-defined struct; in that case use
-  // |fields| instead.
+  // |fields| instead. This type should be treated as if it were used in the
+  // v8::internal namespace; that is, type "X::Y" can mean any of the following,
+  // in order of decreasing preference:
+  // - v8::internal::X::Y
+  // - v8::X::Y
+  // - X::Y
   std::u16string type_name;
 
   // In some cases, |type_name| may be a simple type representing a compressed

tools/v8windbg/src/v8windbg-extension.cc

@@ -80,6 +80,20 @@ WRL::ComPtr<IDebugHostType> Extension::GetTypeFromV8Module(
 
   auto& dictionary_entry = cached_v8_module_types_[type_name];
   if (dictionary_entry == nullptr) {
+    const std::wstring type_name_w(reinterpret_cast<const wchar_t*>(type_name));
+    // The contract from debug_helper functions is to provide type names that
+    // would be valid if used in C++ code within the v8::internal namespace.
+    // They might be fully qualified but aren't required to be. Thus, we must
+    // simluate an "unqualified name lookup" here, by searching for the type
+    // starting in the innermost namespace and working outward.
+    if (SUCCEEDED(sp_v8_module_->FindTypeByName(
+            (L"v8::internal::" + type_name_w).c_str(), &dictionary_entry))) {
+      return dictionary_entry;
+    }
+    if (SUCCEEDED(sp_v8_module_->FindTypeByName((L"v8::" + type_name_w).c_str(),
+                                                &dictionary_entry))) {
+      return dictionary_entry;
+    }
     sp_v8_module_->FindTypeByName(reinterpret_cast<PCWSTR>(type_name),
                                   &dictionary_entry);
   }