Skip to content

Use more musl code for time to avoid static allocations in JS #12043

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 3 commits into from
Aug 26, 2020
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 25, 2020

Use musl code for non-reentrant time functions to avoid a static allocation in
JS. Instead, C does the allocation and sends the buffer to the _r
versions of the time functions. This is better for code size too.

Musl calls __ prefixed versions, so add aliases for them.

Add a fix from upstream musl to asctime.c.

Add ctime_r and asctime_r to libstandalone, which were missing.

Helps WebAssembly/binaryen#3043

@kripken kripken requested a review from sbc100 August 25, 2020 23:45
@kripken kripken changed the title Nostat t1 Use more musl code for time to avoid static allocations in JS Aug 25, 2020

char *asctime(const struct tm *tm)
{
static char buf[26];
return __asctime(tm, buf);
return __asctime_r(tm, buf);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this file is updated to latest upstream musl, the lack of _r was a bug apparently.

btw, this file is basically the same as the other non-reentrant ones - they allocate a buffer, and then call the reentrant version (which we implement in JS).

@kripken kripken merged commit 635fc7b into master Aug 26, 2020
@kripken kripken deleted the nostat-t1 branch August 26, 2020 13:40
sbc100 added a commit that referenced this pull request Apr 23, 2021
It looks like a patch from a more recent version of musl was applied
in #12043.

This change reverts that and removes the extra alias needed in in
library.js.  library.js now implements just a single `__asctime` which
is used by both `asctime.c` and `asctime_r.c`.

This is in the name doing more without musl matches and less in JS.
sbc100 added a commit that referenced this pull request Apr 23, 2021
It looks like a patch from a more recent version of musl was applied
in #12043.

This change reverts that and removes the extra alias needed in in
library.js.  library.js now implements just a single `__asctime` which
is used by both `asctime.c` and `asctime_r.c`.

This is in the name doing more without musl matches and less in JS.
sbc100 added a commit that referenced this pull request Apr 23, 2021
It looks like a patch from a more recent version of musl was applied
in #12043.

This change reverts that and removes the extra alias needed in in
library.js.  library.js now implements just a single `__asctime` which
is used by both `asctime.c` and `asctime_r.c`.

This is in the name doing more without musl matches and less in JS.
sbc100 added a commit that referenced this pull request Apr 23, 2021
It looks like a patch from a more recent version of musl was applied
in #12043.

This change reverts that and removes the extra alias needed in in
library.js.  library.js now implements just a single `__asctime` which
is used by both `asctime.c` and `asctime_r.c`.

This is in the name doing more without musl matches and less in JS.
sbc100 added a commit that referenced this pull request Jul 20, 2021
Found when auditing untested library functions.

This was added as part of #12043 but unlike the other aliases musl does
not use `__ctime_r` anywhere so this alias is completely unused.
sbc100 added a commit that referenced this pull request Jul 20, 2021
Found when auditing untested library functions.

This was added as part of #12043 but unlike the other aliases musl does
not use `__ctime_r` anywhere so this alias is completely unused.
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