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

Allow ASM_CONSTS keys to be non-contiguous #9740

Merged
merged 2 commits into from
Oct 30, 2019
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 30, 2019

As part of the plan to fix #9013, I plan to use the absolute address
of the string in memory to index into this object.

As part of the plan to fix #9013, I plan to use the absolute address
of the string in memory to index into this object.
sbc100 added a commit to WebAssembly/binaryen that referenced this pull request Oct 30, 2019
Before we used 0-based indexes which meant that we needed to modify the
code calling the emask function to replace the first argument.

Now we use the string address itself as the index/code for the emasm
constant.  This allows use code to go unmodifed as the emscripten side
will accept the memory address as the index/code.

See: emscripten-core/emscripten#9013
Depends on: emscripten-core/emscripten#9740
tests/test_other.py Show resolved Hide resolved
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this and the other PRs help with the PIC issue, so I'm missing the point I guess.

Before we had an abstract 0-indexed list of EM_ASMs. After this, we have a map keyed on the string address. The new index depends on memory and so must be adjusted for relocation, which seems more complicated than before?

tests/test_other.py Show resolved Hide resolved
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 30, 2019

I'll try to explain why this helps us.

This change is mostly to make binaryen's job simpler in the AsmConstWalker pass. In the current model the walker has two jobs:

  1. Extract the location of the string constant in linear memory.
  2. Replace arg0 of the function call (which should always evaluate to the (1) above) with a unique integer starting at 0.

After this change binaryen only needs to perform (1). No AST changes are needed at all. This in itself simplifies the pass, but the motivating factor is that doing (2) becomes very hard once arg0 has logic in its children. With PIC code the logic for determining string addresses become "global.get + offset", and llvm can do things like cash the result of the global.get making this even more complicated. We see llvm generating a local.tee to stash the result of the global.get, which means just replacing arg0 with a constant now has unintended side effects. So simply not replacing arg0 seems like a good way to avoid this :)

@sbc100 sbc100 merged commit 4b02802 into incoming Oct 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the allow_sparse_emasm_codes branch October 30, 2019 18:49
sbc100 added a commit that referenced this pull request Oct 30, 2019
See like fetch depends on emscripten_is_main_browser_thread and not
on emscripten_is_main_runtime_thread.

I'm not sure how write a test for this but is came up as part of
#9740.
sbc100 added a commit that referenced this pull request Oct 30, 2019
See like fetch depends on emscripten_is_main_browser_thread and not
on emscripten_is_main_runtime_thread.

I'm not sure how write a test for this but is came up as part of
#9740.
sbc100 added a commit that referenced this pull request Oct 30, 2019
See like fetch depends on emscripten_is_main_browser_thread and not
on emscripten_is_main_runtime_thread.

I'm not sure how write a test for this but is came up as part of
#9740.
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
As part of the plan to fix emscripten-core#9013, I plan to use the absolute address
of the string in memory to index into this object.
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
See like fetch depends on emscripten_is_main_browser_thread and not
on emscripten_is_main_runtime_thread.

I'm not sure how write a test for this but is came up as part of
emscripten-core#9740.
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.

wasm-backend: Code compiled with fPIC is not compatible with regular executables
3 participants