Log EValue tag names instead of numeric values (#7538)

GregoryComer · facebook-github-bot · commit 7e463866927d · 2025-01-19T00:04:25.000-08:00
Summary:
Update error log messages that include EValue tags to use a string representation of the tag rather than the numerical index. This improves readability for users.

Example Old Message:
```
[method.cpp:814] The 0-th input of method should have the same type as the input_evalue, but get tag 1 and tag 4
```

Example New Message:
```
[method.cpp:813] Input 0 was expected to be Tensor but was Int.
```


Test Plan:
Locally built the executorch bento kernel and tested with an invalid EValue input to capture the example new message above. Repeated with "-c executorch.enable_enum_strings=false" to verify fallback behavior.

Added tests for `tag_to_string` to tag_test.cpp, runnable via
```
buck test executorch/runtime/core/test:tag_test
```

Reviewed By: digantdesai, manuelcandales

Differential Revision: D67888756

Pulled By: GregoryComer
diff --git a/runtime/core/defines.h b/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
diff --git a/runtime/core/tag.cpp b/runtime/core/tag.cpp
@@ -0,0 +1,51 @@
+/*
+ * 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 <executorch/runtime/core/defines.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
diff --git a/runtime/core/tag.h b/runtime/core/tag.h
@@ -9,6 +9,8 @@
 #pragma once
 
 #include <cstdint>
+#include <executorch/runtime/core/defines.h>
+#include <executorch/runtime/platform/compiler.h>
 
 namespace executorch {
 namespace runtime {
@@ -36,6 +38,24 @@ 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 bby 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.
+#define TAG_NAME_BUFFER_SIZE 19
+
 } // namespace runtime
 } // namespace executorch
 
diff --git a/runtime/core/targets.bzl b/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/...",
         ],
diff --git a/runtime/core/test/tag_test.cpp b/runtime/core/test/tag_test.cpp
@@ -0,0 +1,77 @@
+/*
+ * 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 <array>
+#include <gtest/gtest.h>
+
+using namespace ::testing;
+using executorch::runtime::tag_to_string;
+using executorch::runtime::Tag;
+
+// 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(SpanToString, 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(SpanToString, TagNameBufferSize) {
+  // Validate that TAG_NAME_BUFFER_SIZE is large enough to hold the all tag
+  // strings without truncation.
+  std::array<char, TAG_NAME_BUFFER_SIZE> 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, TAG_NAME_BUFFER_SIZE) << "TAG_NAME_BUFFER_SIZE is too small to hold " #tag; \
+  longest = std::max(longest, tag##_len);
+
+  EXECUTORCH_FORALL_TAGS(TEST_CASE)
+#undef TEST_CASE
+    
+  EXPECT_EQ(longest + 1, TAG_NAME_BUFFER_SIZE) << "TAG_NAME_BUFFER_SIZE has incorrect value, expected " << longest + 1;
+}
+
+TEST(SpanToString, 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(SpanToString, 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
diff --git a/runtime/core/test/targets.bzl b/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"],
diff --git a/runtime/executor/method.cpp b/runtime/executor/method.cpp
@@ -816,26 +816,41 @@ 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));
+  
+  if (!e.isTensor() && !e.isScalar()) {
+  #if ET_LOG_ENABLED
+    char tag_name[TAG_NAME_BUFFER_SIZE];
+    tag_to_string(e.tag, tag_name, sizeof(tag_name));
+  #endif
 
+    ET_CHECK_OR_RETURN_ERROR(
+        e.isTensor() || e.isScalar(),
+        InvalidArgument,
+        "Input %zu was expected to be a Tensor or primitive but was %s.",
+        input_idx,
+        tag_name);
+  }
+
+  if (e.tag != input_evalue.tag) {
+  #if ET_LOG_ENABLED
+    char e_tag_name[TAG_NAME_BUFFER_SIZE];
+    char input_tag_name[TAG_NAME_BUFFER_SIZE];
+    tag_to_string(e.tag, e_tag_name, sizeof(e_tag_name));
+    tag_to_string(input_evalue.tag, input_tag_name, sizeof(input_tag_name));
+  #endif
   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 %zu was expected to have type %s but was %s.",
       input_idx,
-      static_cast<uint32_t>(e.tag),
-      static_cast<uint32_t>(input_evalue.tag));
+      e_tag_name,
+      input_tag_name);
+  }
 
   if (e.isTensor()) {
     const auto& t_dst = e.toTensor();
@@ -925,7 +940,11 @@ 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);
+    char tag_name[16];
+  #if ET_LOG_ENABLED
+    tag_to_string(e.tag, tag_name, sizeof(tag_name));
+  #endif
+    ET_LOG(Error, "Unsupported input type: %s", tag_name);
     return Error::InvalidArgument;
   }
   return Error::Ok;
@@ -977,11 +996,18 @@ 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
+    char tag_name[TAG_NAME_BUFFER_SIZE];
+    tag_to_string(output.tag, tag_name, sizeof(tag_name));
+  #endif
+
+    ET_CHECK_OR_RETURN_ERROR(
+        output.isTensor(),
+        InvalidArgument,
+        "output type: %s is not tensor",
+        tag_name);
+  }
 
   auto tensor_meta = this->method_meta().output_tensor_meta(output_idx);
   if (tensor_meta->is_memory_planned()) {
@@ -994,11 +1020,18 @@ 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
+    char tag_name[TAG_NAME_BUFFER_SIZE];
+    tag_to_string(output.tag, tag_name, sizeof(tag_name));
+  #endif
+
+    ET_CHECK_OR_RETURN_ERROR(
+        output.isTensor(),
+        InvalidArgument,
+        "output type: %s is not tensor",
+        tag_name);
+  }
   ET_CHECK_OR_RETURN_ERROR(
       t.nbytes() <= size,
       InvalidArgument,
