Generate differentiated error codes for buffer oob checking #4097

greg-lunarg · 2021-01-06T22:48:15Z

This allows the GPU-AV layer to differentiate between errors with
uniform buffers versus storage buffers and map these to the relevant
VUIDs.

greg-lunarg · 2021-01-07T06:12:22Z

@dnovillo Can someone at Google please review?

dnovillo · 2021-01-07T13:26:55Z

@greg-lunarg I'll review today.

dnovillo · 2021-01-07T14:07:23Z

include/spirv-tools/instrument.hpp

 static const int kInstErrorBindlessUninit = 1;
 static const int kInstErrorBuffAddrUnallocRef = 2;
-static const int kInstErrorBindlessBuffOOB = 3;
+// Deleted: static const int kInstErrorBindlessBuffOOB = 3;


Suggest to just delete the line. It can be recovered from revision control if needed later.

Thank you for the careful review! :)

Since this is a public header and I have deleted a previously used code, I did this so if there are problems between the instrumentation and the tool using this instrumentation around this change, it is a little more clear what the problem is ie. that the constant was purposely removed.

Rather than delete this line, I am going to add another comment line explaining things better. Let me know if you still have a problem with this.

Oh, so the value 3 is now reserved in the sense that you need backwards compatibility?

In that case, is there a way for us to at some point remove this line? Or do we have to have this value reserved for ever?

Incidentally, I still don't see the changes you are referring to :)

Can you see the new line at 174?

The comment will eventually go away. It is just there for the transitional period.

I was expecting this rationale in the comment. Otherwise, looking at this code without the context of our conversation will tempt anyone to just get rid of the line.

OK. I have added the rationale. Let me know if this is good.

This change is breaking the validation layers which use the old value. Can we please leave the old value in place, add the new values, update downstream and then remove the old value?

greg-lunarg · 2021-01-08T00:29:21Z

@dnovillo I think I have now addressed all your concerns. Please have another look!

dnovillo · 2021-01-08T16:49:40Z

include/spirv-tools/instrument.hpp

 static const int kInstErrorBindlessUninit = 1;
 static const int kInstErrorBuffAddrUnallocRef = 2;
-static const int kInstErrorBindlessBuffOOB = 3;
+// Deleted: static const int kInstErrorBindlessBuffOOB = 3;


I was expecting this rationale in the comment. Otherwise, looking at this code without the context of our conversation will tempt anyone to just get rid of the line.

dnovillo · 2021-01-08T16:51:59Z

test/opt/inst_bindless_check_test.cpp

+;CHECK:                   OpStore %97 %48
+;CHECK:             %99 = OpIAdd %uint %59 %uint_10
+;CHECK:             %100 = OpAccessChain %_ptr_StorageBuffer_uint %54 %uint_1 %99
+;CHECK:                   OpStore %100 %49


I know this is pre-existing code. However, would it make sense to check a few key elements instead of having CHECKs sprinkled all over the place? May facilitate maintenance in the future. No need to do this now, just wondering if all these CHECKs actually provide value.

Yes, I haven't done a lot to keep these minimal. The thinking was to use CHECK-NOT and CHECK as sort of a -/+ diff format so people, especially maintainers can see exactly the substitutions that are being done. The idea is to use this as a two-fer: documentation and tests at the same time.

This is not to say that some clean up can't be done or wouldn't be helpful. I can probably get rid of definitions of uint_* since these are pretty self explanatory. A number of type declarations could probably be removed as well since their names are so self-explanatory. There is also replication between tests, for instance, the auxiliary functions that are created to read and write the special validation buffers.

I will follow this PR up with a cleanup PR.

SG. Thanks!

This allows the GPU-AV layer to differentiate between errors with uniform buffers versus storage buffers and map these to the relevant VUIDs.

dnovillo

Thanks! LGTM.

dj2 · 2021-01-12T15:28:30Z

I'm going to revert this as it breaks the validation layers by removing kInstErrorBindlessBuffOOB.

…4097)" This reverts commit c32277c.

…4097)" (#4100) This reverts commit c32277c.

greg-lunarg · 2021-01-13T18:42:38Z

@dj2 FWIW, even if we undeleted the old error code, this version is only generating the new error codes, so the layer would still not work. It is not correct to build the layers with any version of glslang and spirv-tools not specified through known-good.

