Handle conflict between debug info and existing validation rule #3104

jaebaek · 2019-12-16T20:32:17Z

This commit includes several changes to resolve conflict between
OpenCL.100.debug extension instructions and existing validation
rule.

Allow us to put OpExtInst between section 9 (type, const) and
section 10 (function) when it is used for debug info (fixes Contradiction between OpExtInst and debug extension spec #3086).
Allow a forward reference to a result of OpFunction from
DebugFunction (fixes DebugFunction has a function as its operand which results in spirv-val "not defined ID" error #3102).
Allow OpExtInst for DebugScope, DebugNoScope,
DebugDeclare, and DebugValue between OpLabel and OpVariable
for function local variable (fixes OpVariable whose storage class is Function right after debug info instructions causes a spirv-val error #3103).
Update IR loader of spirv-opt to make the result of round trip
running spirv-opt the same with the initial SPIR-V code with debug
info.

jaebaek · 2020-01-06T15:46:38Z

After submitting this CL, I will create a separate PR for the validation of debug info itself.

dneto0 · 2020-01-06T17:37:58Z

source/opt/ir_context.h

+  // Iterators for debug info instructions (excluding OpLine & OpNoLine)
+  // contained in this module.  These are OpExtInst for OpenCL.DebugInfo.100
+  // or DebugInfo extension placed between section 9 and 10.
+  inline Module::inst_iterator debuginfo_begin();


The name is confusing because there are so many kinds of debug info.
Please rename this to ext_inst_debuginfo or ext_inst_debug_info

dneto0 · 2020-01-06T17:38:20Z

source/opt/ir_context.h

@@ -1018,6 +1044,10 @@ void IRContext::AddDebug3Inst(std::unique_ptr<Instruction>&& d) {
  module()->AddDebug3Inst(std::move(d));
 }

+void IRContext::AddDebugInfoInst(std::unique_ptr<Instruction>&& d) {


Please rename this method to AddExtInstDebugInfo

dneto0 · 2020-01-06T17:42:12Z

source/opt/ir_loader.cpp

+          const uint32_t ext_inst_index = inst->words[4];
+          const OpenCLDebugInfo100Instructions ext_inst_key =
+              OpenCLDebugInfo100Instructions(ext_inst_index);
+          if (ext_inst_key != OpenCLDebugInfo100DebugScope &&


What happens if this code is executed with a module using the older DebugInfo extended instruction set?
Does it work by accident? fail by accident?
Please add a comment about what is expected to work. And if it's easy to make work with DebugInfo, then please do so.

Good question. I just added supporting the old DebugInfo extension as well.

Note that there is a serious spec issue in the old DebugInfo that it requires to use the result id of OpSource for DebugCompilationUnit, which does not exist (OpSource does not have a result id).
I just added only one simple test case for the old DebugInfo.

dneto0 · 2020-01-06T17:42:59Z

source/reduce/remove_unreferenced_instruction_reduction_opportunity_finder.cpp

@@ -52,6 +52,13 @@ RemoveUnreferencedInstructionReductionOpportunityFinder::
    result.push_back(MakeUnique<RemoveInstructionReductionOpportunity>(&inst));
  }

+  for (auto& inst : context->module()->debuginfo()) {


It's a nice touch to add this to the fuzzer!

jaebaek

PTAL!

jaebaek · 2020-01-06T19:18:26Z

source/opt/ir_context.h

+  // Iterators for debug info instructions (excluding OpLine & OpNoLine)
+  // contained in this module.  These are OpExtInst for OpenCL.DebugInfo.100
+  // or DebugInfo extension placed between section 9 and 10.
+  inline Module::inst_iterator debuginfo_begin();


jaebaek · 2020-01-06T19:18:32Z

source/opt/ir_context.h

@@ -1018,6 +1044,10 @@ void IRContext::AddDebug3Inst(std::unique_ptr<Instruction>&& d) {
  module()->AddDebug3Inst(std::move(d));
 }

+void IRContext::AddDebugInfoInst(std::unique_ptr<Instruction>&& d) {


jaebaek · 2020-01-06T19:19:33Z

source/opt/ir_loader.cpp

+          const uint32_t ext_inst_index = inst->words[4];
+          const OpenCLDebugInfo100Instructions ext_inst_key =
+              OpenCLDebugInfo100Instructions(ext_inst_index);
+          if (ext_inst_key != OpenCLDebugInfo100DebugScope &&


Good question. I just added supporting the old DebugInfo extension as well.

Note that there is a serious spec issue in the old DebugInfo that it requires to use the result id of OpSource for DebugCompilationUnit, which does not exist (OpSource does not have a result id).
I just added only one simple test case for the old DebugInfo.

Fixes KhronosGroup#3086

dneto0

Looking pretty good.
I'd like another test please.

dneto0 · 2020-01-22T21:45:43Z

source/val/validate_layout.cpp

+        if (_.current_layout_section() < kLayoutTypes ||
+            _.current_layout_section() >= kLayoutFunctionDeclarations) {
+          return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
+                 << "OpenCL.DebugInfo.100 instructions other than "


Nit: This applies to both OpenCL.DebugInfo.100 and DebugInfo

I changed the wording "Debug info extension instructions other than .. "

dneto0 · 2020-01-22T21:50:37Z

source/val/validate_layout.cpp

+              // DebugScope, DebugNoScope, DebugDeclare, DebugValue must
+              // appear in a function body.
+              return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
+                     << "DebugScope, DebugNoScope, DebugDeclare, DebugValue "


This failure is not tested.

jaebaek

PTAL

jaebaek · 2020-01-23T15:54:26Z

source/val/validate_layout.cpp

+        if (_.current_layout_section() < kLayoutTypes ||
+            _.current_layout_section() >= kLayoutFunctionDeclarations) {
+          return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
+                 << "OpenCL.DebugInfo.100 instructions other than "


I changed the wording "Debug info extension instructions other than .. "

jaebaek · 2020-01-23T15:54:35Z

source/val/validate_layout.cpp

+              // DebugScope, DebugNoScope, DebugDeclare, DebugValue must
+              // appear in a function body.
+              return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
+                     << "DebugScope, DebugNoScope, DebugDeclare, DebugValue "


Roll third_party/glslang/ 965bd4d..19ec0d2 (4 commits) KhronosGroup/glslang@965bd4d...19ec0d2 $ git log 965bd4d..19ec0d2 --date=short --no-merges --format='%ad %ae %s' 2020-01-27 cepheus Build: Fix more build warnings caused by PR KhronosGroup#2038. 2020-01-26 cepheus Build warning: Fix KhronosGroup#2062, missing enum value in a switch. 2019-12-24 laddoc Add Tess machine dependent built-in variables initialization for GLES 3.2 2019-10-18 timo.suoranta Fixes for gcc 9 / -Werror=deprecated-copy Roll third_party/googletest/ 8b4817e3d..10b1902d8 (6 commits) google/googletest@8b4817e...10b1902 $ git log 8b4817e3d..10b1902d8 --date=short --no-merges --format='%ad %ae %s' 2020-01-21 absl-team Googletest export 2020-01-17 absl-team Googletest export 2020-01-16 absl-team Googletest export 2020-01-15 36923279+ivan1993br Remove exclusion of *-main and*-all targets 2020-01-12 hilmanbeyri Use IsReadableTypeName IsReadableTypeName in OfType function in gmock-matchers_test.cc 2020-01-12 hilmanbeyri fix unit test failure on NoShortCircuitOnFailure and DetectsFlakyShortCircuit when GTEST_HAS_RTTI is 1 Roll third_party/spirv-cross/ f9818f080..68bf0f824 (6 commits) KhronosGroup/SPIRV-Cross@f9818f0...68bf0f8 $ git log f9818f080..68bf0f824 --date=short --no-merges --format='%ad %ae %s' 2020-01-27 post Compile fix on older compilers. 2020-01-27 post GLSL: Support GL_ARB_enchanced_layouts for XFB. 2020-01-25 cdavis MSL: Move inline uniform blocks to the end of the argument buffer. 2019-12-16 cdavis MSL: Support inline uniform blocks in argument buffers. 2020-01-23 post Make SmallVector noexcept. 2020-01-22 42098783+barath121 Typo at line 324 Roll third_party/spirv-headers/ 204cd13..dc77030 (2 commits) KhronosGroup/SPIRV-Headers@204cd13...dc77030 $ git log 204cd13..dc77030 --date=short --no-merges --format='%ad %ae %s' 2020-01-20 dneto Fix the license to match LICENSE 2020-01-20 syoussefi Add BUILD.gn Roll third_party/spirv-tools/ 323a81f..1b34410 (9 commits) KhronosGroup/SPIRV-Tools@323a81f...1b34410 $ git log 323a81f..1b34410 --date=short --no-merges --format='%ad %ae %s' 2020-01-24 syoussefi Fix chromium build (KhronosGroup#3152) 2020-01-24 dneto Clarify mapping of target env to SPIR-V version (KhronosGroup#3150) 2020-01-24 greg Use dummy switch instead of dummy loop in MergeReturn pass. (KhronosGroup#3151) 2020-01-23 alanbaker Fix structured exit validation (KhronosGroup#3141) 2020-01-23 dneto Add spvParseVulkanEnv (KhronosGroup#3142) 2020-01-23 jaebaek Handle conflict between debug info and existing validation rule (KhronosGroup#3104) 2020-01-23 syoussefi Use spirv-headers' BUILD.gn (KhronosGroup#3148) 2020-01-23 syoussefi Roll external/spirv-headers/ af64a9e..dc77030 (4 commits) (KhronosGroup#3147) 2020-01-21 afdx spirv-fuzz: Refactoring and type-related fixes (KhronosGroup#3144) Created with: roll-dep third_party/effcee third_party/glslang third_party/googletest third_party/re2 third_party/spirv-cross third_party/spirv-headers third_party/spirv-tools

jaebaek added the component:validation label Dec 16, 2019

jaebaek self-assigned this Dec 16, 2019

jaebaek changed the title ~~Spvval dbg 3086~~ WIP: Handle spirv-val error for debug info Dec 16, 2019

jaebaek force-pushed the spvval_dbg_3086 branch 2 times, most recently from 3a560d9 to e7bdcdc Compare December 28, 2019 13:31

jaebaek changed the title ~~WIP: Handle spirv-val error for debug info~~ Handle conflict between debug info and existing validation rule Jan 6, 2020

jaebaek requested review from dneto0, s-perron and alan-baker January 6, 2020 15:45

dneto0 requested changes Jan 6, 2020

View reviewed changes

jaebaek commented Jan 6, 2020

View reviewed changes

jaebaek mentioned this pull request Jan 8, 2020

Add validation rules for OpenCL.DebugInfo.100 extension #3133

Merged

jaebaek added 13 commits January 21, 2020 17:07

WIP: Allow OpExtInst for DebugInfo between secion 9 and 10

cb10548

Fixes KhronosGroup#3086

Handle spirv-opt errors on DebugInfo Ext

e2d35ae

Add IR Loader test

a3eaa75

Fix ir loader bug

7863ade

Fix sort error on Windows

cd15801

Fix NDK compiler error

7bd81a7

Fix cmake compiler error

d54e5b4

Handle DebugFunction/DebugTypeMember forward reference

b80c432

Fix Android NDK build failure

5fd3880

Fix Android NDK build failure

8f29c8a

Fix TODOs for test cases (forward reference to function)

87435e0

[refactoring] change confusing function names

80b9e8a

Support old DebugInfo extension

eadbeb9

jaebaek force-pushed the spvval_dbg_3086 branch from c2d40f4 to eadbeb9 Compare January 21, 2020 22:24

dneto0 requested changes Jan 22, 2020

View reviewed changes

Validate local debug info out of function

574e3fd

jaebaek commented Jan 23, 2020

View reviewed changes

dneto0 approved these changes Jan 23, 2020

View reviewed changes

dneto0 merged commit dd37d73 into KhronosGroup:master Jan 23, 2020

jaebaek mentioned this pull request Jun 1, 2020

OpenCL.DebugInfo.100 information preservation in spirv-opt legalization passes #3386

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle conflict between debug info and existing validation rule #3104

Handle conflict between debug info and existing validation rule #3104

jaebaek commented Dec 16, 2019 •

edited

Loading

jaebaek commented Jan 6, 2020

dneto0 Jan 6, 2020

jaebaek Jan 6, 2020

dneto0 Jan 6, 2020

jaebaek Jan 6, 2020

dneto0 Jan 6, 2020

jaebaek Jan 6, 2020

dneto0 Jan 6, 2020

jaebaek Jan 6, 2020

jaebaek left a comment

jaebaek Jan 6, 2020

jaebaek Jan 6, 2020

jaebaek Jan 6, 2020

dneto0 left a comment

dneto0 Jan 22, 2020

jaebaek Jan 23, 2020

dneto0 Jan 22, 2020

jaebaek Jan 23, 2020

jaebaek left a comment

jaebaek Jan 23, 2020

jaebaek Jan 23, 2020

Handle conflict between debug info and existing validation rule #3104

Handle conflict between debug info and existing validation rule #3104

Conversation

jaebaek commented Dec 16, 2019 • edited Loading

jaebaek commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaebaek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dneto0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaebaek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaebaek commented Dec 16, 2019 •

edited

Loading