Add bounds checking for enqueue operations to the validation layer.

[aarongreig]

This is accomplished with the various size queries for buffers, images and USM allocations. Since not all adapters have these queries implemented the bounds checking isn't entirely comprehensive on all platforms just yet.

Aims to address issues discussed in #926

[aarongreig]

@pbalcer I'd be interested if you had any feedback about this approach. The implementation does work but it isn't very elegant. I especially hate this https://github.com/oneapi-src/unified-runtime/pull/1093/files#diff-6ccceeaee484ce2a4014958578efbcd7732574d1bf5002a94ed76d992d5367adR1309 but I'm struggling to come up with a better alternative.

[aarongreig]

The other major issue with this is signposting it so that if anyone ever adds a new enqueue operation that could utilize bounds checking it's obvious that they should come and update this mechanism to account for it.

[aarongreig]

pbalcer I'd be interested if you had any feedback about this approach. The implementation does work but it isn't very elegant. I especially hate this https://github.com/oneapi-src/unified-runtime/pull/1093/files#diff-6ccceeaee484ce2a4014958578efbcd7732574d1bf5002a94ed76d992d5367adR1309 but I'm struggling to come up with a better alternative.

What's there now is marginally nicer but I'm still not thrilled about it.

[pbalcer]

Maybe move the list of checks to the spec? So for memcpy, it could look like this:

type: function
desc: "Enqueue a command to copy USM memory" 
...
    - type: void*
      name: pDst
      desc: "[in][bounds_check(size)] pointer to the destination USM memory object"
    - type: "const void*"
      name: pSrc
      desc: "[in][bounds_check(size)] pointer to the source USM memory object"
    - type: size_t
      name: size
      desc: "[in] size in bytes to be copied"
...

Based on that, you can figure out in the validation layer that this is a pointer (so needs to use usm bounds check), and iterate over the rest of arguments to retrieve the context/queue or whatever other object might be needed.

Alternatively, it might also make sense to align to how the rest of the validation layer is defined and put this information in the returns section:

returns:      
    - $X_RESULT_ERROR_INVALID_QUEUE
    - $X_RESULT_ERROR_INVALID_EVENT
    - $X_RESULT_ERROR_INVALID_SIZE:
        - "`bounds_check_usm(pDst, size)`"
        - "`bounds_check_usm(pSrc, size)`"
        - "`size == 0`"
        - "If `size` is higher than the allocation size of `pSrc` or `pDst`"
    - $X_RESULT_ERROR_INVALID_EVENT_WAIT_LIST:
        - "`phEventWaitList == NULL && numEventsInWaitList > 0`"
        - "`phEventWaitList != NULL && numEventsInWaitList == 0`"
        - "If event objects in phEventWaitList are not valid events."
    - $X_RESULT_ERROR_INVALID_MEM_OBJECT
    - $X_RESULT_ERROR_OUT_OF_HOST_MEMORY
    - $X_RESULT_ERROR_OUT_OF_RESOURCES

This is more explicit, but also moves the definition of constrains on a type further away from the type definition itself. Both solutions have merit.

[pbalcer]

If you go with the first approach, another thought I just had is that all the checks have similar arguments: handle/pointer, offset, size (with offset/size potentially having more than 1 dimension). Maybe we should enforce this interface, and so the usm check would become:

    - type: void*
      name: pDst
      desc: "[in][bounds_check(0, size)] pointer to the destination USM memory object"

Since we are checking if the pDst fits inside of the 0<>size bounds. Having a consistent interface would make it easier to use in other parts of the spec.

[aarongreig]

I've implemented the parameter tagging approach, it makes it nice and visible for anyone looking to implement a new entry point with bounds checking and it should generalize to stuff outside enqueue without script modification (up to a point). Thanks for the suggestions. Just looking to see if there's a nicer way to propagate the errors from the GetInfo calls we'll be doing and this should be good to go

[pbalcer]

please add documentation to YaML.md.

[pbalcer]

this is much neater than the first version.

[aarongreig]

@oneapi-src/unified-runtime-hip-write by a stroke of luck this change fixes a CI breakage we've had since friday, could I ask somebody to take a look?

[codecov-commenter]

Codecov Report

Attention: 125 lines in your changes are missing coverage. Please review.

Comparison is base (fe2735a) 15.79% compared to head (4b7f70f) 15.73%.
Report is 4 commits behind head on adapters.

Files	Patch %	Lines
...e/loader/layers/validation/ur_validation_layer.cpp	0.00%	51 Missing ⚠️
source/loader/layers/validation/ur_valddi.cpp	0.00%	46 Missing ⚠️
test/conformance/testing/include/uur/fixtures.h	0.00%	15 Missing ⚠️
test/conformance/kernel/urKernelSetArgSampler.cpp	0.00%	4 Missing ⚠️
include/ur_print.hpp	0.00%	2 Missing ⚠️
...ance/kernel/urKernelSetSpecializationConstants.cpp	0.00%	2 Missing ⚠️
source/adapters/null/ur_nullddi.cpp	0.00%	1 Missing ⚠️
source/loader/layers/tracing/ur_trcddi.cpp	0.00%	1 Missing ⚠️
source/loader/ur_ldrddi.cpp	0.00%	1 Missing ⚠️
source/loader/ur_libapi.cpp	0.00%	1 Missing ⚠️
... and 1 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##           adapters    #1093      +/-   ##
============================================
- Coverage     15.79%   15.73%   -0.06%     
============================================
  Files           223      223              
  Lines         31351    31459     +108     
  Branches       3511     3542      +31     
============================================
  Hits           4951     4951              
- Misses        26349    26457     +108     
  Partials         51       51              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.