[GitHub] [tvm] tkonolige commented on a change in pull request #7706: [RUNTIME] Cleanup build for libbacktrace

[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