-
-
Notifications
You must be signed in to change notification settings - Fork 117
fix(compiler): Write universal exports on linked module #2234
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
Conversation
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.
Mostly looks good just two small questions.
I was testing with this locally and I think we changed the behaviour of the start function.
when compiled with We also get similar behaviour with |
@spotandjake Good catch. I restored the old behavior. We should maybe consider a warning for reserved export names, but that's not for this PR. |
I just tried this again the
we seem to hit a It might also make sense to add regression tests for both the I did some independent investigation on this and the problem is |
aa96a3d
to
98c4864
Compare
98c4864
to
efe5f4e
Compare
This is good to go. @spotandjake I added tests for that behavior and another test caught a different bug. |
function_body, | ||
); | ||
// Remove existing Grain export (if any) | ||
Export.remove_export(wasm_mod, name); |
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.
Why is the remove_export no longer necessary
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.
When we used wasm files as object files, we wrote special Grain export names in the Wasm binaries to facilitate linking later. We don't do this anymore before linking happens before we generate wasm (so there was no Grain export in the first place to worry about removing).
}, | ||
) => | ||
let name = Ident.name(id); | ||
let exported_name = "GRAIN$EXPORT$" ++ name; |
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.
Why is the GRAIN$EXPORT$
prefix gone?
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.
We don't write special Grain exports anymore; now we only write the universal exports (see my other 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.
Looks good to me I had one small question, but otherwise lets get this merged.
@@ -3311,8 +3267,7 @@ let compile_main = (wasm_mod, env, prog) => { | |||
if (Grain_utils.Config.use_start_section^) { | |||
Function.set_start(wasm_mod, start); | |||
} else { | |||
ignore @@ | |||
Export.add_function_export(wasm_mod, grain_start, Comp_utils.grain_start); | |||
ignore @@ Export.add_function_export(wasm_mod, start_name, grain_start); |
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.
Should we be doing a check here and warn and override if a user _start
exists?
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.
Maybe, but that's not for this PR.
In #2104, the
write_universal_exports
function was previously called after linking, but I forgot to reinstate it. This restores that behavior.I deleted some tests because they were testing old behavior that was valid for the intermediate (object) wasm files but are no longer valid in the output of final wasm modules.