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/03/19 14:06:58 UTC
[GitHub] [tvm] tqchen opened a new pull request #7706: [RUNTIME] Cleanup build for libbacktrace
tqchen opened a new pull request #7706:
URL: https://github.com/apache/tvm/pull/7706
- Introduce TVM_USE_LIBBACKTRACE value macro to be consistent
wth other value macros(instead of relying on disabled flag).
- Introduce AUTO mode for libbacktrace
- Temporary disable MacOS support in light of recent bug report.
- Refactor out the libbacktrace.cmake to libs
- Properly use TVM_DLL so that code is cross platform.
- Fallback to the weaker dmlc impl when backtrace is disabled.x
--
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] tkonolige commented on a change in pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7706:
URL: https://github.com/apache/tvm/pull/7706#discussion_r597914221
##########
File path: src/runtime/logging.cc
##########
@@ -17,16 +17,15 @@
* under the License.
*/
-#ifdef TVM_BACKTRACE_DISABLED
+#if TVM_USE_LIBBACKTRACE == 0
+#include <dmlc/logging.h>
+
#include <string>
-// TODO(bkimball,tkonolige) This inline function is to work around a linking error I am having when
-// using MSVC If the function definition is in logging.cc then the linker can't find it no matter
-// what kind of attributes (dllexport) I decorate it with. This is temporary and will be addressed
-// when we get backtrace working on Windows.
namespace tvm {
namespace runtime {
-__declspec(dllexport) std::string Backtrace() { return ""; }
+// Fallback to the dmlc implementation when backtrace is not available.
+std::string Backtrace() { return dmlc::StackTrace(); }
Review comment:
Can you move it into TVM then, so we have consistent flags.
--
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] tqchen commented on pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7706:
URL: https://github.com/apache/tvm/pull/7706#issuecomment-802859146
cc @tkonolige @rkimball @junrushao1994
--
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] tkonolige commented on a change in pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7706:
URL: https://github.com/apache/tvm/pull/7706#discussion_r597908503
##########
File path: src/runtime/logging.cc
##########
@@ -17,16 +17,15 @@
* under the License.
*/
-#ifdef TVM_BACKTRACE_DISABLED
+#if TVM_USE_LIBBACKTRACE == 0
+#include <dmlc/logging.h>
+
#include <string>
-// TODO(bkimball,tkonolige) This inline function is to work around a linking error I am having when
-// using MSVC If the function definition is in logging.cc then the linker can't find it no matter
-// what kind of attributes (dllexport) I decorate it with. This is temporary and will be addressed
-// when we get backtrace working on Windows.
namespace tvm {
namespace runtime {
-__declspec(dllexport) std::string Backtrace() { return ""; }
+// Fallback to the dmlc implementation when backtrace is not available.
+std::string Backtrace() { return dmlc::StackTrace(); }
Review comment:
I don't think we should fall back to dmlc's stack trace implementation. It uses its own set of `-D` flags to control behavior. Users will be confused by the need to set `-DTVM_USE_LIBBACKTRACE=0` and `-DDMLC_LOG_STACK_TRACE=0`.
##########
File path: src/runtime/logging.cc
##########
@@ -17,16 +17,15 @@
* under the License.
*/
-#ifdef TVM_BACKTRACE_DISABLED
+#if TVM_USE_LIBBACKTRACE == 0
Review comment:
I'd like to be consistent with other TVM #if usages.
```suggestion
#if TVM_USE_LIBBACKTRACE
```
--
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] tqchen commented on a change in pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7706:
URL: https://github.com/apache/tvm/pull/7706#discussion_r597913601
##########
File path: src/runtime/logging.cc
##########
@@ -17,16 +17,15 @@
* under the License.
*/
-#ifdef TVM_BACKTRACE_DISABLED
+#if TVM_USE_LIBBACKTRACE == 0
+#include <dmlc/logging.h>
+
#include <string>
-// TODO(bkimball,tkonolige) This inline function is to work around a linking error I am having when
-// using MSVC If the function definition is in logging.cc then the linker can't find it no matter
-// what kind of attributes (dllexport) I decorate it with. This is temporary and will be addressed
-// when we get backtrace working on Windows.
namespace tvm {
namespace runtime {
-__declspec(dllexport) std::string Backtrace() { return ""; }
+// Fallback to the dmlc implementation when backtrace is not available.
+std::string Backtrace() { return dmlc::StackTrace(); }
Review comment:
the dmlc::StackTrace at least gives us something over nothing, so it is useful for systems when the backtrace is not enabled.
--
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] tqchen commented on pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7706:
URL: https://github.com/apache/tvm/pull/7706#issuecomment-803110315
we have yet to confirm the problem in #7705 , once it is confirmed, feel free to send a new PR to enable MacOS x64
--
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] tqchen commented on a change in pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7706:
URL: https://github.com/apache/tvm/pull/7706#discussion_r597958168
##########
File path: src/runtime/logging.cc
##########
@@ -17,16 +17,15 @@
* under the License.
*/
-#ifdef TVM_BACKTRACE_DISABLED
+#if TVM_USE_LIBBACKTRACE == 0
+#include <dmlc/logging.h>
+
#include <string>
-// TODO(bkimball,tkonolige) This inline function is to work around a linking error I am having when
-// using MSVC If the function definition is in logging.cc then the linker can't find it no matter
-// what kind of attributes (dllexport) I decorate it with. This is temporary and will be addressed
-// when we get backtrace working on Windows.
namespace tvm {
namespace runtime {
-__declspec(dllexport) std::string Backtrace() { return ""; }
+// Fallback to the dmlc implementation when backtrace is not available.
+std::string Backtrace() { return dmlc::StackTrace(); }
Review comment:
Given the fallback is less frequently used and this PR did fixes the previous problem and bring in new changes, I think this change is OK as it is. We can consider bring the new impl back later if needed.
--
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] junrushao1994 merged pull request #7706: [RUNTIME] Cleanup build for libbacktrace
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7706:
URL: https://github.com/apache/tvm/pull/7706
--
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