-
Notifications
You must be signed in to change notification settings - Fork 45
Split compiler integration to avoid duplicating Cthulhu when loading the compiler extension #677
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
Split compiler integration to avoid duplicating Cthulhu when loading the compiler extension #677
Conversation
…o low-level-interface
… into low-level-interface
This way, commands merely register shortcuts to execute callbacks, so they get simplified, and at the same time we no longer have to worry about keeping command "values" and configuration values in sync.
…nnotations The renaming was done to improve consistency with similar settings, `remarks` and `exception_type`.
`IRCode` is now used primarily for callsite detection and display. `CodeInfo` may also be used for that purpose, if no `IRCode` is available. The main requirement is that they are consistent with the provided call infos. `CodeInfo` is mainly used to extract slotnames, get more accurate sources via TypedSyntax, and for module dumping for LLVM/native views.
To match the plural used for `remarks`, `effects`, etc
To improve consistency with `remarks`, `effects`, `exception_type`
Now that we have backspace to ascend, we can have 'q' quit unconditionally and remove this option. Furthermore, we now exit without having to handle an interrupt.
… into split-compiler-integration
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #677 +/- ##
=======================================
Coverage 0.00% 0.00%
=======================================
Files 9 22 +13
Lines 1556 1807 +251
=======================================
- Misses 1556 1807 +251 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Sorry for the delay in reviewing this. After reading the PR description, I think it's a great change overall. There are a few things I can't understand just by reading the code, so I'd like to clarify them:
|
Looks like I can just do julia> using Cthulhu
julia> using Compiler # load custom Compiler impl
julia> descend(sin, (Int,); interp=Compiler.NativeInterpreter())and it's using the new Compiler implementation. That's impressive. |
|
I see, so And I guess the external abstract interpreter package XXX.jl is supposed to overload |
|
Thanks for the review!
More accurately, there is now always a single I'll add a few more docs to provide guidelines and explanations about how to do all that integration before merging, hopefully this PR simplifies the whole process. |
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'll add a few more docs to provide guidelines and explanations about how to do all that integration before merging, hopefully this PR simplifies the whole process.
That's great.
I haven't completed the JET integration with AbstractProvider on my side yet, but I'll try it again on this branch. Last time I tried, I remember running into an error related to this Compiler extension complexity, but looking at the changes in this PR, it seems that's been fixed entirely. (the challenge of integrating with JET is actually the other thing, specifically, I need to properly manage CodeInfo in JET.AbstractAnalyzer for passing them to AbstractProvider. But after this PR, once that's done, Cthulhu should be usable with JET.)
|
Feel free to merge this whenever you are satisfied. |
That should be good now, I'll proceed with the merge and tagging of the breaking release 3.0.0 with adequate release notes. |
This PR splits the compiler integration functionality, such that we only need to re-include the corresponding few compiler-related files when we load the Compiler extension, instead of re-loading most of Cthulhu. These files live under
src/compiler/*.jland essentially hold the interpreter and default provider bits (seesrc/CthulhuCompiler.jlwhich includes these).A major benefit of this approach is that we now have a single API regardless of whether to use the compiler extension: most user-facing data structures live in Cthulhu, and only the parts related to the interfacing with the compiler are either in Cthulhu or CthulhuCompilerExt. No more
Cthulhu.AbstractProvider !== CthulhuCompilerExt.AbstractProvider. However,Cthulhu.DefaultProvider !== CthulhuCompilerExt.DefaultProvidermay still be true. When we load the Compiler extension, we fall in one of two situations:Base.Compiler, in which case we don't need to do anything (Base.Compiler.AbstractInterpreter === Compiler.AbstractInterpreter).Base.Compiler, so we re-includesrc/CthulhuCompiler.jlwhich defines new structs and functions, and adds methods to Cthulhu functions.Another benefit that comes from this is that we won't pay the cost of recompilation in the most common case, i.e. Compiler may be loaded but simply aliases
Base.Compiler, as the Cthulhu extension is virtually a no-op in this situation.