gemini-code-assist[bot] commented on code in PR #659: URL: https://github.com/apache/tvm-ffi/pull/659#discussion_r3563251336
########## addons/tvm_ffi_orcjit/tests/sources/c/test_string_return.c: ########## @@ -0,0 +1,146 @@ +/* + * 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) { Review Comment:  The function prefix used here is `__tvmffi_` (e.g., `__tvmffi_test_get_hello_world`), but the standard prefix used by the JIT loader and other test files is `__tvm_ffi_` (with an underscore between `tvm` and `ffi`). Using `__tvmffi_` will likely cause symbol resolution failures during JIT execution of the C variant tests. Please update all function definitions in this file to use the `__tvm_ffi_` prefix. ```c TVM_FFI_DLL_EXPORT int __tvm_ffi_test_get_hello_world(void* self, const TVMFFIAny* args, int32_t num_args, TVMFFIAny* result) { ``` ########## addons/tvm_ffi_orcjit/tests/sources/cc/test_string_return.cc: ########## @@ -0,0 +1,153 @@ +// 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; + } + + // Manually extract data and size for both SmallStr and heap Str + 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); + 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; + } + + // Manually extract size from SmallStr or heap Str + size_t size; + if (args[0].type_index == kTVMFFISmallStr) { + size = args[0].small_str_len; + } else { + TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); + if (bytes == nullptr) { + TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); + return -1; + } + size = bytes->size; Review Comment:  The null check `if (bytes == nullptr)` is a logic error and will never evaluate to `true`. `TVMFFIBytesGetByteArrayPtr` adds `sizeof(TVMFFIObject)` to the input pointer, so even if `args[0].v_ptr` is `nullptr`, the returned pointer will be non-null (e.g., `0x10` or `0x18`). This check fails to protect against a null pointer dereference when `bytes->size` is accessed. Please check if `args[0].v_ptr` is `nullptr` directly before calling `TVMFFIBytesGetByteArrayPtr`. ```c if (args[0].v_ptr == nullptr) { TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string data"); return -1; } TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[0].v_ptr); size = bytes->size; ``` -- 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]
