Skip to content

Use individual socket syscalls. #13272

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

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Use individual socket syscalls. #13272

merged 1 commit into from
Jan 19, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 17, 2021

Remove the SYSCALL_USE_SOCKETCALL definition which was causing
musl to use a single syscall for all socket calls (a mostly legacy
linux convention apparently).

For a little background on this see:
emscripten-core/musl@c2feda4

This is much better for emscripten because it allows DCE and more
precise dependencies.

The changes to library_syscall.js are just re-arranging code, there should be no
functions changes here.

@sbc100 sbc100 force-pushed the remove_socketcall branch from f03a4c9 to 07ff216 Compare January 17, 2021 03:00
@sbc100 sbc100 requested review from juj and kripken January 17, 2021 14:05
@sbc100 sbc100 force-pushed the remove_socketcall branch from 07ff216 to 0a3c359 Compare January 17, 2021 14:10
Remove the SYSCALL_USE_SOCKETCALL definition which was causing
musl to use a single syscall for all socket calls (a mostly legacy
linux convention apparently).

This is much better for emscripten because it allows DCE and more
precise dependencies.
@sbc100 sbc100 force-pushed the remove_socketcall branch from 0a3c359 to b54a861 Compare January 17, 2021 14:19
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 18, 2021

@juj could you take a look? I hope you will be in favor of this as its win for DCE-ability.

#define __NR_sendmsg 370
#define __NR_recvfrom 371
#define __NR_recvmsg 372
#define __NR_shutdown 373
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does musl establish this numbering, or do we? Will this stay in sync with musl?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh nm, reading emscripten-core/musl@c2feda4 suggests these are well established.

It looks like this was a recent change to musl upstream, did we update musl already before to support this? (or do we not really need to do so to update?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC the recent change to upstream musl was to avoid socketcall multiplexing on more architecutres, but I think the idea was that any architectures that support the split out syscalls shoudl already have been using them. (i.e. it was arguably a bug that they were using socketcall).

Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

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

Looks very good!

@sbc100 sbc100 merged commit d7c7aed into master Jan 19, 2021
@sbc100 sbc100 deleted the remove_socketcall branch January 19, 2021 14:50
sbc100 added a commit that referenced this pull request Feb 1, 2021
We never really supported this syscall but prior to #13272 this
fact was not evident since the generic socketcall handler had a
catchall the returned ENOSYS.

Fixes: #13393
sbc100 added a commit that referenced this pull request Feb 2, 2021
We never really supported this syscall but prior to #13272 this
fact was not evident since the generic socketcall handler had a
catchall the returned ENOSYS.

Fixes: #13393
sbc100 added a commit that referenced this pull request Feb 2, 2021
We never really supported this syscall but prior to #13272 this
fact was not evident since the generic socketcall handler had a
catchall the returned ENOSYS.

Fixes: #13393
sbc100 added a commit that referenced this pull request Apr 3, 2023
Also, remove stray `err` logging from #13272.
sbc100 added a commit that referenced this pull request Apr 3, 2023
Also, remove stray `err` logging from #13272.
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.

2 participants