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

fix bugs in negative command_buffer tests #1969

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

franz
Copy link
Contributor

@franz franz commented May 23, 2024

  • when calling command buffer APIs, test with command_queue != NULL should return CL_INVALID_VALUE only if the device doesn't support cl_khr_command_buffer_multi_device (added Skip)

  • some tests enqueued commands with multiple invalid arguments, e.g. clCommandCopyImageToBufferKHR with two images and invalid sync points. AFAIK the order of argument checking is not defined, so implementation can return any valid error value for such API calls, but the tests assumed only one particular error would be returned. Fix the API calls to be unambiguous.

- CL_INVALID_VALUE from command_queue != NULL should only be returned
  if the device doesn't support cl_khr_command_buffer_multi_device

- some tests enqueued commands with multiple invalid arguments, e.g.
  clCommandCopyImageToBufferKHR with two images and invalid sync points.
  AFAIK the order of argument checking is not defined, so multiple errors
  can be returned for such API calls, but the tests assumed only one error.
  Fix the tests to be unambiguous.
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM in general, but one comment about calling Skip() in base classes.

As an aside it might be worth checking out a related OpenCL-Docs PR KhronosGroup/OpenCL-Docs#1146 @franz. It's a minor change to make cl_khr_command_buffer_multi_device with clCommandBarrierWithWaitListKHR error behavior consistent with other entry-points entry-point. Since you implement cl_khr_command_buffer_multi_device it would be good to be aware.

Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM

@neiltrevett neiltrevett merged commit 769984b into KhronosGroup:main Jul 2, 2024
7 checks passed
@franz franz deleted the cts_fixes2 branch August 29, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants