-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
Fix segfault in static_show, by using correct vt
instead of typeof(v)
#38399
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8f54d34
Fix segfault in static_show, by using correct `vt` instead of `typeof…
NHDaly 2e5f22c
Fix illegal GC-violating call to `jl_subtype` in static_show
NHDaly 81aa68a
PR feedback: JL_NOTSAFEPOINT, jl_function_type
NHDaly 2479af7
Add error checking to `jl_static_is_function_`: null and cycles
NHDaly 7e2d967
Print warning to STDERR instead of throwing exception
NHDaly aa77e35
Remove WARNING logs on `jl_static_is_function_()`
NHDaly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Is this sufficient stopping criteria to make sure we never infinite loop? Should I also check for NULL, for example?
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.
Yes, this is fine.
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.
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.
because of where this gets called from, yeah, you should probably check for NULL also
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.
Yeah, I was just about to come back and say that. This function is regularly called on invalid data. You might also want to include a max trip count or something in case the datatype loops.
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.
Oh, shoot make sense.
Okay, i'll switch to print a warning. Thanks @yuyichao!
Sorry, one more question:
jl_printf((JL_STREAM*)STDERR_FILENO,
orjl_printf(JL_STDERR,
?jl_static_*
functions?Done (for now) in 7e2d967:
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.
(JL_STREAM*)STDERR_FILENO
should be used. The use ofJL_STDERR
higher up in the file are not in the same context. As Keno said, this function needs to work as much as possible even on invalid data for debugging purpose. Reading of invalid memory is actually fine since there's protection agains it. Write of heap memory or use of julia runtime functions or external libraries (e.g. libuv) must be avoided.Personally, I would just use
jl_safe_printf
since that function is basically reserved for this purpose. Alternatively, you can print something (or let the caller do so) to the current output stream, which should be safe as well. Simply ignoring the error is also a safe option and that's howjl_static_show_x_
handles clearly invalid tags.Thinking about it more, I think I personally actually prefer to just ignore the error here. For valid objects, I don't think there is likely any false positive (and if it does the warning printed is wrong anyway...) so the error here doesn't really matter much.
OTOH, when inspecting a heap containing invalid objects, it'll be quite annoy if the printing is constantly interrupted by errors telling me something I already know about (i.e. invalid heap reference....). The only case I see this useful is to detect invalid types when the printing of it somehow still works. While that's not unimaginable, I don't think it's likely and this function isn't really the right place to do such a detection anyway...... We could have a function that walks the heap to detect such inconsistency if we really want to get this info...
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.
FWIW, the rest of this code also used to use an iteration maximum, then print
•
, but it was removed in favor of explicit tracking withstruct recur_list *depth
. The helper functions, such as this one,first_arg_datatype
andjl_static_show_func_sig
have previously ignored those invalid possibilities.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.
Hrmm those are good points...
@vtjnash - without the check for infinite loops, the above example definitely does infinite loop:
Is there some way that I should use this
recur_list *depth
? I'm not exactly sure what you're recommending based on the above insight.Based on the feedback from above, my current thinking is that I'll just remove the warnings, and return false from this function if we encounter a loop or a NULL, since both of those mean: it's not a function!
Does that sound good to you all?
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've pushed up aa77e35 which makes the above change. Please take another look if you can! :)