fix(compiler): Write universal exports on linked module#2234
Conversation
spotandjake
left a comment
There was a problem hiding this comment.
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.
Why is the remove_export no longer necessary
There was a problem hiding this comment.
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.
Why is the GRAIN$EXPORT$ prefix gone?
There was a problem hiding this comment.
We don't write special Grain exports anymore; now we only write the universal exports (see my other comment).
spotandjake
left a comment
There was a problem hiding this comment.
Looks good to me I had one small question, but otherwise lets get this merged.
| } 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.
Should we be doing a check here and warn and override if a user _start exists?
There was a problem hiding this comment.
Maybe, but that's not for this PR.
In #2104, the
write_universal_exportsfunction 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.