[GitHub] [tvm] tqchen commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers
tqchen commented on a change in pull request #7153: URL: https://github.com/apache/tvm/pull/7153#discussion_r595375493 ## File path: apps/android_deploy/app/src/main/jni/Application.mk ## @@ -27,7 +27,7 @@ include $(config) APP_STL := c++_static -APP_CPPFLAGS += -DDMLC_LOG_STACK_TRACE=0 -DTVM4J_ANDROID=1 -std=c++14 -Oz -frtti +APP_CPPFLAGS += -DDMLC_USE_LOGGING_LIBRARY=\ -DTVM_BACKTRACE_DISABLED=1 -DTVM4J_ANDROID=1 -std=c++14 -Oz -frtti Review comment: so user build the all in one library does not have to worry too much about these 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: us...@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers
tqchen commented on a change in pull request #7153: URL: https://github.com/apache/tvm/pull/7153#discussion_r595365614 ## File path: apps/android_deploy/app/src/main/jni/Application.mk ## @@ -27,7 +27,7 @@ include $(config) APP_STL := c++_static -APP_CPPFLAGS += -DDMLC_LOG_STACK_TRACE=0 -DTVM4J_ANDROID=1 -std=c++14 -Oz -frtti +APP_CPPFLAGS += -DDMLC_USE_LOGGING_LIBRARY=\ -DTVM_BACKTRACE_DISABLED=1 -DTVM4J_ANDROID=1 -std=c++14 -Oz -frtti Review comment: consider move the def of -DDMLC_USE_LOGGING_LIBRARY=\ -DTVM_BACKTRACE_DISABLED=1 into the all in one file in tvm_runtime.h (very beginning) 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: us...@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers
tqchen commented on a change in pull request #7153:
URL: https://github.com/apache/tvm/pull/7153#discussion_r563164652
##
File path: include/tvm/runtime/logging.h
##
@@ -0,0 +1,380 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file tvm/runtime/logging.h
+ * \brief logging utilities
+ *
+ * We define our own CHECK and LOG macros to replace those from dmlc-core.
+ * These macros are then injected into dmlc-core via the
+ * DMLC_USE_LOGGING_LIBRARY define. dmlc-core will #include this file wherever
+ * it needs logging.
+ */
+#ifndef TVM_RUNTIME_LOGGING_H_
+#define TVM_RUNTIME_LOGGING_H_
+
+#include
+#include
+#include
+#include
+#include
+
+#include "tvm/runtime/c_runtime_api.h"
+
+// a technique that enables overriding macro names on the number of
parameters. This is used
+// to define other macros below
+#define GET_MACRO(_1, _2, _3, _4, _5, NAME, ...) NAME
+
+/*!
+ * \brief COND_X calls COND_X_N where N is the number of parameters passed to
COND_X
+ * X can be any of CHECK_GE, CHECK_EQ, CHECK, or LOG COND_X (but not COND_X_N)
+ * are supposed to be used outside this file.
+ * The first parameter of COND_X (and therefore, COND_X_N), which we call
'quit_on_assert',
+ * is a boolean. The rest of the parameters of COND_X is the same as the
parameters of X.
+ * quit_on_assert determines the overall behavior of COND_X. If it's true
COND_X
+ * quits the program on assertion failure. If it's false, then it moves on and
somehow reports
+ * the assertion failure back to the macro caller in an appropriate manner
(e.g, 'return false'
+ * in a function, or 'continue' or 'break' in a loop)
+ * The default behavior when quit_on_assertion is false, is to 'return false'.
If this is not
+ * desirable, the macro caller can pass one more last parameter to COND_X to
tell COND_X what
+ * to do when when quit_on_assertion is false and the assertion fails.
+ *
+ * Rationale: These macros were designed to implement functions that have two
behaviors
+ * in a concise way. Those behaviors are quitting on assertion failures, or
trying to
+ * move on from assertion failures. Note that these macros hide lots of
control flow in them,
+ * and therefore, makes the logic of the whole code slightly harder to
understand. However,
+ * in pieces of code that use these macros frequently, it will significantly
shorten the
+ * amount of code needed to be read, and we won't need to clutter the main
logic of the
+ * function by repetitive control flow structure. The first problem
+ * mentioned will be improved over time as the developer gets used to the
macro.
+ *
+ * Here is an example of how to use it
+ * \code
+ * bool f(..., bool quit_on_assertion) {
+ * int a = 0, b = 0;
+ * ...
+ * a = ...
+ * b = ...
+ * // if quit_on_assertion is true, if a==b, continue, otherwise quit.
+ * // if quit_on_assertion is false, if a==b, continue, otherwise 'return
false' (default
+ * behaviour) COND_CHECK_EQ(quit_on_assertion, a, b) << "some error message
when quiting"
+ * ...
+ * for (int i = 0; i < N; i++) {
+ * a = ...
+ * b = ...
+ * // if quit_on_assertion is true, if a==b, continue, otherwise quit.
+ * // if quit_on_assertion is false, if a==b, continue, otherwise 'break'
(non-default
+ * // behaviour, therefore, has to be explicitly specified)
+ * COND_CHECK_EQ(quit_on_assertion, a, b, break) << "some error message
when quiting"
+ * }
+ * }
+ * \endcode
+ */
+#define COND_CHECK_GE(...) \
+ GET_MACRO(__VA_ARGS__, COND_CHECK_GE_5, COND_CHECK_GE_4,
COND_CHECK_GE_3)(__VA_ARGS__)
+#define COND_CHECK_EQ(...) \
+ GET_MACRO(__VA_ARGS__, COND_CHECK_EQ_5, COND_CHECK_EQ_4,
COND_CHECK_EQ_3)(__VA_ARGS__)
+#define COND_CHECK(...) \
+ GET_MACRO(__VA_ARGS__, COND_CHECK_5, COND_CHECK_4, COND_CHECK_3,
COND_CHECK_2)(__VA_ARGS__)
+#define COND_LOG(...) \
+ GET_MACRO(__VA_ARGS__, COND_LOG_5, COND_LOG_4, COND_LOG_3,
COND_LOG_2)(__VA_ARGS__)
+
+// Not supposed to be used by users directly.
+#define COND_CHECK_OP(quit_on_assert, x, y, what, op) \
+ if (!quit_on_assert) { \
+if (!((x)op(y))) what;\
+ } else /* NOLINT(*) */
[GitHub] [tvm] tqchen commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers
tqchen commented on a change in pull request #7153:
URL: https://github.com/apache/tvm/pull/7153#discussion_r552999228
##
File path: src/tir/transforms/lower_thread_allreduce.cc
##
@@ -509,7 +509,6 @@ class ThreadAllreduceBuilder final : public StmtExprMutator
{
}
const AttrStmtNode* op = thread_extents_.back();
-DCHECK_EQ(op->attr_key, attr::thread_extent);
Review comment:
This seems to be un-intentional
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:
us...@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers
tqchen commented on a change in pull request #7153:
URL: https://github.com/apache/tvm/pull/7153#discussion_r552998444
##
File path: src/support/logging.cc
##
@@ -0,0 +1,117 @@
+/*
+ * 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
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+struct frame {};
+
+struct backtrace_info {
+ std::vector lines;
+ size_t max_size;
+ std::string error_message;
+};
+
+static backtrace_state* _backtrace_state = nullptr;
+
+std::string demangle_name(const char* name) {
+ int status = 0;
+ size_t length = std::string::npos;
+ std::unique_ptr demangled_name = {
+ abi::__cxa_demangle(name, 0, , ), ::free};
+ if (demangled_name && status == 0 && length > 0) {
+return demangled_name.get();
+ } else {
+return name;
+ }
+}
+
+void tvm_backtrace_error(void* data, const char* msg, int errnum) {
+ auto stack_trace = reinterpret_cast(data);
+ stack_trace->error_message = msg;
+}
+
+void tvm_backtrace_syminfo_callback(void* data, uintptr_t pc, const char*
symname, uintptr_t symval,
+uintptr_t symsize) {
+ auto str = reinterpret_cast(data);
+
+ if (symname != nullptr) {
+*str = demangle_name(symname);
+ } else {
+std::ostringstream s;
+s << "0x" << std::setfill('0') << std::setw(sizeof(uintptr_t) * 2) <<
std::hex << pc;
+*str = s.str();
+ }
+}
+int tvm_backtrace_full_callback(void* data, uintptr_t pc, const char*
filename, int lineno,
Review comment:
A generic comment, use CamelCase as per Google C style. It is OK to wrap
it in namespace if there is no strict need of a C ABI symbol
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:
us...@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7153: [RUNTIME] Add libbacktrace for backtraces with line numbers
tqchen commented on a change in pull request #7153:
URL: https://github.com/apache/tvm/pull/7153#discussion_r548206504
##
File path: include/tvm/ir/attrs.h
##
@@ -393,7 +392,7 @@ struct AttrInitEntry {
value_missing_ = false;
return *this;
}
- TSelf& describe(DMLC_ATTRIBUTE_UNUSED const char* str) { return *this; }
+ TSelf& describe(__attribute__((unused)) const char* str) { return *this; }
Review comment:
Thanks @tkonolige for the intitial POC. Given the amount of refactoring
needed, which we did not anticipate in the beginning. Let us discuss a bit more
about possible alternatives a good resolution before proceeding with this PR.
A generic marco is needed in most of these cases for cross platform
reasons(MSVC support
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:
us...@infra.apache.org
