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

Test errors for out-of-range limit requests #3132

Merged
merged 6 commits into from
Nov 3, 2023

Conversation

kainino0x
Copy link
Collaborator

See https://crbug.com/1499030

Issue: None


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@kainino0x kainino0x requested a review from greggman November 2, 2023 23:26
? 'TypeError'
: limitInfo.class === 'maximum' && value > adapter.limits[limit]
? 'OperationError'
: limitInfo.class === 'alignment' && (value > 2 ** 31 || !isPowerOfTwo(value))
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 spec does not actually currently say that 2**32 or 2**53 is an error for alignment limits. I'm proposing it to the spec:
gpuweb/gpuweb#4358

Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

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

I am confused. The limits tests already test trying to go over the max limit and they all pass.

const shouldReject = kMinimumLimits.has(limit)

What is the bug this is trying to surface that wasn't already tested?

Can you just add a case there instead of making a new test?

@kainino0x
Copy link
Collaborator Author

What is the bug this is trying to surface that wasn't already tested?

We mishandle values ≥ 2^32 for 32-bit limit types, by static_cast<uint32_t>(uint64_t):
https://chromium-review.googlesource.com/c/chromium/src/+/5002999

Can you just add a case there instead of making a new test?

I may have missed the appropriate test to modify, but I only saw tests that used mul/add and I couldn't easily get specific corner cases in those tests.
I could certainly reduce the number of subcases in this test though.

Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

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

ok, lgtm 🤷‍♂️

If you wanted to give it a try you could add a new limit here

export const kMaximumTestValues = ['atLimit', 'overLimit'] as const;

atLimit, overLimit, overStorageSize

but I this fine too I guess. It tests more values.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 3, 2023

Ack, I think I'll keep this since it can easily test a lot of values. It also does fit in with the tests next to it (even if we have some redundancy between this, those, and the limit_utils based tests).

@kainino0x kainino0x enabled auto-merge (squash) November 3, 2023 21:51
@kainino0x kainino0x merged commit 3382e0d into gpuweb:main Nov 3, 2023
1 check passed
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 3, 2023
This could have been a 1-character fix, but I preferred to use
CheckedNumeric, and also wanted to update the error message.

Note this doesn't quite match the WebGPU spec - the spec technically
says to allow alignments to be set to powers of 2 >= 2**32, but this
disallows anything higher than 2**31. I'm going to propose to the spec
to tighten this for simplicity of implementation.

Test: gpuweb/cts#3132
Fixed: 1499030
Change-Id: Id2c6e56fcbc326c445b28219b724f6ae325077ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5002999
Auto-Submit: Kai Ninomiya <[email protected]>
Reviewed-by: Brandon Jones <[email protected]>
Commit-Queue: Kai Ninomiya <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1219757}
@kainino0x kainino0x deleted the limits-out-of-range branch November 3, 2023 23:32
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