From ce03799a622140a3b67ac1bf638f1161c9adb8aa Mon Sep 17 00:00:00 2001 From: Dave Bort Date: Sat, 5 Aug 2023 09:31:19 -0700 Subject: [PATCH 1/2] [executorch] Remove flatbuffer forward declarations from method.h By hiding this, we are compatible with flatbuffer versions from v1.12 up to the latest release (v23.5.26). --- runtime/executor/method.cpp | 40 ++++++++++++++++++------------------- runtime/executor/method.h | 16 ++++----------- 2 files changed, 24 insertions(+), 32 deletions(-) diff --git a/runtime/executor/method.cpp b/runtime/executor/method.cpp index fc6a7b6875c..0e0d183dad9 100644 --- a/runtime/executor/method.cpp +++ b/runtime/executor/method.cpp @@ -257,11 +257,19 @@ bool parse_cond_value(const EValue& cond_value) { } // namespace -Error Method::parse_values( - const flatbuffers::Vector< - flatbuffers::Offset>* flatbuffer_values, - size_t* num_parsed) { - for (size_t i = 0; i < n_value_; ++i) { +Error Method::parse_values() { + auto flatbuffer_values = serialization_plan_->values(); + ET_CHECK(flatbuffer_values != nullptr); + size_t n_value = flatbuffer_values->size(); + values_ = ET_ALLOCATE_LIST_OR_RETURN_ERROR( + memory_manager_->get_runtime_allocator(), EValue, n_value); + + // n_value_ counts the number of successfully-initialized values for ~Method() + // to clean up, and is incremented at the bottom of the loop. This makes it + // safe for errors to return without updating any state. + n_value_ = 0; + + for (size_t i = 0; i < n_value; ++i) { auto serialization_value = flatbuffer_values->Get(i); switch (serialization_value->val_type()) { case executorch_flatbuffer::KernelTypes::Null: { @@ -329,7 +337,6 @@ Error Method::parse_values( "Failed parsing tensor at index %zu: %" PRIu32, i, t.error()); - *num_parsed = i; return t.error(); } new (&values_[i]) EValue(t.get()); @@ -347,7 +354,6 @@ Error Method::parse_values( "Failed parsing tensor list at index %zu: %" PRIu32, i, tensors.error()); - *num_parsed = i; return tensors.error(); } new (&values_[i]) EValue(tensors.get()); @@ -365,7 +371,6 @@ Error Method::parse_values( "Failed parsing optional tensor list at index %zu: %" PRIu32, i, tensors.error()); - *num_parsed = i; return tensors.error(); } new (&values_[i]) EValue(tensors.get()); @@ -383,8 +388,12 @@ Error Method::parse_values( "to see which type this is.", static_cast(serialization_value->val_type()) - 1); } + + // ~Method() will try to clean up n_value_ entries in the values_ array. + // Only increment this once we know the entry is valid, so that we don't try + // to clean up an uninitialized entry. + n_value_ = i + 1; } - *num_parsed = n_value_; return Error::Ok; } @@ -502,18 +511,9 @@ Error Method::init(executorch_flatbuffer::ExecutionPlan* s_plan) { auto runtime_allocator = memory_manager_->get_runtime_allocator(); { - // Load values - auto plan_values = serialization_plan_->values(); - ET_CHECK(plan_values != nullptr); - n_value_ = plan_values->size(); - values_ = - ET_ALLOCATE_LIST_OR_RETURN_ERROR(runtime_allocator, EValue, n_value_); - size_t num_parsed; - Error err = parse_values(plan_values, &num_parsed); + // Parse the elements of the values_ array. + Error err = parse_values(); if (err != Error::Ok) { - // ~Method() will try to clean up entries in this list. Ensure that it - // doesn't look at any uninitialized entries. - n_value_ = num_parsed; return err; } } diff --git a/runtime/executor/method.h b/runtime/executor/method.h index 7918a3badd6..1ff0297839a 100644 --- a/runtime/executor/method.h +++ b/runtime/executor/method.h @@ -15,12 +15,6 @@ // Forward declare flatbuffer types. This is a public header and must not // include the generated flatbuffer header. -namespace flatbuffers { -template -class Vector; -template -struct Offset; -} // namespace flatbuffers namespace executorch_flatbuffer { struct Chain; struct ExecutionPlan; @@ -262,13 +256,11 @@ class Method final { bool pre_allocated_input_; /** - * Initialize the EValue table of the program. *num_parsed is the number - * of elements actually parsed, which may be less than n_value_ on failure. + * Parses the elements of the values_ array. On error, n_value_ will be set to + * the number of successfully-initialized entries so that ~Method doesn't try + * to clean up uninitialized entries. */ - __ET_NODISCARD Error parse_values( - const flatbuffers::Vector< - flatbuffers::Offset>* fb_values, - size_t* num_parsed); + __ET_NODISCARD Error parse_values(); __ET_NODISCARD Error resolve_operator( int32_t op_index, From ecab42731412b4cd3623d076a067fe801c30d9d4 Mon Sep 17 00:00:00 2001 From: Dave Bort Date: Fri, 4 Aug 2023 20:58:32 -0700 Subject: [PATCH 2/2] [flatbuffers] Update to flatbuffers v23.5.26 Only required some minor changes. We could support v1.12 with a couple of pretty simple #if blocks. Should fix issue #37 --- third-party/TARGETS | 18 ++++++++++++++---- third-party/flatbuffers | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/third-party/TARGETS b/third-party/TARGETS index f9e9f3990ba..ff6f1144b6b 100644 --- a/third-party/TARGETS +++ b/third-party/TARGETS @@ -165,23 +165,35 @@ runtime.cxx_binary( "flatbuffers/grpc/src/compiler/python_generator.cc", "flatbuffers/grpc/src/compiler/swift_generator.cc", "flatbuffers/grpc/src/compiler/ts_generator.cc", + "flatbuffers/src/annotated_binary_text_gen.cpp", + "flatbuffers/src/bfbs_gen_lua.cpp", + "flatbuffers/src/bfbs_gen_nim.cpp", + "flatbuffers/src/binary_annotator.cpp", + "flatbuffers/src/code_generators.cpp", + "flatbuffers/src/file_binary_writer.cpp", + "flatbuffers/src/file_name_saving_file_manager.cpp", + "flatbuffers/src/file_writer.cpp", + "flatbuffers/src/flatc.cpp", "flatbuffers/src/flatc_main.cpp", + "flatbuffers/src/idl_gen_binary.cpp", "flatbuffers/src/idl_gen_cpp.cpp", "flatbuffers/src/idl_gen_csharp.cpp", "flatbuffers/src/idl_gen_dart.cpp", + "flatbuffers/src/idl_gen_fbs.cpp", "flatbuffers/src/idl_gen_go.cpp", "flatbuffers/src/idl_gen_grpc.cpp", "flatbuffers/src/idl_gen_java.cpp", - "flatbuffers/src/idl_gen_js_ts.cpp", "flatbuffers/src/idl_gen_json_schema.cpp", "flatbuffers/src/idl_gen_kotlin.cpp", "flatbuffers/src/idl_gen_lobster.cpp", - "flatbuffers/src/idl_gen_lua.cpp", "flatbuffers/src/idl_gen_php.cpp", "flatbuffers/src/idl_gen_python.cpp", "flatbuffers/src/idl_gen_rust.cpp", "flatbuffers/src/idl_gen_swift.cpp", "flatbuffers/src/idl_gen_text.cpp", + "flatbuffers/src/idl_gen_ts.cpp", + "flatbuffers/src/idl_parser.cpp", + "flatbuffers/src/reflection.cpp", "flatbuffers/src/util.cpp", ], include_directories = [ @@ -189,12 +201,10 @@ runtime.cxx_binary( "flatbuffers/include", ], raw_headers = [ - "flatbuffers/grpc/src/compiler/config.h", "flatbuffers/grpc/src/compiler/cpp_generator.h", "flatbuffers/grpc/src/compiler/go_generator.h", "flatbuffers/grpc/src/compiler/java_generator.h", "flatbuffers/grpc/src/compiler/python_generator.h", - "flatbuffers/grpc/src/compiler/python_private_generator.h", "flatbuffers/grpc/src/compiler/schema_interface.h", "flatbuffers/grpc/src/compiler/swift_generator.h", "flatbuffers/grpc/src/compiler/ts_generator.h", diff --git a/third-party/flatbuffers b/third-party/flatbuffers index 338393f854e..0100f6a5779 160000 --- a/third-party/flatbuffers +++ b/third-party/flatbuffers @@ -1 +1 @@ -Subproject commit 338393f854eb5ba24761a22cd9316ff5cee4eab0 +Subproject commit 0100f6a5779831fa7a651e4b67ef389a8752bd9b