Skip to content
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

Export getTempRet0 like other EH library functions #7268

Merged
merged 2 commits into from Oct 12, 2018

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Oct 10, 2018

In the current emscripten EH with the LLVM wasm backend, __tempRet0
variable is in the wasm side and set by JS code and read by wasm code.
So we need setTempRet0 but not getTempRet0. This does not seem to be
currently exported
so it's not used anyway.

In the current emscripten EH with the LLVM wasm backend, `__tempRet0`
variable is in the wasm side and set by JS code and read by wasm code.
So we need `setTempRet0` but not `getTempRet0`. This does not seem to be
currently exported so it's not used anyway.
@kripken
Copy link
Member

kripken commented Oct 11, 2018

I think we use it in another way, when wasm returns an i64 we call setTempRet0 from wasm, then call getTempRet0 from JS.

We do have tests for that, though, so maybe I'm missing something - other.test_sixtyfour_bit_return_value should be failing on the wasm backend currently, without that exported, strange...

@sbc100
Copy link
Collaborator

sbc100 commented Oct 11, 2018

Ah, yes, it looks like these symbols are being injected by binaryen in LegalizeJSInterface. We should probably remove them from there.

@aheejin
Copy link
Member Author

aheejin commented Oct 11, 2018

@sbc100 Then do we even need other functions?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 11, 2018

My understanding is that all three functions are needed yes. We just need to decide who generates them. One option is to simply rely on binaryen (wasm-emscripten-finalize) to generated them and simply delete this C file.

@aheejin
Copy link
Member Author

aheejin commented Oct 11, 2018

I prefer the library option because it feels more normal. But what you mean is, we are currently relying on this file for only two functions (setThrew and setTempRet0) and let Binaryen generate the remaining getTempRet0? Whatever we do I guess we should do in the same way for all the functions...

@kripken
Copy link
Member

kripken commented Oct 11, 2018

Binaryen's legalization will add them if they aren't present (it needs to, so legalization can work), but it won't if they are already present, so this should not cause an error.

@aheejin
Copy link
Member Author

aheejin commented Oct 11, 2018

Hmm, then what should we do? I guess there should be a unified way to handle all three of them. Then would it be better to export getTempRet0 too? If we export all three of them, we don't need LegalizeJSInterface anymore and can delete it, or is the pass being used for other purposes as well?

@kripken
Copy link
Member

kripken commented Oct 11, 2018

I think we can export this method too, so all 3 are exported. That does mean the the legalizeJSInterface pass doesn't need to generate them in the common case, but that's fine - there may still be a case like the optimizer removing these functions, then legalize being run late, and it re-creates them. (In general, we run legalizeJSInterface to make sure the JS interface can be called from JS, so no i64s going in or going out - so we do still need this pass, even if it doesn't need to create the 3 helper functions.)

@aheejin aheejin changed the title Remove getTempRet0 from compiler-rt (NFC) Export getTempRet0 like other EH library functions Oct 11, 2018
@aheejin aheejin merged commit 1773f65 into emscripten-core:incoming Oct 12, 2018
@aheejin aheejin deleted the remove_get_tempret0 branch October 12, 2018 00:38
@sbc100
Copy link
Collaborator

sbc100 commented Oct 12, 2018

The problem is that the implementation of these functions in extras.c and that in LegalizeJSInterface is different. LegalizeJSInterface uses actual wasm globals whereas extras.c uses memory locations.

So this change broke other.test_sixtyfour_bit_return_value I think, at least with the wasm backend, because the tempret global was set by the codegen of LegalizeJSInterface, but since getTempRet0 now comes from extras.c its getting something different.

It looks like code in both llvm and fastcomp for WebAssemblyLowerEmscriptenEHSjLj.cpp uses the memoy locations, so it seem like we've had two diffferent approaches for while now. Could it be that 64-bit return and exceptions never worked at the same time?

@kripken
Copy link
Member

kripken commented Oct 12, 2018

Mixing a get from one mechanism with a set from the other would definitely fail, yeah - is that what happened here? (Otherwise, while extras.c and legalizeJSInterface use a different mechanism, the code calling these APIs should not care about which it is.)

Assuming the issue is mixing a get from one mechanism with a set for the other, I think some options are

  • Standardize how the mechanism works (e.g. a specific wasm global, like stacktop works I think?)
  • Make sure the extras.c pair are kept alive together, or both are dce'd, to avoid mixing. (Technically, by making the get depend on the set and vice versa, using a volatile trick or such.)

It's possible 64-bit return an exceptions never worked together - we just have a few tests for the former, and not many for the latter.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 12, 2018

Can we revert this change for now while we investigate?

aheejin added a commit that referenced this pull request Oct 12, 2018
aheejin added a commit that referenced this pull request Oct 12, 2018
* Revert "Increase use of with_env_modify test decorator (#7269)"

This reverts commit f7ff569.

* Revert "Export getTempRet0 like other EH library functions (#7268)"

This reverts commit 1773f65.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants