[logging] Systematically emit CodeCreateEvents for builtins

Introduce a single point to emit CodeCreateEvents for all builtins in
Isolate::Init. At this location, we cover both the case of builtin generation
(e.g. in mksnapshot) and deserialized builtins (in standard builds),
whereas previously we only emitted events post-builtin-generation.

In order to preserve behavior for bytecode handler events, pack the bytecode
and operand scale into our existing builtin metadata table.

Drive-by: Update way-out-of-date comment in the static initializer
check.

Bug: v8:8674
Change-Id: Iced8f73568e920846cde6f7b0a9c1e61844258ad
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1627337
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Dan Elphick <delphick@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61782}

src/builtins/builtins.cc

@@ -11,6 +11,9 @@
 #include "src/codegen/macro-assembler.h"
 #include "src/diagnostics/code-tracer.h"
 #include "src/execution/isolate.h"
+#include "src/interpreter/bytecodes.h"
+#include "src/logging/code-events.h"  // For CodeCreateEvent.
+#include "src/logging/log.h"          // For Logger.
 #include "src/objects/fixed-array.h"
 #include "src/objects/objects-inl.h"
 #include "src/ostreams.h"
@@ -32,20 +35,46 @@ namespace {
 struct BuiltinMetadata {
   const char* name;
   Builtins::Kind kind;
-  // For CPP builtins it's cpp_entry address and for TFJ it's a
-  // parameter count.
-  Address cpp_entry_or_parameter_count;
+
+  struct BytecodeAndScale {
+    interpreter::Bytecode bytecode : 8;
+    interpreter::OperandScale scale : 8;
+  };
+
+  STATIC_ASSERT(sizeof(interpreter::Bytecode) == 1);
+  STATIC_ASSERT(sizeof(interpreter::OperandScale) == 1);
+  STATIC_ASSERT(sizeof(BytecodeAndScale) <= sizeof(Address));
+
+  // The `data` field has kind-specific contents.
+  union KindSpecificData {
+    // TODO(jgruber): Union constructors are needed since C++11 does not support
+    // designated initializers (e.g.: {.parameter_count = count}). Update once
+    // we're at C++20 :)
+    // The constructors are marked constexpr to avoid the need for a static
+    // initializer for builtins.cc (see check-static-initializers.sh).
+    constexpr KindSpecificData() : cpp_entry(kNullAddress) {}
+    constexpr KindSpecificData(Address cpp_entry) : cpp_entry(cpp_entry) {}
+    constexpr KindSpecificData(int parameter_count,
+                               int /* To disambiguate from above */)
+        : parameter_count(static_cast<int16_t>(parameter_count)) {}
+    constexpr KindSpecificData(interpreter::Bytecode bytecode,
+                               interpreter::OperandScale scale)
+        : bytecode_and_scale{bytecode, scale} {}
+    Address cpp_entry;                    // For CPP builtins.
+    int16_t parameter_count;              // For TFJ builtins.
+    BytecodeAndScale bytecode_and_scale;  // For BCH builtins.
+  } data;
 };
 
 #define DECL_CPP(Name, ...) \
-  {#Name, Builtins::CPP, FUNCTION_ADDR(Builtin_##Name)},
-#define DECL_TFJ(Name, Count, ...) \
-  {#Name, Builtins::TFJ, static_cast<Address>(Count)},
-#define DECL_TFC(Name, ...) {#Name, Builtins::TFC, kNullAddress},
-#define DECL_TFS(Name, ...) {#Name, Builtins::TFS, kNullAddress},
-#define DECL_TFH(Name, ...) {#Name, Builtins::TFH, kNullAddress},
-#define DECL_BCH(Name, ...) {#Name, Builtins::BCH, kNullAddress},
-#define DECL_ASM(Name, ...) {#Name, Builtins::ASM, kNullAddress},
+  {#Name, Builtins::CPP, {FUNCTION_ADDR(Builtin_##Name)}},
+#define DECL_TFJ(Name, Count, ...) {#Name, Builtins::TFJ, {Count, 0}},
+#define DECL_TFC(Name, ...) {#Name, Builtins::TFC, {}},
+#define DECL_TFS(Name, ...) {#Name, Builtins::TFS, {}},
+#define DECL_TFH(Name, ...) {#Name, Builtins::TFH, {}},
+#define DECL_BCH(Name, OperandScale, Bytecode) \
+  {#Name, Builtins::BCH, {Bytecode, OperandScale}},
+#define DECL_ASM(Name, ...) {#Name, Builtins::ASM, {}},
 const BuiltinMetadata builtin_metadata[] = {BUILTIN_LIST(
     DECL_CPP, DECL_TFJ, DECL_TFC, DECL_TFS, DECL_TFH, DECL_BCH, DECL_ASM)};
 #undef DECL_CPP
@@ -125,7 +154,7 @@ Handle<Code> Builtins::builtin_handle(int index) {
 // static
 int Builtins::GetStackParameterCount(Name name) {
   DCHECK(Builtins::KindOf(name) == TFJ);
-  return static_cast<int>(builtin_metadata[name].cpp_entry_or_parameter_count);
+  return builtin_metadata[name].data.parameter_count;
 }
 
 // static
@@ -192,7 +221,7 @@ void Builtins::PrintBuiltinSize() {
 // static
 Address Builtins::CppEntryOf(int index) {
   DCHECK(Builtins::IsCpp(index));
-  return builtin_metadata[index].cpp_entry_or_parameter_count;
+  return builtin_metadata[index].data.cpp_entry;
 }
 
 // static
@@ -249,6 +278,35 @@ void Builtins::UpdateBuiltinEntryTable(Isolate* isolate) {
   }
 }
 
+// static
+void Builtins::EmitCodeCreateEvents(Isolate* isolate) {
+  if (!isolate->logger()->is_listening_to_code_events() &&
+      !isolate->is_profiling()) {
+    return;  // No need to iterate the entire table in this case.
+  }
+
+  Address* builtins = isolate->builtins_table();
+  int i = 0;
+  for (; i < kFirstBytecodeHandler; i++) {
+    auto code = AbstractCode::cast(Object(builtins[i]));
+    PROFILE(isolate, CodeCreateEvent(CodeEventListener::BUILTIN_TAG, code,
+                                     Builtins::name(i)));
+  }
+
+  STATIC_ASSERT(kLastBytecodeHandlerPlusOne == builtin_count);
+  for (; i < builtin_count; i++) {
+    auto code = AbstractCode::cast(Object(builtins[i]));
+    interpreter::Bytecode bytecode =
+        builtin_metadata[i].data.bytecode_and_scale.bytecode;
+    interpreter::OperandScale scale =
+        builtin_metadata[i].data.bytecode_and_scale.scale;
+    PROFILE(isolate,
+            CodeCreateEvent(
+                CodeEventListener::BYTECODE_HANDLER_TAG, code,
+                interpreter::Bytecodes::ToString(bytecode, scale).c_str()));
+  }
+}
+
 namespace {
 
 class OffHeapTrampolineGenerator {

src/builtins/builtins.h

@@ -60,6 +60,14 @@ class Builtins {
 
   static const int32_t kNoBuiltinId = -1;
 
+  static constexpr int kFirstWideBytecodeHandler =
+      kFirstBytecodeHandler + kNumberOfBytecodeHandlers;
+  static constexpr int kFirstExtraWideBytecodeHandler =
+      kFirstWideBytecodeHandler + kNumberOfWideBytecodeHandlers;
+  static constexpr int kLastBytecodeHandlerPlusOne =
+      kFirstExtraWideBytecodeHandler + kNumberOfWideBytecodeHandlers;
+  STATIC_ASSERT(kLastBytecodeHandlerPlusOne == builtin_count);
+
   static constexpr bool IsBuiltinId(int maybe_id) {
     return 0 <= maybe_id && maybe_id < builtin_count;
   }
@@ -114,14 +122,6 @@ class Builtins {
   // True, iff the given code object is a builtin with off-heap embedded code.
   static bool IsIsolateIndependentBuiltin(const Code code);
 
-  static constexpr int kFirstWideBytecodeHandler =
-      kFirstBytecodeHandler + kNumberOfBytecodeHandlers;
-  static constexpr int kFirstExtraWideBytecodeHandler =
-      kFirstWideBytecodeHandler + kNumberOfWideBytecodeHandlers;
-  STATIC_ASSERT(kFirstExtraWideBytecodeHandler +
-                    kNumberOfWideBytecodeHandlers ==
-                builtin_count);
-
   // True, iff the given builtin contains no isolate-specific code and can be
   // embedded into the binary.
   static constexpr bool kAllBuiltinsAreIsolateIndependent = true;
@@ -142,6 +142,9 @@ class Builtins {
   // the builtins table.
   static void UpdateBuiltinEntryTable(Isolate* isolate);
 
+  // Emits a CodeCreateEvent for every builtin.
+  static void EmitCodeCreateEvents(Isolate* isolate);
+
   bool is_initialized() const { return initialized_; }
 
   // Used by SetupIsolateDelegate and Deserializer.

src/builtins/setup-builtins-internal.cc

@@ -15,7 +15,6 @@
 #include "src/interpreter/bytecodes.h"
 #include "src/interpreter/interpreter-generator.h"
 #include "src/interpreter/interpreter.h"
-#include "src/logging/code-events.h"
 #include "src/objects/objects-inl.h"
 #include "src/objects/shared-function-info.h"
 #include "src/objects/smi.h"
@@ -31,11 +30,6 @@ BUILTIN_LIST_C(FORWARD_DECLARE)
 
 namespace {
 
-void PostBuildProfileAndTracing(Isolate* isolate, Code code, const char* name) {
-  PROFILE(isolate, CodeCreateEvent(CodeEventListener::BUILTIN_TAG,
-                                   AbstractCode::cast(code), name));
-}
-
 AssemblerOptions BuiltinAssemblerOptions(Isolate* isolate,
                                          int32_t builtin_index) {
   AssemblerOptions options = AssemblerOptions::Default(isolate);
@@ -128,7 +122,6 @@ Code BuildWithMacroAssembler(Isolate* isolate, int32_t builtin_index,
 #if defined(V8_OS_WIN_X64)
   isolate->SetBuiltinUnwindData(builtin_index, masm.GetUnwindInfo());
 #endif
-  PostBuildProfileAndTracing(isolate, *code, s_name);
   return *code;
 }
 
@@ -152,7 +145,6 @@ Code BuildAdaptor(Isolate* isolate, int32_t builtin_index,
                           .set_self_reference(masm.CodeObject())
                           .set_builtin_index(builtin_index)
                           .Build();
-  PostBuildProfileAndTracing(isolate, *code, name);
   return *code;
 }
 
@@ -177,7 +169,6 @@ Code BuildWithCodeStubAssemblerJS(Isolate* isolate, int32_t builtin_index,
   generator(&state);
   Handle<Code> code = compiler::CodeAssembler::GenerateCode(
       &state, BuiltinAssemblerOptions(isolate, builtin_index));
-  PostBuildProfileAndTracing(isolate, *code, name);
   return *code;
 }
 
@@ -205,7 +196,6 @@ Code BuildWithCodeStubAssemblerCS(Isolate* isolate, int32_t builtin_index,
   generator(&state);
   Handle<Code> code = compiler::CodeAssembler::GenerateCode(
       &state, BuiltinAssemblerOptions(isolate, builtin_index));
-  PostBuildProfileAndTracing(isolate, *code, name);
   return *code;
 }
 
@@ -284,13 +274,9 @@ Code GenerateBytecodeHandler(Isolate* isolate, int builtin_index,
                              interpreter::OperandScale operand_scale,
                              interpreter::Bytecode bytecode) {
   DCHECK(interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
-
   Handle<Code> code = interpreter::GenerateBytecodeHandler(
       isolate, bytecode, operand_scale, builtin_index,
       BuiltinAssemblerOptions(isolate, builtin_index));
-
-  PostBuildProfileAndTracing(isolate, *code, name);
-
   return *code;
 }
 

src/execution/isolate.cc

@@ -3189,11 +3189,6 @@ void CreateOffHeapTrampolines(Isolate* isolate) {
     // From this point onwards, the old builtin code object is unreachable and
     // will be collected by the next GC.
     builtins->set_builtin(i, *trampoline);
-
-    if (isolate->logger()->is_listening_to_code_events() ||
-        isolate->is_profiling()) {
-      isolate->logger()->LogCodeObject(*trampoline);
-    }
   }
 }
 
@@ -3454,8 +3449,8 @@ bool Isolate::Init(ReadOnlyDeserializer* read_only_deserializer,
   delete setup_delegate_;
   setup_delegate_ = nullptr;
 
-  // Initialize the builtin entry table.
   Builtins::UpdateBuiltinEntryTable(this);
+  Builtins::EmitCodeCreateEvents(this);
 
 #ifdef DEBUG
   // Verify that the current heap state (usually deserialized from the snapshot)

src/interpreter/interpreter-generator.cc

@@ -19,7 +19,6 @@
 #include "src/interpreter/bytecodes.h"
 #include "src/interpreter/interpreter-assembler.h"
 #include "src/interpreter/interpreter-intrinsics-generator.h"
-#include "src/logging/code-events.h"
 #include "src/objects/cell.h"
 #include "src/objects/js-generator.h"
 #include "src/objects/module.h"
@@ -3292,10 +3291,7 @@ Handle<Code> GenerateBytecodeHandler(Isolate* isolate, Bytecode bytecode,
   }
 
   Handle<Code> code = compiler::CodeAssembler::GenerateCode(&state, options);
-  PROFILE(isolate, CodeCreateEvent(
-                       CodeEventListener::BYTECODE_HANDLER_TAG,
-                       AbstractCode::cast(*code),
-                       Bytecodes::ToString(bytecode, operand_scale).c_str()));
+
 #ifdef ENABLE_DISASSEMBLER
   if (FLAG_trace_ignition_codegen) {
     StdoutStream os;
@@ -3303,6 +3299,7 @@ Handle<Code> GenerateBytecodeHandler(Isolate* isolate, Bytecode bytecode,
     os << std::flush;
   }
 #endif  // ENABLE_DISASSEMBLER
+
   return code;
 }
 

tools/check-static-initializers.sh

@@ -30,8 +30,8 @@
 # initializer in d8 matches the one defined below.
 
 # Allow:
-#  - _GLOBAL__I__ZN2v810LineEditor6first_E
-#  - _GLOBAL__I__ZN2v88internal32AtomicOps_Internalx86CPUFeaturesE
+# _GLOBAL__sub_I_d8.cc
+# _GLOBAL__sub_I_iostream.cpp
 expected_static_init_count=2
 
 v8_root=$(readlink -f $(dirname $BASH_SOURCE)/../)