-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 very basic logging to the debug info transform #9526
Conversation
crates/cranelift/src/debug/transform/debug_transform_logging.rs
Outdated
Show resolved
Hide resolved
196b783
to
05ae1c8
Compare
The DI transform is a kind of compiler and logging is a very good way to gain insight into compilers.
05ae1c8
to
3e1b255
Compare
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 generally fine, thanks! I personally agree that this sort of tracing, included in checked-in source rather than ad-hoc as one debugs, is really useful when working out issues. So thanks for building this out.
A few thoughts below but nothing major!
crates/cranelift/src/debug/transform/debug_transform_logging.rs
Outdated
Show resolved
Hide resolved
crates/cranelift/src/debug/transform/debug_transform_logging.rs
Outdated
Show resolved
Hide resolved
And switch logging macros to always be enabled in debug. Verified "trace-log" **does not** show up when running 'cargo tree -f "{p} {f}" -e features,normal,build'
971263b
to
2bdaabd
Compare
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, thanks for the patience here!
Hmm, some quite odd breakage.
|
The DI transform is a kind of compiler and logging is a very good way to gain insight into compilers - hence this change.
For example, this has already helped me narrow down the cause of #9512:
I also took the liberty of renaming a few variables for clarity.
Sample output