gemini-code-assist[bot] commented on code in PR #659:
URL: https://github.com/apache/tvm-ffi/pull/659#discussion_r3563275817
##########
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!"
+ assert isinstance(result, str)
+
+
[email protected]("v", _all_variants, ids=_variant_id)
+def test_jit_returns_empty_string(v: Variant) -> None:
+ """JIT function returns an empty String object (kTVMFFIStr)."""
+ mod = load(v.string_return_obj())
+ result = mod.get_function(v.fn("test_get_empty_string"))()
+ assert result == ""
+ assert isinstance(result, str)
+
+
[email protected]("v", _all_variants, ids=_variant_id)
+def test_jit_concatenates_strings(v: Variant) -> None:
+ """JIT function takes two strings and returns concatenated result."""
+ mod = load(v.string_return_obj())
+ concat_fn = mod.get_function(v.fn("test_concatenate_strings"))()
+
Review Comment:
The trailing `()` at the end of `mod.get_function(...)` will immediately
execute the retrieved PackedFunc with zero arguments. Since
`test_concatenate_strings` expects exactly 2 arguments, this will raise a
`ValueError` and cause the test to fail. Remove the trailing `()` to correctly
store the function handle.
```suggestion
concat_fn = mod.get_function(v.fn("test_concatenate_strings"))
```
##########
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!"
+ assert isinstance(result, str)
+
+
[email protected]("v", _all_variants, ids=_variant_id)
+def test_jit_returns_empty_string(v: Variant) -> None:
+ """JIT function returns an empty String object (kTVMFFIStr)."""
+ mod = load(v.string_return_obj())
+ result = mod.get_function(v.fn("test_get_empty_string"))()
+ assert result == ""
+ assert isinstance(result, str)
+
+
[email protected]("v", _all_variants, ids=_variant_id)
+def test_jit_concatenates_strings(v: Variant) -> None:
+ """JIT function takes two strings and returns concatenated result."""
+ mod = load(v.string_return_obj())
+ concat_fn = mod.get_function(v.fn("test_concatenate_strings"))()
+
+ # Test basic concatenation
+ result = concat_fn("Hello", " World")
+ assert result == "Hello World"
+
+ # Test empty strings
+ result = concat_fn("", "test")
+ assert result == "test"
+
+ result = concat_fn("test", "")
+ assert result == "test"
+
+
[email protected]("v", _all_variants, ids=_variant_id)
+def test_jit_string_length(v: Variant) -> None:
+ """JIT function returns the length of a string."""
+ mod = load(v.string_return_obj())
+ length_fn = mod.get_function(v.fn("test_string_length"))()
Review Comment:
The trailing `()` at the end of `mod.get_function(...)` will immediately
execute the retrieved PackedFunc with zero arguments. Since
`test_string_length` expects exactly 1 argument, this will raise a `ValueError`
and cause the test to fail. Remove the trailing `()` to correctly store the
function handle.
```suggestion
length_fn = mod.get_function(v.fn("test_string_length"))
```
##########
addons/tvm_ffi_orcjit/tests/sources/cc/test_string_return.cc:
##########
@@ -0,0 +1,161 @@
+// 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 {
+ if (args[0].v_ptr == nullptr) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string
data");
+ return -1;
+ }
+ 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 {
+ if (args[1].v_ptr == nullptr) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract
string data");
+ return -1;
+ }
+ TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[1].v_ptr);
+ data1 = bytes->data;
Review Comment:
The indentation inside this `else` block is inconsistent with the rest of
the file (using 8 spaces instead of 6 spaces).
```c
if (args[1].v_ptr == nullptr) {
TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract
string data");
return -1;
}
TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(args[1].v_ptr);
data1 = bytes->data;
size1 = bytes->size;
```
##########
addons/tvm_ffi_orcjit/tests/sources/cc/test_string_return.cc:
##########
@@ -0,0 +1,161 @@
+// 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 {
+ if (args[0].v_ptr == nullptr) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string
data");
+ return -1;
+ }
+ 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 {
+ if (args[1].v_ptr == nullptr) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract
string data");
+ return -1;
+ }
+ 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;
Review Comment:
Constructing temporary `std::string` objects and concatenating them can be
inefficient. Additionally, passing a potentially null pointer (if `data0` or
`data1` is null when size is 0) to the `std::string` constructor is undefined
behavior. Using `std::string::append` conditionally avoids both temporary
allocations and potential undefined behavior.
```c
std::string concatenated;
if (size0 > 0) {
concatenated.append(data0, size0);
}
if (size1 > 0) {
concatenated.append(data1, size1);
}
```
##########
addons/tvm_ffi_orcjit/tests/sources/c/test_string_return.c:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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 __tvm_ffi_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 __tvm_ffi_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 __tvm_ffi_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;
+ }
+
+ /* Manually extract data and size from 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 {
+ if (args[0].v_ptr == NULL) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string
data");
+ return -1;
+ }
+ 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 {
+ if (args[1].v_ptr == NULL) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Failed to extract string
data");
+ return -1;
+ }
+ 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);
Review Comment:
Calling `memcpy` with a potentially NULL pointer (even when the size is 0)
is undefined behavior in C. Wrapping the `memcpy` calls in conditional checks
ensures safety.
```c
if (size0 > 0) {
memcpy(buffer, data0, size0);
}
if (size1 > 0) {
memcpy(buffer + size0, data1, size1);
}
```
--
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]