* Deprecate WebGPU SPIRV support Roll third_party/spirv-tools/ 1bb80d2..ee39b5d (16 commits) KhronosGroup/SPIRV-Tools@1bb80d2...ee39b5d $ git log 1bb80d2..ee39b5d --date=short --no-merges --format='%ad %ae %s' 2021-01-15 46493288+sfricke-samsung spirv-val: Add Vulkan Addressing Model check (KhronosGroup#4107) 2021-01-14 rharrison Remove WebGPU support (KhronosGroup#4108) 2021-01-14 46493288+sfricke-samsung spirv-val: Vulkan atomic storage class (KhronosGroup#4079) 2021-01-13 jaebaek Avoid integrity check failures caused by propagating line instructions (KhronosGroup#4096) 2021-01-13 pierremoreau Linker usability improvements (KhronosGroup#4084) 2021-01-12 dj2 Revert "Generate differentiated error codes for buffer oob checking (KhronosGroup#4097)" (KhronosGroup#4100) 2021-01-11 greg Generate differentiated error codes for buffer oob checking (KhronosGroup#4097) 2021-01-07 dneto use std::string::empty() to test for emptiness (KhronosGroup#4098) 2021-01-07 46493288+sfricke-samsung spirv-val: Label standalone Vulkan VUID (KhronosGroup#4091) 2021-01-06 46493288+sfricke-samsung spirv-val: Add Vulkan decroation VUID (KhronosGroup#4090) 2021-01-06 stevenperron Fix binding number calculation in desc sroa (KhronosGroup#4095) 2021-01-06 stevenperron Build deps: dump ini from 1.3.5 to 1.3.7 in tools/sva (KhronosGroup#4092) 2021-01-06 46493288+sfricke-samsung spirv-val: Add Vulkan FP Mode VUID (KhronosGroup#4088) 2021-01-06 46493288+sfricke-samsung spirv-val: Fix Vulkan image sampled check (KhronosGroup#4085) 2021-01-06 46493288+sfricke-samsung spirv-val: Add Vulkan ForwardPointer VUID (KhronosGroup#4089) 2021-01-06 46493288+sfricke-samsung spirv-val: Add Vulkan ImageTexelPointer format check (KhronosGroup#4087) Created with: roll-dep third_party/spirv-tools Fixes KhronosGroup#1166 * Retain the public API enums * Fix implicit fallthrough

greg-lunarg force-pushed the buf_over12 branch from 2ad0145 to 52a07e5 Compare January 7, 2021 02:15

dnovillo self-requested a review January 7, 2021 13:26

dnovillo requested changes Jan 7, 2021

View reviewed changes

greg-lunarg force-pushed the buf_over12 branch from 52a07e5 to 883eee9 Compare January 7, 2021 17:59

dnovillo requested changes Jan 8, 2021

View reviewed changes

Generate differentiated error codes for buffer oob checking

6a7e784

This allows the GPU-AV layer to differentiate between errors with uniform buffers versus storage buffers and map these to the relevant VUIDs.

greg-lunarg force-pushed the buf_over12 branch from 883eee9 to 6a7e784 Compare January 8, 2021 21:58

dnovillo approved these changes Jan 11, 2021

View reviewed changes

dnovillo merged commit c32277c into KhronosGroup:master Jan 11, 2021

dj2 added a commit that referenced this pull request Jan 12, 2021

Revert "Generate differentiated error codes for buffer oob checking (#…

99b4026

…4097)" This reverts commit c32277c.

dj2 mentioned this pull request Jan 12, 2021

Revert "Generate differentiated error codes for buffer oob checking" #4100

Merged

dj2 added a commit that referenced this pull request Jan 12, 2021

Revert "Generate differentiated error codes for buffer oob checking (#…

7bbe1a3

…4097)" (#4100) This reverts commit c32277c.

Generate differentiated error codes for buffer oob checking #4097

Generate differentiated error codes for buffer oob checking #4097

Uh oh!

Conversation

greg-lunarg commented Jan 6, 2021

Uh oh!

greg-lunarg commented Jan 7, 2021

Uh oh!

dnovillo commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-lunarg Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-lunarg commented Jan 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnovillo left a comment

Choose a reason for hiding this comment

Uh oh!

dj2 commented Jan 12, 2021

Uh oh!

greg-lunarg commented Jan 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greg-lunarg Jan 7, 2021 •

edited

Loading