You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/01/03 05:17:57 UTC
[GitHub] [tvm] cxcxcxcx opened a new pull request #7190: Makes sure g_last_error is null terminated.
cxcxcxcx opened a new pull request #7190:
URL: https://github.com/apache/tvm/pull/7190
This addresses GCC 10 error:
```
"src/runtime/crt/common/crt_runtime_api.c"
include/tvm/runtime/c_runtime_api.h: 在函数‘TVMAPISetLastError’中:
src/runtime/crt/common/crt_runtime_api.c:42:3: 错误：‘strncpy’ specified
bound 1024 equals destination size [-Werror=stringop-truncation]
42 | strncpy(g_last_error, msg, sizeof(g_last_error));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1：所有的警告都被当作是错误
```
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cxcxcxcx commented on pull request #7190: Makes sure g_last_error is null terminated.
Posted by GitBox <gi...@apache.org>.
cxcxcxcx commented on pull request #7190:
URL: https://github.com/apache/tvm/pull/7190#issuecomment-753569542
@liangfu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] liangfu merged pull request #7190: Makes sure g_last_error is null terminated.
Posted by GitBox <gi...@apache.org>.
liangfu merged pull request #7190:
URL: https://github.com/apache/tvm/pull/7190
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cxcxcxcx commented on a change in pull request #7190: Makes sure g_last_error is null terminated.
Posted by GitBox <gi...@apache.org>.
cxcxcxcx commented on a change in pull request #7190:
URL: https://github.com/apache/tvm/pull/7190#discussion_r551159043
##########
File path: src/runtime/crt/common/crt_runtime_api.c
##########
@@ -38,7 +38,10 @@
static char g_last_error[1024];
-void TVMAPISetLastError(const char* msg) { strncpy(g_last_error, msg, sizeof(g_last_error)); }
+void TVMAPISetLastError(const char* msg) {
+ strncpy(g_last_error, msg, sizeof(g_last_error) - 1);
+ g_last_error[sizeof(g_last_error) - 1] = 0;
Review comment:
Does "truncate msg to the size of g_last_error" mean mutating "msg"? That doesn't sound always doable. My understanding:
1. Before the proposed change, when the error message is longer than 1024, g_last_error won't be null terminated. I believe this is what the warning is about.
2. With the change, g_last_error will always be null-terminated.
3. If we use `strcpy`, we need to strlen first. If the length is long, we need to do something special. I don't think it's trivial. It could be faster, but I wonder how much it matters, since errors are probably rare.
4. snprintf may be an alternative. But probably not too different in the context.
Given these, would you reconsider the change as is?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] liangfu commented on a change in pull request #7190: Makes sure g_last_error is null terminated.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #7190:
URL: https://github.com/apache/tvm/pull/7190#discussion_r551154025
##########
File path: src/runtime/crt/common/crt_runtime_api.c
##########
@@ -38,7 +38,10 @@
static char g_last_error[1024];
-void TVMAPISetLastError(const char* msg) { strncpy(g_last_error, msg, sizeof(g_last_error)); }
+void TVMAPISetLastError(const char* msg) {
+ strncpy(g_last_error, msg, sizeof(g_last_error) - 1);
+ g_last_error[sizeof(g_last_error) - 1] = 0;
Review comment:
I think no null-character is implicitly appended at the end of destination only if source is longer than num, otherwise, destination is padded with zeros until a total of num characters have been written to it. See [strncpy - C++ reference](http://www.cplusplus.com/reference/cstring/strncpy/).
The error reported in the compiler meant "bound 1024 equals destination size". As a correction for this, I think it'll be better to copy msg directly to g_last_error:
```c
strcpy(g_last_error, msg);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] liangfu commented on pull request #7190: Makes sure g_last_error is null terminated.
Posted by GitBox <gi...@apache.org>.
liangfu commented on pull request #7190:
URL: https://github.com/apache/tvm/pull/7190#issuecomment-753837813
Thanks @cxcxcxcx for the proposed change. This is now merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] liangfu commented on a change in pull request #7190: Makes sure g_last_error is null terminated.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #7190:
URL: https://github.com/apache/tvm/pull/7190#discussion_r551175687
##########
File path: src/runtime/crt/common/crt_runtime_api.c
##########
@@ -38,7 +38,10 @@
static char g_last_error[1024];
-void TVMAPISetLastError(const char* msg) { strncpy(g_last_error, msg, sizeof(g_last_error)); }
+void TVMAPISetLastError(const char* msg) {
+ strncpy(g_last_error, msg, sizeof(g_last_error) - 1);
+ g_last_error[sizeof(g_last_error) - 1] = 0;
Review comment:
Good point. On a second thought, the proposed change is good enough to put msg to g_last_error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org