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.

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

wasm2c: update memory/table operations to use u64 + harmonize checks #2506

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

keithw
Copy link
Member

@keithw keithw commented Nov 11, 2024

The PR updates the bulk memory operations (memory.fill, memory.copy, table.fill, etc.) to support 64-bit addresses and counts, and standardizes on a 64-bit version of RANGE_CHECK everywhere.

Previously we were only taking u32's for these arguments, even with memory64 enabled. (I don't think the memory64 tests check the ability to use memory.copy or the other operations beyond the first 4 GiB of a memory -- I wonder if there would be a way to add this as an "intensive" test if people don't mind having to allocate >4 GiB to run the test.)

This is a stepping-stone to being able to mix software-bounds-checked i64 memories and "guard-page-checked" i32 memories in the same module (#2507) and supporting custom-page-sizes (#2508).

@keithw keithw requested a review from sbc100 November 11, 2024 09:08
@keithw keithw force-pushed the w2c-harmonize-types branch 2 times, most recently from 176ec50 to 7bea8ee Compare November 11, 2024 10:01
src/template/wasm2c.declarations.c Outdated Show resolved Hide resolved
return _addcarry_u64(0, a, b, resptr);
}
#endif

#define RANGE_CHECK(mem, offset, len) \
Copy link
Collaborator

@shravanrn shravanrn Nov 11, 2024

Choose a reason for hiding this comment

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

While defaulting to the 64-bit RANGE check is fine for memcpy, tables, etc. (it's unlikely to affect performance), the concern is that the 64-bit RANGE_CHECK will slow down accesses to 32-bit linear memories for bounds-checked wasm2c. Firefox uses the bounds-checked wasm2c for Wasm on 32-bit devices, and so it is perf sensitive to this.

I don't know if this is addressed in a future PR, but this particular PR would be a perf problem from the Firefox use case.

  • If you believe future PRs you are landing will give us the property "bounds checks on 32-bit memories are not slowed down", then i don't have any concerns. (I'd prefer landing this PR and the PR that fixes it in quick succession though). I'll look through the other PRs next to see if this is resolved by them

  • If you believe this is not addressed in future PRs, we may need to specialize the bounds checked added depending on the type of memory, which may need specializing i32_load etc. on the type of memory

  • An alternate approach would be to make the current PR about changing the RANGE_CHECK on the memory_fill style operations only, but leaving the RANGE_CHECKs on memory ops as is, i.e., it checks depending on SUPPORT_MEMORY64

Edit: I see that this might possibly be addressed in the next PR. If yes, please disregard the concern

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for these thoughtful (and well-taken) comments. I believe #2507 will nail this for you (by preserving the current RANGE_CHECK on 32-bit, default-page-size memories), so, how about we wait to get alignment on both #2506 and #2507 and then land them at the same time.

I should say that even the current RANGE_CHECK uses 64-bit arithmetic:

#define RANGE_CHECK(mem, offset, len)               \
  if (UNLIKELY(offset + (uint64_t)len > mem->size)) \
    TRAP(OOB);

... but the difference is that RANGE_CHECK64 does an explicit check for 64-bit overflow. I wish I had the benchmarking infrastructure to promise you it won't affect performance on 32-bit x86 but... safer to wait for #2507 which lets you keep the same code.

@keithw keithw force-pushed the w2c-harmonize-types branch from 70556e4 to d15bfb9 Compare November 11, 2024 23:09
@shravanrn
Copy link
Collaborator

shravanrn commented Nov 11, 2024

I wonder if there would be a way to add this as an "intensive" test if people don't mind having to allocate >4 GiB to run the test.

Is the concern that the test-suite may not run on small machines?

One thought I had to run at least simple tests due to lazy memory allocation on Linux-like OSes? If tests are of the form such as below:

create wasm memory of 8 GB
memcpy 100 bytes at index 100
memcpy 100 bytes at index 4GB + 100

this should end up allocating only two physical pages for the heap. So apart from the large virtual memory footprint, the test should run fine even on small machines?

@keithw keithw force-pushed the w2c-harmonize-types branch 2 times, most recently from beb8809 to 64a616d Compare November 12, 2024 05:09
@keithw keithw force-pushed the w2c-harmonize-types branch from 487cb8a to 326e3cb Compare November 12, 2024 18:17
@keithw
Copy link
Member Author

