-
Notifications
You must be signed in to change notification settings - Fork 3
fix(printer.rs): include ZeroSized FnDef consts in functions #112
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(printer.rs): include ZeroSized FnDef consts in functions #112
Conversation
|
Add a test case that reproduces issue #55 where function pointers passed as arguments (e.g. to Option::map) are not captured in the functions array. Also update normalise-filter.jq to filter unstable type IDs in ArrayType.size.kind.Value.
Zero-sized constants can represent function items (FnDef) used as values, e.g. when passing a function pointer to a higher-order function. This ensures such functions are included in the link map so they appear in the `functions` array of the SMIR JSON. Fixes #55
787a38b to
ea3606b
Compare
When LINK_INST is disabled, the link map key only contains Ty without instance kind, so different instances (Item vs ReifyShim) of the same function type would conflict. Instead of using a heuristic preference function, we now: - Skip ReifyShim entries if an Item entry already exists - Replace ReifyShim entries with Item entries when encountered This is cleaner because ReifyShim has no body in items (it's generated by the linker), so Item is always more useful.
ea3606b to
940f2da
Compare
31f5ec3 to
0253b9c
Compare
| # Remove the hashes at the end of mangled names | ||
| .functions = ( [ .functions[] | if .[1].NormalSym then .[1].NormalSym = .[1].NormalSym[:-17] else . end ] ) | ||
| | .items = ( [ .items[] | if .symbol_name then .symbol_name = .symbol_name[:-17] else . end ] ) | ||
| | .items = ( [ .items[] | if .symbol_name then .symbol_name = .symbol_name[:-17] else . end ] | map(walk(if type == "object" then del(.ty) else . end)) ) |
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.
This is not necessary because
a) it is done again in line 6
b) there can only be NormalSym, NoopSym or IntrinsicSim, with no further fields.
Sorry, I misread this. However, we should place this further down in the "filtering" section instead of up here where we only want to adjust the function names.
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.
Solved
| # delete unstable alloc, function, and type IDs | ||
| | .allocs = ( .allocs | map(del(.alloc_id)) | map(del(.ty)) ) | ||
| | .functions = ( [ .functions[] ] | map(del(.[0])) ) | ||
| | .functions = ( [ .functions[] ] | map(del(.[0])) | map(walk(if type == "object" then del(.ty) else . end)) ) |
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.
not necessary because there can only be NormalSym, NoopSym or IntrinsicSim, with no further fields.
| | .functions = ( [ .functions[] ] | map(del(.[0])) | map(walk(if type == "object" then del(.ty) else . end)) ) | |
| | .functions = ( [ .functions[] ] | map(del(.[0])) ) |
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.
Solved.
jberthold
left a 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.
LGTM, thanks for adjusting!
functionslookup array #55 (and downstream.map(...)desugaring not supported → leads tounknown function -1mir-semantics#891 Support function pointers as callees mir-semantics#488).