Skip to content

Conversation

@mannk123
Copy link
Contributor

@mannk123 mannk123 commented Nov 10, 2022

This is a fix for the issue reported in PR #18171 .

More details in my comment on that PR:
#18171 (comment)

#endif

#ifdef __EMSCRIPTEN_WASM_WORKERS__
#define RETRY_SBRK 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combining these into a single block makes sense to me.

Also, perhaps instead of defined(__EMSCRIPTEN_PTHREADS__) || defined(__EMSCRIPTEN_WASM_WORKERS__) you can just use ifdef __EMSCRIPTEN_SHARED_MEMORY__ ? Or equivalently ifdef _REENDTRANT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm well, I suppose this could now just read #ifdef __EMSCRIPTEN_SHARED_MEMORY__ everywhere without introducing a RETRY_SBRK define at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I had thought to keep it for the brevity (since EMSCRIPTEN_SHARED_MEMORY is kind of long), and maybe for future use. Anyway, changed to #ifdef __EMSCRIPTEN_SHARED_MEMORY__ as you suggested.

@juj
Copy link
Collaborator

juj commented Nov 11, 2022

Ohh, good find there were missing shared paths.

I think #ifdef __EMSCRIPTEN_SHARED_MEMORY__ would be good here. That way these call sites will also work in the third build mode, where one is building against shared memory, but without pthreads nor wasm workers.

… use __EMSCRIPTEN_SHARED_MEMORY__ for shared pthread/wasm workers cases
@mannk123
Copy link
Contributor Author

Makes sense, changed to use #ifdef EMSCRIPTEN_SHARED_MEMORY .

@juj juj enabled auto-merge (squash) November 14, 2022 11:58
@juj
Copy link
Collaborator

juj commented Nov 14, 2022

Great, thanks!

@juj juj merged commit 112812d into emscripten-core:main Nov 14, 2022
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.

3 participants