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

Automatically convert i32 pointer args to unsigned when CAN_ADDRESS_2GB is set #19740

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Automatically convert i32 pointer args to unsigned when CAN_ADDRESS_2GB is set #19740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/jsifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ function runJSify() {
if (sig[i] == 'j' && i53abi) {
argConvertions += ` ${receiveI64ParamAsI53(name, undefined, false)}\n`;
newArgs.push(defineI64Param(name));
} else if (sig[i] == 'p' && CAN_ADDRESS_2GB) {
argConvertions += ` ${name} >>>= 0;\n`;
newArgs.push(name);
} else {
newArgs.push(name);
}
Expand Down Expand Up @@ -191,8 +194,8 @@ function(${args}) {

const sig = LibraryManager.library[symbol + '__sig'];
const i53abi = LibraryManager.library[symbol + '__i53abi'];
if (sig && ((i53abi && sig.includes('j')) ||
(MEMORY64 && sig.includes('p')))) {
if (sig &&
((i53abi && sig.includes('j')) || ((MEMORY64 || CAN_ADDRESS_2GB) && sig.includes('p')))) {
snippet = handleI64Signatures(symbol, snippet, sig, i53abi);
i53ConversionDeps.forEach((d) => deps.push(d))
}
Expand Down
8 changes: 6 additions & 2 deletions src/library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ mergeInto(LibraryManager.library, {
$ptrToString: (ptr) => {
#if ASSERTIONS
assert(typeof ptr === 'number');
#endif
#if !CAN_ADDRESS_2GB && !MEMORY64
// With CAN_ADDRESS_2GB or MEMORY64, pointers are already unsigned.
ptr >>>= 0;
#endif
return '0x' + ptr.toString(16).padStart(8, '0');
},
Expand Down Expand Up @@ -199,7 +203,8 @@ mergeInto(LibraryManager.library, {
emscripten_resize_heap: 'ip',
emscripten_resize_heap: (requestedSize) => {
var oldSize = HEAPU8.length;
#if MEMORY64 != 1
#if !MEMORY64 && !CAN_ADDRESS_2GB
// With CAN_ADDRESS_2GB or MEMORY64, pointers are already unsigned.
requestedSize >>>= 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why do this? The code is only run if requestedSize === (requestedSize >>> 0).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for this special case is that we still want emscripten_resize_heap(MAX_INT32) to fail here even if CAN_ADDRESS_2GB is not set.. at least the tests expect that. So this function in particular has to force its parameter to unsigned even when CAN_ADDRESS_2GB is not set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact we want to fail especially if CAN_ADDRESS_2GB is not set.

Without this line the below assert(requestedSize > oldSize); will fail because requestedSize would come through as -1.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right... this is for proper error handling. Higher values are not valid and we error on them.

#endif
#if ALLOW_MEMORY_GROWTH == 0
Expand Down Expand Up @@ -3090,7 +3095,6 @@ mergeInto(LibraryManager.library, {
// Used by wasm-emscripten-finalize to implement STACK_OVERFLOW_CHECK
__handle_stack_overflow__deps: ['emscripten_stack_get_base', 'emscripten_stack_get_end', '$ptrToString'],
__handle_stack_overflow: (requested) => {
requested >>>= 0;
var base = _emscripten_stack_get_base();
var end = _emscripten_stack_get_end();
abort(`stack overflow (Attempt to set SP to ${ptrToString(requested)}` +
Expand Down
12 changes: 6 additions & 6 deletions src/library_fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,8 +1132,8 @@ FS.staticInit();` +
return stream.position;
},
read: (stream, buffer, offset, length, position) => {
#if CAN_ADDRESS_2GB
offset >>>= 0;
#if ASSERTIONS
assert(offset >= 0);
#endif
sbc100 marked this conversation as resolved.
Show resolved Hide resolved
if (length < 0 || position < 0) {
throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
Expand Down Expand Up @@ -1166,8 +1166,8 @@ FS.staticInit();` +
return bytesRead;
},
write: (stream, buffer, offset, length, position, canOwn) => {
#if CAN_ADDRESS_2GB
offset >>>= 0;
#if ASSERTIONS
assert(offset >= 0);
#endif
if (length < 0 || position < 0) {
throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
Expand Down Expand Up @@ -1242,8 +1242,8 @@ FS.staticInit();` +
return stream.stream_ops.mmap(stream, length, position, prot, flags);
},
msync: (stream, buffer, offset, length, mmapFlags) => {
#if CAN_ADDRESS_2GB
offset >>>= 0;
#if ASSERTIONS
assert(offset >= 0);
#endif
if (!stream.stream_ops.msync) {
return 0;
Expand Down
9 changes: 0 additions & 9 deletions src/library_memfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,6 @@ mergeInto(LibraryManager.library, {
// May allocate more, to provide automatic geometric increase and amortized linear performance appending writes.
// Never shrinks the storage.
expandFileStorage: function(node, newCapacity) {
#if CAN_ADDRESS_2GB
newCapacity >>>= 0;
#endif
var prevCapacity = node.contents ? node.contents.length : 0;
if (prevCapacity >= newCapacity) return; // No need to expand, the storage was already large enough.
// Don't expand strictly to the given requested limit if it's only a very small increase, but instead geometrically grow capacity.
Expand All @@ -123,9 +120,6 @@ mergeInto(LibraryManager.library, {

// Performs an exact resize of the backing file storage to the given size, if the size is not exactly this, the storage is fully reallocated.
resizeFileStorage: function(node, newSize) {
#if CAN_ADDRESS_2GB
newSize >>>= 0;
#endif
if (node.usedBytes == newSize) return;
if (newSize == 0) {
node.contents = null; // Fully decommit when requesting a resize to zero.
Expand Down Expand Up @@ -360,9 +354,6 @@ mergeInto(LibraryManager.library, {
if (!ptr) {
throw new FS.ErrnoError({{{ cDefs.ENOMEM }}});
}
#if CAN_ADDRESS_2GB
ptr >>>= 0;
#endif
HEAP8.set(contents, ptr);
}
return { ptr, allocated };
Expand Down
6 changes: 0 additions & 6 deletions src/library_syscall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,6 @@ var SyscallsLibrary = {
// MAP_PRIVATE calls need not to be synced back to underlying fs
return 0;
}
#if CAN_ADDRESS_2GB
addr >>>= 0;
#endif
var buffer = HEAPU8.slice(addr, addr + len);
FS.msync(stream, buffer, offset, len, flags);
},
Expand Down Expand Up @@ -142,9 +139,6 @@ var SyscallsLibrary = {
var res = FS.mmap(stream, len, offset, prot, flags);
var ptr = res.ptr;
{{{ makeSetValue('allocated', 0, 'res.allocated', 'i32') }}};
#if CAN_ADDRESS_2GB
ptr >>>= 0;
#endif
{{{ makeSetValue('addr', 0, 'ptr', '*') }}};
return 0;
#else // no filesystem support; report lack of support
Expand Down
3 changes: 0 additions & 3 deletions src/library_webgpu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1841,7 +1841,6 @@ var LibraryWebGPU = {

if (size === 0) warnOnce('getMappedRange size=0 no longer means WGPU_WHOLE_MAP_SIZE');

size >>>= 0;
if (size === {{{ gpu.WHOLE_MAP_SIZE }}}) size = undefined;

var mapped;
Expand Down Expand Up @@ -1875,7 +1874,6 @@ var LibraryWebGPU = {

if (size === 0) warnOnce('getMappedRange size=0 no longer means WGPU_WHOLE_MAP_SIZE');

size >>>= 0;
if (size === {{{ gpu.WHOLE_MAP_SIZE }}}) size = undefined;

if (bufferWrapper.mapMode !== {{{ gpu.MapMode.Write }}}) {
Expand Down Expand Up @@ -1916,7 +1914,6 @@ var LibraryWebGPU = {
bufferWrapper.onUnmap = [];
var buffer = bufferWrapper.object;

size >>>= 0;
if (size === {{{ gpu.WHOLE_MAP_SIZE }}}) size = undefined;

// `callback` takes (WGPUBufferMapAsyncStatus status, void * userdata)
Expand Down