Log EValue tag names instead of numeric values

Differential Revision: D67888756

Pull Request resolved: #7538

Author: GregoryComer

runtime/core/defines.h

@@ -0,0 +1,20 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+/**
+ * @file
+ * Contains preprocessor definitions used by ExecuTorch core.
+ */
+
+#pragma once
+
+// Enable ET_ENABLE_ENUM_STRINGS by default. This option gates inclusion of
+// enum string names and can be disabled by explicitly setting it to 0.
+#ifndef ET_ENABLE_ENUM_STRINGS
+#define ET_ENABLE_ENUM_STRINGS 1
+#endif

runtime/core/tag.cpp

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <executorch/runtime/core/tag.h>
+
+#include <cstdio>
+
+namespace executorch {
+namespace runtime {
+
+/**
+ * Convert a tag value to a string representation. If ET_ENABLE_ENUM_STRINGS is
+ * set (it is on by default), this will return a string name (for example,
+ * "Tensor"). Otherwise, it will return a string representation of the index
+ * value ("1").
+ *
+ * If the user buffer is not large enough to hold the string representation, the
+ * string will be truncated.
+ *
+ * The return value is the number of characters written, or in the case of
+ * truncation, the number of characters that would be written if the buffer was
+ * large enough.
+ */
+size_t tag_to_string(Tag tag, char* buffer, size_t buffer_size) {
+#if ET_ENABLE_ENUM_STRINGS
+  const char* name_str;
+#define DEFINE_CASE(name) \
+  case Tag::name:         \
+    name_str = #name;     \
+    break;
+
+  switch (tag) {
+    EXECUTORCH_FORALL_TAGS(DEFINE_CASE)
+    default:
+      name_str = "Unknown";
+      break;
+  }
+
+  return snprintf(buffer, buffer_size, "%s", name_str);
+#undef DEFINE_CASE
+#else
+  return snprintf(buffer, buffer_size, "%d", static_cast<int>(tag));
+#endif // ET_ENABLE_ENUM_TO_STRING
+}
+
+} // namespace runtime
+} // namespace executorch

runtime/core/tag.h

@@ -8,6 +8,8 @@
 
 #pragma once
 
+#include <executorch/runtime/core/defines.h>
+#include <executorch/runtime/platform/compiler.h>
 #include <cstdint>
 
 namespace executorch {
@@ -36,6 +38,27 @@ enum class Tag : uint32_t {
 #undef DEFINE_TAG
 };
 
+/**
+ * Convert a tag value to a string representation. If ET_ENABLE_ENUM_STRINGS is
+ * set (it is on by default), this will return a string name (for example,
+ * "Tensor"). Otherwise, it will return a string representation of the index
+ * value ("1").
+ *
+ * If the user buffer is not large enough to hold the string representation, the
+ * string will be truncated.
+ *
+ * The return value is the number of characters written, or in the case of
+ * truncation, the number of characters that would be written if the buffer was
+ * large enough.
+ */
+size_t tag_to_string(Tag tag, char* buffer, size_t buffer_size);
+
+/* The size of the buffer needed to hold the longest tag string, including the
+ * null terminator. This value is expected to be updated manually, but it
+ * checked in test_tag.cpp.
+ */
+constexpr size_t kTagNameBufferSize = 19;
+
 } // namespace runtime
 } // namespace executorch
 

runtime/core/targets.bzl

@@ -18,6 +18,14 @@ def get_sdk_flags():
         sdk_flags += ["-DEXECUTORCH_BUILD_DEVTOOLS"]
     return sdk_flags
 
+def enable_enum_strings():
+    return native.read_config("executorch", "enable_enum_strings", "true") == "true"
+
+def get_core_flags():
+    core_flags = []
+    core_flags += ["-DET_ENABLE_ENUM_STRINGS=" + ("1" if enable_enum_strings() else "0")]
+    return core_flags
+
 def define_common_targets():
     """Defines targets that should be shared between fbcode and xplat.
 
@@ -30,6 +38,7 @@ def define_common_targets():
         exported_headers = [
             "array_ref.h",  # TODO(T157717874): Migrate all users to span and then move this to portable_type
             "data_loader.h",
+            "defines.h",
             "error.h",
             "freeable_buffer.h",
             "result.h",
@@ -39,6 +48,7 @@ def define_common_targets():
             "//executorch/...",
             "@EXECUTORCH_CLIENTS",
         ],
+        exported_preprocessor_flags = get_core_flags(),
         exported_deps = [
             "//executorch/runtime/platform:platform",
         ],
@@ -109,9 +119,14 @@ def define_common_targets():
 
     runtime.cxx_library(
         name = "tag",
+        srcs = ["tag.cpp"],
         exported_headers = [
             "tag.h",
         ],
+        exported_deps = [
+            ":core",
+            "//executorch/runtime/platform:compiler",
+        ],
         visibility = [
             "//executorch/...",
         ],

runtime/core/test/tag_test.cpp

@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <executorch/runtime/core/tag.h>
+
+#include <gtest/gtest.h>
+#include <array>
+
+using namespace ::testing;
+using executorch::runtime::kTagNameBufferSize;
+using executorch::runtime::Tag;
+using executorch::runtime::tag_to_string;
+
+// The behavior of tag_to_string depends on the value of ET_ENABLE_ENUM_STRINGS.
+// If it is not set, tag_to_string will return a string representation of the
+// enum index value. As this behavior is compile-time gated, tests must also
+// be compile-time gated.
+#if ET_ENABLE_ENUM_STRINGS
+TEST(TagToString, TagValues) {
+  std::array<char, 16> name;
+
+  tag_to_string(Tag::Tensor, name.data(), name.size());
+  EXPECT_STREQ("Tensor", name.data());
+
+  tag_to_string(Tag::Int, name.data(), name.size());
+  EXPECT_STREQ("Int", name.data());
+
+  tag_to_string(Tag::Double, name.data(), name.size());
+  EXPECT_STREQ("Double", name.data());
+
+  tag_to_string(Tag::Bool, name.data(), name.size());
+  EXPECT_STREQ("Bool", name.data());
+}
+
+TEST(TagToString, TagNameBufferSize) {
+  // Validate that kTagNameBufferSize is large enough to hold the all tag
+  // strings without truncation.
+  std::array<char, kTagNameBufferSize> name;
+
+  // Note that the return value of tag_to_string does not include the null
+  // terminator.
+  size_t longest = 0;
+
+#define TEST_CASE(tag)                                                \
+  auto tag##_len = tag_to_string(Tag::tag, name.data(), name.size()); \
+  EXPECT_LT(tag##_len, kTagNameBufferSize)                            \
+      << "kTagNameBufferSize is too small to hold " #tag;             \
+  longest = std::max(longest, tag##_len);
+
+  EXECUTORCH_FORALL_TAGS(TEST_CASE)
+#undef TEST_CASE
+
+  EXPECT_EQ(longest + 1, kTagNameBufferSize)
+      << "kTagNameBufferSize has incorrect value, expected " << longest + 1;
+}
+
+TEST(TagToString, FitsExact) {
+  std::array<char, 4> name;
+
+  auto ret = tag_to_string(Tag::Int, name.data(), name.size());
+
+  EXPECT_EQ(3, ret);
+  EXPECT_STREQ("Int", name.data());
+}
+
+TEST(TagToString, Truncate) {
+  std::array<char, 6> name;
+  std::fill(name.begin(), name.end(), '-');
+
+  auto ret = tag_to_string(Tag::Double, name.data(), name.size());
+  EXPECT_EQ(6, ret);
+  EXPECT_TRUE(name[name.size() - 1] == 0);
+  EXPECT_STREQ("Doubl", name.data());
+}
+#endif // ET_ENABLE_ENUM_STRINGS

runtime/core/test/targets.bzl

@@ -73,6 +73,16 @@ def define_common_targets():
         ],
     )
 
+    runtime.cxx_test(
+        name = "tag_test",
+        srcs = [
+            "tag_test.cpp",
+        ],
+        deps = [
+            "//executorch/runtime/core:tag",
+        ],
+    )
+
     runtime.cxx_test(
         name = "tensor_shape_dynamism_test_aten",
         srcs = ["tensor_shape_dynamism_test_aten.cpp"],

runtime/executor/method.cpp

@@ -8,6 +8,7 @@
 
 #include <executorch/runtime/executor/method.h>
 
+#include <array>
 #include <cinttypes> // @donotremove
 #include <cstdint>
 #include <cstdio>
@@ -823,26 +824,43 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) {
   ET_CHECK_OR_RETURN_ERROR(
       input_idx < inputs_size(),
       InvalidArgument,
-      "Given input index must be less than the number of inputs in method, but got %zu and %zu",
+      "Input index (%zu) must be less than the number of inputs in method (%zu).",
       input_idx,
       inputs_size());
 
   const auto& e = get_value(get_input_index(input_idx));
-  ET_CHECK_OR_RETURN_ERROR(
-      e.isTensor() || e.isScalar(),
-      InvalidArgument,
-      "The %zu-th input in method is expected Tensor or prim, but received %" PRIu32,
-      input_idx,
-      static_cast<uint32_t>(e.tag));
 
-  ET_CHECK_OR_RETURN_ERROR(
-      e.tag == input_evalue.tag,
-      InvalidArgument,
-      "The %zu-th input of method should have the same type as the input_evalue, but get tag %" PRIu32
-      " and tag %" PRIu32,
-      input_idx,
-      static_cast<uint32_t>(e.tag),
-      static_cast<uint32_t>(input_evalue.tag));
+  if (!e.isTensor() && !e.isScalar()) {
+#if ET_LOG_ENABLED
+    std::array<char, kTagNameBufferSize> tag_name;
+    tag_to_string(e.tag, tag_name.data(), tag_name.size());
+    ET_LOG(
+        Error,
+        "Input %zu was expected to be a Tensor or primitive but was %s.",
+        input_idx,
+        tag_name.data());
+#endif
+
+    return Error::InvalidArgument;
+  }
+
+  if (e.tag != input_evalue.tag) {
+#if ET_LOG_ENABLED
+    std::array<char, kTagNameBufferSize> e_tag_name;
+    std::array<char, kTagNameBufferSize> input_tag_name;
+    tag_to_string(e.tag, e_tag_name.data(), e_tag_name.size());
+    tag_to_string(
+        input_evalue.tag, input_tag_name.data(), input_tag_name.size());
+    ET_LOG(
+        Error,
+        "Input %zu was expected to have type %s but was %s.",
+        input_idx,
+        e_tag_name.data(),
+        input_tag_name.data());
+#endif
+
+    return Error::InvalidArgument;
+  }
 
   if (e.isTensor()) {
     const auto& t_dst = e.toTensor();
@@ -932,7 +950,12 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) {
         e.toString().data(),
         input_evalue.toString().data());
   } else {
-    ET_LOG(Error, "Unsupported input type: %d", (int32_t)e.tag);
+#if ET_LOG_ENABLED
+    std::array<char, kTagNameBufferSize> tag_name;
+    tag_to_string(e.tag, tag_name.data(), tag_name.size());
+    ET_LOG(Error, "Unsupported input type: %s", tag_name.data());
+#endif
+
     return Error::InvalidArgument;
   }
   return Error::Ok;
@@ -984,11 +1007,15 @@ Method::set_output_data_ptr(void* buffer, size_t size, size_t output_idx) {
       outputs_size());
 
   auto& output = mutable_value(get_output_index(output_idx));
-  ET_CHECK_OR_RETURN_ERROR(
-      output.isTensor(),
-      InvalidArgument,
-      "output type: %zu is not tensor",
-      (size_t)output.tag);
+  if (!output.isTensor()) {
+#if ET_LOG_ENABLED
+    std::array<char, kTagNameBufferSize> tag_name;
+    tag_to_string(output.tag, tag_name.data(), tag_name.size());
+    ET_LOG(Error, "Output type: %s is not a tensor.", tag_name.data());
+#endif
+
+    return Error::InvalidArgument;
+  }
 
   auto tensor_meta = this->method_meta().output_tensor_meta(output_idx);
   if (tensor_meta->is_memory_planned()) {
@@ -1001,11 +1028,16 @@ Method::set_output_data_ptr(void* buffer, size_t size, size_t output_idx) {
   }
 
   auto& t = output.toTensor();
-  ET_CHECK_OR_RETURN_ERROR(
-      output.isTensor(),
-      InvalidArgument,
-      "output type: %zu is not tensor",
-      (size_t)output.tag);
+  if (!output.isTensor()) {
+#if ET_LOG_ENABLED
+    std::array<char, kTagNameBufferSize> tag_name;
+    tag_to_string(output.tag, tag_name.data(), tag_name.size());
+    ET_LOG(Error, "output type: %s is not a tensor.", tag_name.data());
+#endif
+
+    return Error::InvalidArgument;
+  }
+
   ET_CHECK_OR_RETURN_ERROR(
       t.nbytes() <= size,
       InvalidArgument,