[GitHub] [tvm] junrushao1994 commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers

[GitBox <gi...@apache.org>]

junrushao1994 commented on a change in pull request #7153:
URL: https://github.com/apache/tvm/pull/7153#discussion_r572264278



##########
File path: apps/android_camera/app/src/main/jni/tvm_runtime.h
##########
@@ -25,17 +25,11 @@
 
 #include <fstream>
 
-/* Enable custom logging - this will cause TVM to pass every log message
- * through CustomLogMessage instead of LogMessage. By enabling this, we must
- * implement dmlc::CustomLogMessage::Log. We use this to pass TVM log
- * messages to Android logcat.
+/* Enable custom logging - this will cause TVM to use a custom implementation
+ * of tvm::runtime::detail::LogMessage. We use this to pass TVM log messages to
+ * Android logcat.
  */
-#define DMLC_LOG_CUSTOMIZE 1
-
-/* Ensure that fatal errors are passed to the logger before throwing
- * in LogMessageFatal
- */
-#define DMLC_LOG_BEFORE_THROW 1
+#define TVM_LOG_CUSTOMIZE 1

Review comment:
       CC @yzhliu for the android part

##########
File path: CMakeLists.txt
##########
@@ -87,6 +92,7 @@ include_directories(${CMAKE_INCLUDE_PATH})
 include_directories("include")
 include_directories(SYSTEM ${DLPACK_PATH})
 include_directories(SYSTEM ${DMLC_PATH})
+add_definitions(-DDMLC_USE_LOGGING_LIBRARY=<tvm/runtime/logging.h>)

Review comment:
       generally should we avoid global flags in cmake?

##########
File path: CMakeLists.txt
##########
@@ -379,6 +387,22 @@ set_property(TARGET tvm APPEND PROPERTY LINK_OPTIONS "${TVM_VISIBILITY_FLAG}")
 add_library(tvm_runtime SHARED $<TARGET_OBJECTS:tvm_runtime_objs>)
 set_property(TARGET tvm_runtime APPEND PROPERTY LINK_OPTIONS "${TVM_VISIBILITY_FLAG}")
 
+if(USE_LIBBACKTRACE)
+  message(STATUS "Building with libbacktrace...")
+  include(cmake/modules/Libbacktrace.cmake)
+  target_link_libraries(tvm PRIVATE libbacktrace)
+  target_link_libraries(tvm_runtime PRIVATE libbacktrace)
+  add_dependencies(tvm_objs libbacktrace)
+  add_dependencies(tvm_runtime_objs libbacktrace)
+  # pre 3.12 versions of cmake cannot propagate include directories from imported targets so we set them manually
+  target_include_directories(tvm PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/include")
+  target_include_directories(tvm_objs PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/include")
+  target_include_directories(tvm_runtime PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/include")
+  target_include_directories(tvm_runtime_objs PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/include")
+else()
+  add_definitions(-DTVM_BACKTRACE_DISABLED)

Review comment:
       ditto

##########
File path: include/tvm/ir/diagnostic.h
##########
@@ -37,6 +37,15 @@ namespace tvm {
 using tvm::parser::SourceMap;
 using tvm::runtime::TypedPackedFunc;
 
+/*! \brief The diagnostic level, controls the printing of the message. */
+enum class DiagnosticLevel : int {
+  kBug = 10,

Review comment:
       +1

##########
File path: CMakeLists.txt
##########
@@ -526,3 +550,25 @@ if(MSVC)
   target_compile_definitions(tvm_objs PRIVATE -DTVM_EXPORTS)
   target_compile_definitions(tvm_runtime_objs PRIVATE -DTVM_EXPORTS)
 endif()
+
+set(TVM_IS_DEBUG_BUILD OFF)
+if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo" OR CMAKE_CXX_FLAGS MATCHES "-g")
+  set(TVM_IS_DEBUG_BUILD ON)
+endif()
+
+# Change relative paths in backtrace to absolute ones
+if(TVM_IS_DEBUG_BUILD)
+  target_compile_options(tvm PRIVATE "-ffile-prefix-map=..=${CMAKE_CURRENT_SOURCE_DIR}")

Review comment:
       Is this GCC specific flag? If so, we probably should check compiler id

##########
File path: cmake/modules/Libbacktrace.cmake
##########
@@ -0,0 +1,54 @@
+# 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.
+include(ExternalProject)
+
+# Git tag for libbacktrace
+set(TAG dedbe13fda00253fe5d4f2fb812c909729ed5937)
+
+ExternalProject_Add(project_libbacktrace
+  PREFIX libbacktrace
+  GIT_REPOSITORY https://github.com/ianlancetaylor/libbacktrace.git
+  GIT_TAG "${TAG}"
+  CONFIGURE_COMMAND "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/src/project_libbacktrace/configure"
+                    "--prefix=${CMAKE_CURRENT_BINARY_DIR}/libbacktrace" --with-pic
+  INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace"
+  BUILD_COMMAND make
+  INSTALL_COMMAND make install
+  BUILD_BYPRODUCTS "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/lib/libbacktrace.a"
+                   "${CMAKE_CURRENT_BINARY_DIR}/libbacktrace/include/backtrace.h"
+  # disable the builtin update because it rebuilds on every build
+  UPDATE_COMMAND ""
+  )
+
+# Only rebuild libbacktrace if this file changes
+# libbacktrace has a bug on macOS with shared libraries, so we patch it here
+ExternalProject_Add_Step(project_libbacktrace checkout

Review comment:
       An alternative to ExternalProject is that we can actually add libbacktrace as a submodule in `3rdparty/`, and we patch on our own fork

##########
File path: tests/scripts/task_build.sh
##########
@@ -16,4 +16,4 @@
 # specific language governing permissions and limitations
 # under the License.
 export VTA_HW_PATH=`pwd`/3rdparty/vta-hw
-cd $1 && cmake .. && make $2 && cd ..
+cd $1 && cmake .. -DCMAKE_BUILD_TYPE=Debug && make $2 && cd ..

Review comment:
       Is this change intentional? Perhaps it makes sense to test with `RelWithDebInfo`?

##########
File path: tutorials/auto_scheduler/tune_network_mali.py
##########
@@ -329,7 +329,7 @@ def tune_and_evaluate():
 #   The last line also prints the total number of measurement trials,
 #   total time spent on auto-tuning and the id of the next task to tune.
 #
-#   There will also be some "dmlc::Error"s errors, because the
+#   There will also be some "tvn::Error"s errors, because the

Review comment:
       oops
   ```suggestion
   #   There will also be some "tvm::Error"s errors, because the
   ```

##########
File path: tutorials/auto_scheduler/tune_network_x86.py
##########
@@ -251,7 +251,7 @@ def run_tuning():
 #   The last line also prints the total number of measurement trials,
 #   total time spent on auto-tuning and the id of the next task to tune.
 #
-#   There will also be some "dmlc::Error"s errors, because the
+#   There will also be some "tvn::Error"s errors, because the

Review comment:
       ditto
   ```suggestion
   #   There will also be some "tvm::Error"s errors, because the
   ```




----------------------------------------------------------------
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