Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add stacktrace into cudf exception types #13298
Add stacktrace into cudf exception types #13298
Changes from 29 commits
7d7cb1e
6b51a29
22e6553
f4ae832
f641add
8d45ee3
aa2b963
78d39f5
cde4ec5
a03a497
1d291fa
c88dc7f
9085058
b158d65
9b04caa
14ab74f
c1d8dee
4747704
97c8046
353cac8
eb324ca
1ebde06
a2c830b
9b816e5
0ca1bba
ef7ebb9
b820f14
e247cb8
4112288
3d7e337
b24420b
5da984a
1033535
991acbc
ba8d97b
06d7ce7
83bd318
695bfa9
0656b3b
3224559
27da4e4
7b2cca9
72859b8
651ed1c
5d86822
7f1df5b
a36c191
f382ea6
0c312c8
f25bf64
bd4085d
e08ba04
e44f2a2
e7c5967
0e0399c
25ff793
dc03826
58ff5c7
6ebee14
9ebe2a2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Folks should be aware that this is changing the size of the exception type even when compiling in non-stacktrace mode.
That isn't to say it's a good idea to not inherit from
stacktrace_recorder
in non-stacktrace mode because there would be the possibility for weird ABI issues with different builds of libcudf having different sizes for exception types.The ideal way to do this without changing the size of the exception type would be to include the stack trace in the
what()
string such that there is only a singlestring
member.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.
The "what()" string is accessed for logging. If we extend it, it may be very large for downstream applications to print out every time. This issue has been mentioned before, please see #12422 (comment).
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.
Does the recorder need to be part of the signature?
The stack trace does not seem to have any real association with the exception object.
Could it not just be a free function used like this?
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.
Unfortunately not.
stacktrace()
gives you the call stack up to the point insidestacktrace()
. From there, you cannot trace back to where the exception was created.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.
In fact, the stack trace is created and attached to the exception object. The exception classes now derive from
stacktrace_recorder
, which captures the call stack immediately upon construction.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.
An alternative would be for each exception type to own a
stacktrace_recorder
that is constructed in its constructor. That approach would require ignoring 2 frames instead of just 1, though, and it would be more verbose. I don't have a strong opinion that that is a necessary change here.