-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RUNTIME] Add libbacktrace for backtraces with line numbers #7153
Conversation
Thank you Tristan for the hard work! Would love to ask some questions just for clarification on the forum first :-) Would you like to also copy your bullet points to the forum as well? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's split the renaming to another PR so that this one could be more reviewable...
I am not the best guy to review the cmake scripts and the cross-platform part (especially windows). I know a really expert @rkimball - would you like to take a second look if you have time? Thanks a lot! |
20b743f
to
c96b076
Compare
@tqchen @junrushao1994 @antinucleon @areusch dmlc-core has merged the changes needed for the PR. I've update this PR to inject logging macros into dmlc-core, so we now use our new logging macros everywhere. I think this PR is ready for a new round of reviews when you get a chance. |
Right now I've put the Error types in the tvm::runtime namespace and then re-exported them from the tvm namespace. Should I just put them in the tvm namespace instead? |
f7f94b1
to
be07941
Compare
08ee22a
to
f5b083c
Compare
@junrushao1994 @tqchen @areusch CI is all green now. Could I get a re-review? |
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use terminology that reflects logging levels here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are levels for feedback to the user from compiling a relay expression. I'm not sure if it makes sense to merge them as these are user-facing while logging is more for developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think logging will be for users when it is possible to adjust the c log level at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you want LOG(BUG)
, LOG(ERROR)
, LOG(WARNING)
, LOG(NOTE)
, LOG(HELP)
? Our current levels are INFO
, WARNING
, ERROR
, FATAL
. We cause alias INFO
to NOTE
and then add BUG
and HELP
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't mind these labels for indicating what a help message contains, but it would be helpful to comment the translation to log levels here so I don't have to look that up. I imagine people will try to work with these labels but will be approaching this from "the build log is too spammy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a translation to log messages here. DiagnosticLevel
is used within relay and TIR to determine how to print the message (these messages are never hidden or filtered).
I've switched this to using a submodule. |
I built and tested locally the backtrace using some existing testcases, and here is the output:
Have to say it is really neat! Thank you so much Tristan! CC: @comaniac @zhiics @yzhliu @icemelon9 @kevinthesun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Some potential improvements in the future:
- Use cmake for libbacktrace in the future. There is no official support but an ongoing PR on the upstream: CMake support ianlancetaylor/libbacktrace#19. Note that it seems to support windows :-)
- Not sure if it is a good idea, but perhaps we can strip some unreadable frames in the STL, like this one:
3: std::enable_if<std::__and_<std::is_void<void>, std::__is_invocable<void (*&)(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*), tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*> >::value, void>::type std::__invoke_r<void, void (*&)(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*), tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*>(void (*&)(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*), tvm::runtime::TVMArgs&&, tvm::runtime::TVMRetValue*&&)
at /usr/include/c++/10/bits/invoke.h:153
I googled around and found some related cmake support for libbacktrace, like libgo: https://go.googlesource.com/gollvm/+/refs/heads/master/cmake/modules/LibbacktraceUtils.cmake @tkonolige do you think we should switch libbacktrace to using cmake, as the tvm project is built with cmake? |
Thanks @tkonolige @rkimball @junrushao1994 @areusch @antinucleon . This PR is now merged |
Thanks everyone for putting the time in to review this. I know it was a big PR. |
@tkonolige Looks like libbacktrace build does not look at
At least it can be disabled |
@apivovarov I believe Tristan is working on this: #7705 (comment) |
) * [RUNTIME] Add libbacktrace for backtraces with line numbers Co-authored-by: Robert Kimball <[email protected]>
) * [RUNTIME] Add libbacktrace for backtraces with line numbers Co-authored-by: Robert Kimball <[email protected]>
include/tvm/support/dmlc.h
and hide LOG and CHECK macros from dmlc.This branch is not up to date with main because the changes to check hit a lot of places in the codebase. I'll update it when we are in agreement to merge it.
RFC: https://discuss.tvm.apache.org/t/rfc-line-numbers-in-backtraces/8711
@areusch @junrushao1994 @u99127