keithw commented Nov 12, 2024

I wonder if there would be a way to add this as an "intensive" test if people don't mind having to allocate >4 GiB to run the test.

Is the concern that the test-suite may not run on small machines?

Yeah, and also that a test could be really slow to run (e.g. memory.fill with an "n" greater than UINT32_MAX).

keithw added a commit that referenced this pull request Dec 4, 2024
(Sequenced behind #2506)

This PR allows "software-bounds-checked" memories and
"guard-page-checked" memories to coexist in the same module.

It creates two versions of every memory operation: an unrestricted
version (that works with any memory) and a `_default32` version (for
memories with default page size and i32 indexing).

The unrestricted version calls `MEMCHECK_GENERAL`, which does a 64-bit
software `RANGE_CHECK` to check that the operation reads/writes within
the bounds of the memory.

The `_default32` version calls `MEMCHECK_DEFAULT32`, which is the same
as the old `MEMCHECK`: if the runtime declares
`WASM_RT_MEMCHECK_GUARD_PAGES`, it will do nothing. Otherwise it will do
a 32-bit software `RANGE_CHECK` (which seems to be one less instruction
than the 64-bit `RANGE_CHECK`).

This is a stepping stone to supporting custom-page-sizes (which will
need to be software bounds-checked) (#2508).
keithw added a commit that referenced this pull request Dec 4, 2024
(Sequenced behind #2506)

This PR allows "software-bounds-checked" memories and
"guard-page-checked" memories to coexist in the same module.

It creates two versions of every memory operation: an unrestricted
version (that works with any memory) and a `_default32` version (for
memories with default page size and i32 indexing).

The unrestricted version calls `MEMCHECK_GENERAL`, which does a 64-bit
software `RANGE_CHECK` to check that the operation reads/writes within
the bounds of the memory.

The `_default32` version calls `MEMCHECK_DEFAULT32`, which is the same
as the old `MEMCHECK`: if the runtime declares
`WASM_RT_MEMCHECK_GUARD_PAGES`, it will do nothing. Otherwise it will do
a 32-bit software `RANGE_CHECK` (which seems to be one less instruction
than the 64-bit `RANGE_CHECK`).

This is a stepping stone to supporting custom-page-sizes (which will
need to be software bounds-checked) (#2508).
@keithw keithw force-pushed the w2c-harmonize-types branch from 6fd3383 to 89ba648 Compare December 4, 2024 20:37
@keithw
Copy link
Member Author

keithw commented Dec 4, 2024

I merged this with #2507 per the requests to avoid a regression in between; please speak up if anybody wants to re-review.

keithw and others added 4 commits December 6, 2024 06:40
(Sequenced behind #2506)

This PR allows "software-bounds-checked" memories and
"guard-page-checked" memories to coexist in the same module.

It creates two versions of every memory operation: an unrestricted
version (that works with any memory) and a `_default32` version (for
memories with default page size and i32 indexing).

The unrestricted version calls `MEMCHECK_GENERAL`, which does a 64-bit
software `RANGE_CHECK` to check that the operation reads/writes within
the bounds of the memory.

The `_default32` version calls `MEMCHECK_DEFAULT32`, which is the same
as the old `MEMCHECK`: if the runtime declares
`WASM_RT_MEMCHECK_GUARD_PAGES`, it will do nothing. Otherwise it will do
a 32-bit software `RANGE_CHECK` (which seems to be one less instruction
than the 64-bit `RANGE_CHECK`).

This is a stepping stone to supporting custom-page-sizes (which will
need to be software bounds-checked) (#2508).
@keithw keithw force-pushed the w2c-harmonize-types branch from 89ba648 to 648efc8 Compare December 6, 2024 14:40
@keithw
Copy link
Member Author

keithw commented Dec 6, 2024

@shravanrn Could you please release your "request changes" unless you still want something on this one?

@keithw
Copy link
Member Author

keithw commented Dec 18, 2024

Okay, with nothing heard, will merge and move on to #2508.

@keithw keithw merged commit ea193b4 into main Dec 18, 2024
18 checks passed
@keithw keithw deleted the w2c-harmonize-types branch December 18, 2024 03:07
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