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.

Sign up for GitHub

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

Jump to bottom

Add emscripten_memset_js and use it from memset #21683

Closed
wants to merge 2 commits into from
Closed

Add emscripten_memset_js and use it from memset #21683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d15dd6
Add emscripten_memset_js and use it from memset
kg Apr 2, 2024
efdfe40
Fix build
kg Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ addToLibrary({
#else
_emscripten_memcpy_js: (dest, src, num) => HEAPU8.copyWithin(dest, src, src + num),
#endif
_emscripten_memset_js: (dest, value, num) => HEAPU8.fill(value, dest, dest + num),

#endif

Expand Down Expand Up @@ -1204,7 +1205,7 @@ addToLibrary({
// GMT offset as either 'Z' or +-HH:MM or +-HH or +-HHMM
if (value.toLowerCase() === 'z'){
date.gmtoff = 0;
} else {
} else {
var match = value.match(/^((?:\-|\+)\d\d):?(\d\d)?/);
date.gmtoff = match[1] * 3600;
if (match[2]) {
Expand Down Expand Up @@ -1237,7 +1238,7 @@ addToLibrary({
{{{ makeSetValue('tm', C_STRUCTS.tm.tm_yday, 'arraySum(isLeapYear(fullDate.getFullYear()) ? MONTH_DAYS_LEAP : MONTH_DAYS_REGULAR, fullDate.getMonth()-1)+fullDate.getDate()-1', 'i32') }}};
{{{ makeSetValue('tm', C_STRUCTS.tm.tm_isdst, '0', 'i32') }}};
{{{ makeSetValue('tm', C_STRUCTS.tm.tm_gmtoff, 'date.gmtoff', LONG_TYPE) }}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe revert these whitespace changes.


// we need to convert the matched sequence into an integer array to take care of UTF-8 characters > 0x7F
// TODO: not sure that intArrayFromString handles all unicode characters correctly
return buf+intArrayFromString(matches[0]).length-1;
Expand Down
1 change: 1 addition & 0 deletions src/library_sigs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ sigs = {
_emscripten_get_progname__sig: 'vpi',
_emscripten_lookup_name__sig: 'ip',
_emscripten_memcpy_js__sig: 'vppp',
_emscripten_memset_js__sig: 'vpip',
_emscripten_notify_mailbox_postmessage__sig: 'vppp',
_emscripten_push_main_loop_blocker__sig: 'vppp',
_emscripten_push_uncounted_main_loop_blocker__sig: 'vppp',
Expand Down
3 changes: 3 additions & 0 deletions system/lib/libc/emscripten_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ extern "C" {
void _emscripten_memcpy_js(void* __restrict__ dest,
const void* __restrict__ src,
size_t n) EM_IMPORT(_emscripten_memcpy_js);
void _emscripten_memset_js(void* __restrict__ dest,
int value,
size_t n) EM_IMPORT(_emscripten_memset_js);

void* _emscripten_memcpy_bulkmem(void* __restrict__ dest,
const void* __restrict__ src,
Expand Down
22 changes: 20 additions & 2 deletions system/lib/libc/emscripten_memset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,16 @@ __attribute__((no_sanitize("address"))) void *__memset(void *str, int c, size_t
__attribute__((__weak__)) void *__musl_memset(void *str, int c, size_t n);
__attribute__((__weak__)) void *__memset(void *str, int c, size_t n);

#ifdef EMSCRIPTEN_OPTIMIZE_FOR_OZ
#if defined(EMSCRIPTEN_OPTIMIZE_FOR_OZ) || __has_feature(address_sanitizer)

void *__memset(void *str, int c, size_t n) {
#if !defined(EMSCRIPTEN_STANDALONE_WASM)
if (n >= 512) {
_emscripten_memset_js(str, c, n);
return str;
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

It looks like emscripten_memcpy.c only uses the _js version once in that file, in the last case. That is, it doesn't use it in the first case, here, for -Oz/ASan. Is there a reason to do things differently for memset?

Copy link
Author

Choose a reason for hiding this comment

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

We want good performance in -Oz, right? I can understand not wanting it to apply for asan, though, since the JS fill will bypass asan checks.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, for ASan I think we want to be able to instrument it as you said, and performance is less critical there.

For -Oz we've focused on reducing size at all costs, which includes avoiding loop unrolling here. I think avoiding adding a fast path through JS is consistent with that?


unsigned char *s = (unsigned char *)str;
#pragma clang loop unroll(disable)
while(n--) *s++ = c;
Expand All @@ -26,10 +33,21 @@ void *__memset(void *str, int c, size_t n) {

#else

#define memset __memset
#define memset __musl_memset
#include "musl/src/string/memset.c"
#undef memset

void *__memset(void *str, int c, size_t n) {
#if !defined(EMSCRIPTEN_STANDALONE_WASM)
if (n >= 512) {
_emscripten_memset_js(str, c, n);
return str;
}
#endif

return __musl_memset(str, c, n);
}

#endif

weak_alias(__memset, emscripten_builtin_memset);
Expand Down
2 changes: 2 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14057,9 +14057,11 @@ def run(args, expect_bulk_mem):
js = read_file('test_memops_bulk_memory.js')
if expect_bulk_mem:
self.assertNotContained('_emscripten_memcpy_js', js)
self.assertNotContained('_emscripten_memset_js', js)
self.assertIn('$_emscripten_memcpy_bulkmem', funcs)
else:
self.assertContained('_emscripten_memcpy_js', js)
self.assertContained('_emscripten_memset_js', js)
self.assertNotIn('$_emscripten_memcpy_bulkmem', funcs)

# By default we expect to find `_emscripten_memcpy_js` in the generaed JS
Expand Down