gemini-code-assist[bot] commented on code in PR #659: URL: https://github.com/apache/tvm-ffi/pull/659#discussion_r3560297382
########## addons/tvm_ffi_orcjit/tests/sources/c/test_string_return.c: ########## @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * String return type tests: JIT functions that return String objects + * Tests the conversion of kTVMFFIStr type_index from JIT to Python. + */ + +#include <string.h> +#include <tvm/ffi/c_api.h> + +/* test_get_hello_world: returns a simple ASCII string */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_get_hello_world(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + /* Create a string object with "Hello, World!" */ + const char* message = "Hello, World!"; + TVMFFIByteArray input = {.data = message, .size = strlen(message)}; + + /* Convert byte array to String (automatically selects SmallStr or Str based on size) */ + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create string"); + return -1; + } + + return 0; +} + +/* test_get_empty_string: returns an empty String */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_get_empty_string(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + TVMFFIByteArray input = {.data = "", .size = 0}; + + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create empty string"); + return -1; + } + + return 0; +} + +/* test_concatenate_strings: takes two string args and returns concatenated result */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_concatenate_strings(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + if (num_args != 2) { + TVMFFIErrorSetRaisedFromCStr("ValueError", "Expected 2 arguments"); + return -1; + } + + /* Accept both SmallStr and Str for input arguments */ + int is_str0 = args[0].type_index == kTVMFFIStr || args[0].type_index == kTVMFFISmallStr; + int is_str1 = args[1].type_index == kTVMFFIStr || args[1].type_index == kTVMFFISmallStr; + if (!is_str0 || !is_str1) { + TVMFFIErrorSetRaisedFromCStr("TypeError", "Both arguments must be strings"); + return -1; + } + + /* Get byte arrays from string objects (handling both SmallStr and heap Str) */ + TVMFFIByteArray* bytes0 = (args[0].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[0]) + : TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + TVMFFIByteArray* bytes1 = (args[1].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[1]) + : TVMFFIBytesGetByteArrayPtr(args[1].v_ptr); + + if (bytes0 == NULL || bytes1 == NULL) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + + /* Concatenate the two strings */ + size_t total_len = bytes0->size + bytes1->size; + char buffer[512]; /* Fixed-size buffer for small test strings */ + + if (total_len >= sizeof(buffer)) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Concatenated string too large"); + return -1; + } + + memcpy(buffer, bytes0->data, bytes0->size); + memcpy(buffer + bytes0->size, bytes1->data, bytes1->size); + + /* Create result string (TVMFFIStringFromByteArray will select SmallStr or Str) */ + TVMFFIByteArray output = {.data = buffer, .size = total_len}; + if (TVMFFIStringFromByteArray(&output, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create concatenated string"); + return -1; + } + + return 0; +} + +/* test_string_length: returns length of input string */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_string_length(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + if (num_args != 1) { + TVMFFIErrorSetRaisedFromCStr("ValueError", "Expected 1 argument"); + return -1; + } + + /* Accept both SmallStr and Str */ + int is_str = args[0].type_index == kTVMFFIStr || args[0].type_index == kTVMFFISmallStr; + if (!is_str) { + TVMFFIErrorSetRaisedFromCStr("TypeError", "Argument must be a string"); + return -1; + } + + /* Get byte array (handling both SmallStr and heap Str) */ + TVMFFIByteArray* bytes = (args[0].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[0]) + : TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + if (bytes == NULL) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + + result->type_index = kTVMFFIInt; + result->zero_padding = 0; + result->v_int64 = (int64_t)bytes->size; Review Comment:  Similar to the concatenation function, `TVMFFISmallBytesGetContentByteArray` and `TVMFFIBytesGetByteArrayPtr` are unavailable in pure C, and their ternary usage causes a type mismatch. Manually extract the data and size fields instead. ```c const char* data; size_t size; if (args[0].type_index == kTVMFFISmallStr) { data = args[0].v_bytes; size = args[0].small_str_len; } else { TVMFFIByteArray* bytes = (TVMFFIByteArray*)((char*)args[0].v_ptr + sizeof(TVMFFIObject)); data = bytes->data; size = bytes->size; } result->type_index = kTVMFFIInt; result->zero_padding = 0; result->v_int64 = (int64_t)size; ``` ########## addons/tvm_ffi_orcjit/tests/sources/c/test_string_return.c: ########## @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * String return type tests: JIT functions that return String objects + * Tests the conversion of kTVMFFIStr type_index from JIT to Python. + */ + +#include <string.h> +#include <tvm/ffi/c_api.h> + +/* test_get_hello_world: returns a simple ASCII string */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_get_hello_world(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + /* Create a string object with "Hello, World!" */ + const char* message = "Hello, World!"; + TVMFFIByteArray input = {.data = message, .size = strlen(message)}; + + /* Convert byte array to String (automatically selects SmallStr or Str based on size) */ + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create string"); + return -1; + } + + return 0; +} + +/* test_get_empty_string: returns an empty String */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_get_empty_string(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + TVMFFIByteArray input = {.data = "", .size = 0}; + + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create empty string"); + return -1; + } + + return 0; +} + +/* test_concatenate_strings: takes two string args and returns concatenated result */ +TVM_FFI_DLL_EXPORT int __tvmffi_test_concatenate_strings(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + if (num_args != 2) { + TVMFFIErrorSetRaisedFromCStr("ValueError", "Expected 2 arguments"); + return -1; + } + + /* Accept both SmallStr and Str for input arguments */ + int is_str0 = args[0].type_index == kTVMFFIStr || args[0].type_index == kTVMFFISmallStr; + int is_str1 = args[1].type_index == kTVMFFIStr || args[1].type_index == kTVMFFISmallStr; + if (!is_str0 || !is_str1) { + TVMFFIErrorSetRaisedFromCStr("TypeError", "Both arguments must be strings"); + return -1; + } + + /* Get byte arrays from string objects (handling both SmallStr and heap Str) */ + TVMFFIByteArray* bytes0 = (args[0].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[0]) + : TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + TVMFFIByteArray* bytes1 = (args[1].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[1]) + : TVMFFIBytesGetByteArrayPtr(args[1].v_ptr); + + if (bytes0 == NULL || bytes1 == NULL) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + + /* Concatenate the two strings */ + size_t total_len = bytes0->size + bytes1->size; + char buffer[512]; /* Fixed-size buffer for small test strings */ + + if (total_len >= sizeof(buffer)) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Concatenated string too large"); + return -1; + } + + memcpy(buffer, bytes0->data, bytes0->size); + memcpy(buffer + bytes0->size, bytes1->data, bytes1->size); Review Comment:  The helper functions `TVMFFISmallBytesGetContentByteArray` and `TVMFFIBytesGetByteArrayPtr` are only defined in `c_api.h` under a `__cplusplus` guard, making them unavailable in pure C. Additionally, `TVMFFISmallBytesGetContentByteArray` returns a struct by value, while `TVMFFIBytesGetByteArrayPtr` returns a pointer, resulting in a type mismatch in the ternary operator. Instead, manually extract the data and size fields from `args` for both `kTVMFFISmallStr` and `kTVMFFIStr` types. ```c const char* data0; size_t size0; if (args[0].type_index == kTVMFFISmallStr) { data0 = args[0].v_bytes; size0 = args[0].small_str_len; } else { TVMFFIByteArray* bytes = (TVMFFIByteArray*)((char*)args[0].v_ptr + sizeof(TVMFFIObject)); data0 = bytes->data; size0 = bytes->size; } const char* data1; size_t size1; if (args[1].type_index == kTVMFFISmallStr) { data1 = args[1].v_bytes; size1 = args[1].small_str_len; } else { TVMFFIByteArray* bytes = (TVMFFIByteArray*)((char*)args[1].v_ptr + sizeof(TVMFFIObject)); data1 = bytes->data; size1 = bytes->size; } /* Concatenate the two strings */ size_t total_len = size0 + size1; char buffer[512]; /* Fixed-size buffer for small test strings */ if (total_len >= sizeof(buffer)) { TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Concatenated string too large"); return -1; } memcpy(buffer, data0, size0); memcpy(buffer + size0, data1, size1); ``` ########## addons/tvm_ffi_orcjit/tests/sources/cc/test_string_return.cc: ########## @@ -0,0 +1,139 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// String return type tests (C++): JIT functions that return String objects. +// Tests the conversion of kTVMFFIStr type_index from JIT to Python. + +#include <tvm/ffi/c_api.h> + +#include <cstring> +#include <string> + +// test_get_hello_world: returns a simple ASCII string +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_get_hello_world(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + std::string message = "Hello, World!"; + TVMFFIByteArray input; + input.data = message.c_str(); + input.size = message.size(); + + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create string"); + return -1; + } + + return 0; +} + +// test_get_empty_string: returns an empty string +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_get_empty_string(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + TVMFFIByteArray input; + input.data = ""; + input.size = 0; + + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create empty string"); + return -1; + } + + return 0; +} + +// test_concatenate_strings: takes two string args and returns concatenated result +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_concatenate_strings(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + try { + if (num_args != 2) { + TVMFFIErrorSetRaisedFromCStr("ValueError", "Expected 2 arguments"); + return -1; + } + + // Accept both SmallStr and Str for input arguments + bool is_str0 = args[0].type_index == kTVMFFIStr || args[0].type_index == kTVMFFISmallStr; + bool is_str1 = args[1].type_index == kTVMFFIStr || args[1].type_index == kTVMFFISmallStr; + if (!is_str0 || !is_str1) { + TVMFFIErrorSetRaisedFromCStr("TypeError", "Arguments must be strings"); + return -1; + } + + // Get byte arrays (handling both SmallStr and heap Str) + TVMFFIByteArray* bytes0 = (args[0].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[0]) + : TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + TVMFFIByteArray* bytes1 = (args[1].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[1]) + : TVMFFIBytesGetByteArrayPtr(args[1].v_ptr); + + if (bytes0 == nullptr || bytes1 == nullptr) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + + std::string str0(bytes0->data, bytes0->size); + std::string str1(bytes1->data, bytes1->size); Review Comment:  In C++, `TVMFFISmallBytesGetContentByteArray` returns a `TVMFFIByteArray` by value, whereas `TVMFFIBytesGetByteArrayPtr` returns a `TVMFFIByteArray*`. Using them in a ternary operator causes a compilation error due to incompatible types. Manually extract the string data and size safely. ```c const char* data0; size_t size0; if (args[0].type_index == kTVMFFISmallStr) { data0 = args[0].v_bytes; size0 = args[0].small_str_len; } else { TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); data0 = bytes->data; size0 = bytes->size; } const char* data1; size_t size1; if (args[1].type_index == kTVMFFISmallStr) { data1 = args[1].v_bytes; size1 = args[1].small_str_len; } else { TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[1].v_ptr); data1 = bytes->data; size1 = bytes->size; } std::string str0(data0, size0); std::string str1(data1, size1); ``` ########## addons/tvm_ffi_orcjit/tests/sources/cc/test_string_return.cc: ########## @@ -0,0 +1,139 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// String return type tests (C++): JIT functions that return String objects. +// Tests the conversion of kTVMFFIStr type_index from JIT to Python. + +#include <tvm/ffi/c_api.h> + +#include <cstring> +#include <string> + +// test_get_hello_world: returns a simple ASCII string +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_get_hello_world(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + std::string message = "Hello, World!"; + TVMFFIByteArray input; + input.data = message.c_str(); + input.size = message.size(); + + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create string"); + return -1; + } + + return 0; +} + +// test_get_empty_string: returns an empty string +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_get_empty_string(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + TVMFFIByteArray input; + input.data = ""; + input.size = 0; + + if (TVMFFIStringFromByteArray(&input, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create empty string"); + return -1; + } + + return 0; +} + +// test_concatenate_strings: takes two string args and returns concatenated result +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_concatenate_strings(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + try { + if (num_args != 2) { + TVMFFIErrorSetRaisedFromCStr("ValueError", "Expected 2 arguments"); + return -1; + } + + // Accept both SmallStr and Str for input arguments + bool is_str0 = args[0].type_index == kTVMFFIStr || args[0].type_index == kTVMFFISmallStr; + bool is_str1 = args[1].type_index == kTVMFFIStr || args[1].type_index == kTVMFFISmallStr; + if (!is_str0 || !is_str1) { + TVMFFIErrorSetRaisedFromCStr("TypeError", "Arguments must be strings"); + return -1; + } + + // Get byte arrays (handling both SmallStr and heap Str) + TVMFFIByteArray* bytes0 = (args[0].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[0]) + : TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + TVMFFIByteArray* bytes1 = (args[1].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[1]) + : TVMFFIBytesGetByteArrayPtr(args[1].v_ptr); + + if (bytes0 == nullptr || bytes1 == nullptr) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + + std::string str0(bytes0->data, bytes0->size); + std::string str1(bytes1->data, bytes1->size); + std::string concatenated = str0 + str1; + + TVMFFIByteArray output; + output.data = concatenated.c_str(); + output.size = concatenated.size(); + if (TVMFFIStringFromByteArray(&output, result) != 0) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to create concatenated string"); + return -1; + } + + return 0; + } catch (const std::exception& e) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", e.what()); + return -1; + } +} + +// test_string_length: returns length of input string +TVM_FFI_DLL_EXPORT int __tvm_ffi_test_string_length(void* self, const TVMFFIAny* args, + int32_t num_args, TVMFFIAny* result) { + try { + if (num_args != 1) { + TVMFFIErrorSetRaisedFromCStr("ValueError", "Expected 1 argument"); + return -1; + } + + // Accept both SmallStr and Str for input argument + bool is_str = args[0].type_index == kTVMFFIStr || args[0].type_index == kTVMFFISmallStr; + if (!is_str) { + TVMFFIErrorSetRaisedFromCStr("TypeError", "Argument must be a string"); + return -1; + } + + // Get byte array (handling both SmallStr and heap Str) + TVMFFIByteArray* bytes = (args[0].type_index == kTVMFFISmallStr) + ? TVMFFISmallBytesGetContentByteArray(&args[0]) + : TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + if (bytes == nullptr) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + + result->type_index = kTVMFFIInt; + result->zero_padding = 0; + result->v_int64 = static_cast<int64_t>(bytes->size); Review Comment:  The ternary operator has incompatible types (`TVMFFIByteArray` vs `TVMFFIByteArray*`), which will fail to compile. Extract the string data and size manually. ```c const char* data; size_t size; if (args[0].type_index == kTVMFFISmallStr) { data = args[0].v_bytes; size = args[0].small_str_len; } else { TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); data = bytes->data; size = bytes->size; } result->type_index = kTVMFFIInt; result->zero_padding = 0; result->v_int64 = static_cast<int64_t>(size); ``` ########## addons/tvm_ffi_orcjit/tests/test_basic.py: ########## @@ -381,6 +385,58 @@ def test_void_function(v: Variant) -> None: assert result is None +# --------------------------------------------------------------------------- +# String return type tests (Group 2b) +# --------------------------------------------------------------------------- + + [email protected]("v", _all_variants, ids=_variant_id) +def test_jit_returns_string(v: Variant) -> None: + """JIT function returns a String object (kTVMFFIStr).""" + mod = load(v.string_return_obj()) + result = mod.get_function(v.fn("test_get_hello_world"))() + assert result == "Hello World" Review Comment:  The JIT function `test_get_hello_world` returns `"Hello, World!"` (with a comma and exclamation mark), but the test asserts `"Hello World"`. This will cause the test to fail. ```suggestion assert result == "Hello, World!" ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
