Skip to content

Commit 97cbda9

Browse files
spencer-lunargspencer-lunarg
committed
spirv-val: Fix newline not marked as a line for DebugShaderInfo
1 parent 34bc8ea commit 97cbda9

File tree

2 files changed

+178
-12
lines changed

2 files changed

+178
-12
lines changed

‎source/val/validate_extensions.cpp‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1084,6 +1084,15 @@ spv_result_t ValidateClspvReflectionInstruction(ValidationState_t& _,
10841084
return SPV_SUCCESS;
10851085
}
10861086

1087+
std::string GetDebugSourceText(ValidationState_t& _, const Instruction* inst,
1088+
uint32_t ext_inst_index) {
1089+
// Validated to be an OpString
1090+
const uint32_t string_operand =
1091+
(ext_inst_index == NonSemanticShaderDebugInfo100DebugSource) ? 6 : 5;
1092+
auto* debug_source_text_insn = _.FindDef(inst->word(string_operand));
1093+
return debug_source_text_insn->GetOperandAs<std::string>(1);
1094+
}
1095+
10871096
// We build up a vector that is length of the DebugSource lines and get how long
10881097
// they are to make sure anyone using a DebugSource provides valid Line/Columns
10891098
// inside
@@ -1094,16 +1103,33 @@ void BuildDebugSourceLineLength(ValidationState_t& _, const Instruction* inst,
10941103
return; // The optional text was not provided
10951104
}
10961105

1097-
// Validated to be an OpString
1098-
const uint32_t string_operand =
1099-
(ext_inst_index == NonSemanticShaderDebugInfo100DebugSource) ? 6 : 5;
1100-
auto* debug_source_text_insn = _.FindDef(inst->word(string_operand));
1101-
std::string debug_source_text =
1102-
debug_source_text_insn->GetOperandAs<std::string>(1);
1106+
std::string debug_source_text = GetDebugSourceText(_, inst, ext_inst_index);
11031107

11041108
// walk back to get DebugSource to update it's line length list
11051109
uint32_t debug_source_id = inst->id();
11061110
uint32_t continue_count = 0;
1111+
1112+
// There might be
1113+
// %a = OpString "line starts here"
1114+
// %b = OpString " and still the same line"
1115+
//
1116+
// %c = OpExtInst %void %1 DebugSource %_ %a
1117+
// %d = OpExtInst %void %1 DebugSourceContinued %b
1118+
// So we want to find the previous line for checking if we need to append on
1119+
// to the length of the previous line
1120+
bool start_new_line = true;
1121+
if (ext_inst_index == NonSemanticShaderDebugInfo100DebugSourceContinued) {
1122+
auto prev_index = inst - &_.ordered_instructions()[0] - 1;
1123+
auto prev_inst = &_.ordered_instructions()[prev_index];
1124+
1125+
std::string previous_line_text =
1126+
GetDebugSourceText(_, prev_inst, prev_inst->GetOperandAs<uint32_t>(3));
1127+
if (!previous_line_text.empty() &&
1128+
previous_line_text[previous_line_text.length() - 1] != '\n') {
1129+
start_new_line = false;
1130+
}
1131+
}
1132+
11071133
while (ext_inst_index == NonSemanticShaderDebugInfo100DebugSourceContinued) {
11081134
continue_count++; // might have multiple Continues in a row
11091135
auto prev_index = inst - &_.ordered_instructions()[0] - continue_count;
@@ -1120,6 +1146,12 @@ void BuildDebugSourceLineLength(ValidationState_t& _, const Instruction* inst,
11201146
// Add 1 to length to emulate this later
11211147
uint32_t length = 1;
11221148

1149+
// Continue from the previous line length
1150+
if (!start_new_line) {
1151+
length = line_lengths.back();
1152+
line_lengths.pop_back();
1153+
}
1154+
11231155
for (uint32_t i = 0; i < debug_source_text.size(); ++i) {
11241156
if (debug_source_text[i] == '\n') {
11251157
// Unix-style new line

‎test/val/val_ext_inst_debug_test.cpp‎

Lines changed: 140 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5279,7 +5279,8 @@ TEST_F(ValidateVulkan100DebugInfo, DebugLineDebugSourceContinuedSuccess) {
52795279
%src = OpString "simple.hlsl"
52805280
%code = OpString "line 1
52815281
line 2
5282-
line 3"
5282+
line 3
5283+
"
52835284
%code2 = OpString "line 4
52845285
line 5"
52855286
)";
@@ -5302,7 +5303,8 @@ TEST_F(ValidateVulkan100DebugInfo, DebugLineDebugSourceContinued) {
53025303
const std::string src = R"(
53035304
%src = OpString "simple.hlsl"
53045305
%code = OpString "line 1
5305-
line 2"
5306+
line 2
5307+
"
53065308
%code2 = OpString "line 3
53075309
line 4"
53085310
)";
@@ -5328,9 +5330,11 @@ TEST_F(ValidateVulkan100DebugInfo,
53285330
DebugLineMultipleDebugSourceContinuedSuccess) {
53295331
const std::string src = R"(
53305332
%src = OpString "simple.hlsl"
5331-
%code = OpString "line 1"
5333+
%code = OpString "line 1
5334+
"
53325335
%code2 = OpString "line 2
5333-
line 3"
5336+
line 3
5337+
"
53345338
%code3 = OpString "line 4
53355339
5"
53365340
)";
@@ -5353,9 +5357,11 @@ line 3"
53535357
TEST_F(ValidateVulkan100DebugInfo, DebugLineMultipleDebugSourceContinued) {
53545358
const std::string src = R"(
53555359
%src = OpString "simple.hlsl"
5356-
%code = OpString "line 1"
5360+
%code = OpString "line 1
5361+
"
53575362
%code2 = OpString "line 2
5358-
line 3"
5363+
line 3
5364+
"
53595365
%code3 = OpString "line 4
53605366
5"
53615367
)";
@@ -5817,6 +5823,134 @@ TEST_F(ValidateVulkan100DebugInfo, DebugNoScopeExtraOperandInBody) {
58175823
ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
58185824
}
58195825

5826+
// From
5827+
// https://github.com/KhronosGroup/SPIRV-Tools/pull/5986#issuecomment-4156843064
5828+
TEST_F(ValidateVulkan100DebugInfo,
5829+
DebugLineDebugSourceContinuedEmptyNewLineGood) {
5830+
const std::string src = R"(
5831+
%src = OpString "simple.hlsl"
5832+
%code = OpString "line 1
5833+
line 2
5834+
"
5835+
%code2 = OpString "line 3
5836+
line 4 is really long and hold 32char here"
5837+
5838+
%float_name = OpString "float"
5839+
%foo_name = OpString "foo"
5840+
)";
5841+
5842+
const std::string dbg_inst_header = R"(
5843+
%dbg_src = OpExtInst %void %DbgExt DebugSource %src %code
5844+
%dbg_src2 = OpExtInst %void %DbgExt DebugSourceContinued %code2
5845+
%comp_unit = OpExtInst %void %DbgExt DebugCompilationUnit %u32_2 %u32_4 %dbg_src %u32_5
5846+
%float_info = OpExtInst %void %DbgExt DebugTypeBasic %float_name %u32_32 %u32_3 %u32_0
5847+
%a = OpExtInst %void %DbgExt DebugTypeMember %foo_name %float_info %dbg_src %u32_0 %u32_0 %u32_0 %u32_32 %u32_3
5848+
%t = OpExtInst %void %DbgExt DebugTypeComposite %foo_name %u32_1 %dbg_src %u32_4 %u32_32 %comp_unit %foo_name %u32_32 %u32_3 %a
5849+
)";
5850+
5851+
CompileSuccessfully(GenerateShaderCodeForDebugInfo(
5852+
src, "", dbg_inst_header, "", shader_extension, "Vertex"));
5853+
ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
5854+
}
5855+
5856+
// From
5857+
// https://github.com/KhronosGroup/SPIRV-Tools/pull/6623#issuecomment-4206751217
5858+
TEST_F(ValidateVulkan100DebugInfo, DebugLineDebugSourceContinuedSameLine) {
5859+
const std::string src = R"(
5860+
%src = OpString "simple.hlsl"
5861+
%code0 = OpString "line 1 here
5862+
"
5863+
%code1 = OpString "line 2 here"
5864+
%code2 = OpString " still on line 2"
5865+
%code3 = OpString " still on line 2 and its long
5866+
line 3
5867+
"
5868+
%code4 = OpString "line 4"
5869+
%float_name = OpString "float"
5870+
%foo_name = OpString "foo"
5871+
)";
5872+
5873+
const std::string dbg_inst_header = R"(
5874+
%dbg_src0 = OpExtInst %void %DbgExt DebugSource %src %code0
5875+
%dbg_src1 = OpExtInst %void %DbgExt DebugSourceContinued %code1
5876+
%dbg_src2 = OpExtInst %void %DbgExt DebugSourceContinued %code2
5877+
%dbg_src3 = OpExtInst %void %DbgExt DebugSourceContinued %code3
5878+
%dbg_src4 = OpExtInst %void %DbgExt DebugSourceContinued %code4
5879+
%comp_unit = OpExtInst %void %DbgExt DebugCompilationUnit %u32_2 %u32_4 %dbg_src0 %u32_5
5880+
%float_info = OpExtInst %void %DbgExt DebugTypeBasic %float_name %u32_32 %u32_3 %u32_0
5881+
%a = OpExtInst %void %DbgExt DebugTypeMember %foo_name %float_info %dbg_src0 %u32_0 %u32_0 %u32_0 %u32_32 %u32_3
5882+
%t = OpExtInst %void %DbgExt DebugTypeComposite %foo_name %u32_1 %dbg_src0 %u32_2 %u32_32 %comp_unit %foo_name %u32_32 %u32_3 %a
5883+
)";
5884+
5885+
CompileSuccessfully(GenerateShaderCodeForDebugInfo(
5886+
src, "", dbg_inst_header, "", shader_extension, "Vertex"));
5887+
ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
5888+
}
5889+
5890+
// From
5891+
// https://github.com/KhronosGroup/SPIRV-Tools/pull/5986#issuecomment-4156843064
5892+
TEST_F(ValidateVulkan100DebugInfo,
5893+
DebugLineDebugSourceContinuedEmptyNewLineBad) {
5894+
const std::string src = R"(
5895+
%src = OpString "simple.hlsl"
5896+
%code = OpString "line 1
5897+
line 2
5898+
" ; ignored
5899+
%code2 = OpString "line 3
5900+
line 4 and there is no line 5"
5901+
5902+
%float_name = OpString "float"
5903+
%foo_name = OpString "foo"
5904+
)";
5905+
5906+
const std::string dbg_inst_header = R"(
5907+
%dbg_src = OpExtInst %void %DbgExt DebugSource %src %code
5908+
%dbg_src2 = OpExtInst %void %DbgExt DebugSourceContinued %code2
5909+
%comp_unit = OpExtInst %void %DbgExt DebugCompilationUnit %u32_2 %u32_4 %dbg_src %u32_5
5910+
%float_info = OpExtInst %void %DbgExt DebugTypeBasic %float_name %u32_32 %u32_3 %u32_0
5911+
%a = OpExtInst %void %DbgExt DebugTypeMember %foo_name %float_info %dbg_src %u32_0 %u32_0 %u32_0 %u32_32 %u32_3
5912+
%t = OpExtInst %void %DbgExt DebugTypeComposite %foo_name %u32_1 %dbg_src %u32_5 %u32_1 %comp_unit %foo_name %u32_32 %u32_3 %a
5913+
)";
5914+
5915+
CompileSuccessfully(GenerateShaderCodeForDebugInfo(
5916+
src, "", dbg_inst_header, "", shader_extension, "Vertex"));
5917+
ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
5918+
EXPECT_THAT(getDiagnosticString(),
5919+
HasSubstr("DebugTypeComposite: operand Line (5) is larger then "
5920+
"the 4 lines found in the DebugSource text"));
5921+
}
5922+
5923+
TEST_F(ValidateVulkan100DebugInfo,
5924+
DebugLineDebugSourceContinuedCombinedLineColumnCount) {
5925+
const std::string src = R"(
5926+
%src = OpString "simple.hlsl"
5927+
%code = OpString "line 1" ; 6 column length
5928+
%code2 = OpString "still line 1" ; 12 char
5929+
%code3 = OpString "still line 1" ; 12 char + end-line
5930+
5931+
%float_name = OpString "float"
5932+
%foo_name = OpString "foo"
5933+
)";
5934+
5935+
const std::string dbg_inst_header = R"(
5936+
%dbg_src = OpExtInst %void %DbgExt DebugSource %src %code
5937+
%dbg_src2 = OpExtInst %void %DbgExt DebugSourceContinued %code2
5938+
%dbg_src3 = OpExtInst %void %DbgExt DebugSourceContinued %code3
5939+
%comp_unit = OpExtInst %void %DbgExt DebugCompilationUnit %u32_2 %u32_4 %dbg_src %u32_5
5940+
%float_info = OpExtInst %void %DbgExt DebugTypeBasic %float_name %u32_32 %u32_3 %u32_0
5941+
%a = OpExtInst %void %DbgExt DebugTypeMember %foo_name %float_info %dbg_src %u32_0 %u32_0 %u32_0 %u32_32 %u32_3
5942+
%t = OpExtInst %void %DbgExt DebugTypeComposite %foo_name %u32_1 %dbg_src %u32_1 %u32_32 %comp_unit %foo_name %u32_32 %u32_3 %a
5943+
)";
5944+
5945+
CompileSuccessfully(GenerateShaderCodeForDebugInfo(
5946+
src, "", dbg_inst_header, "", shader_extension, "Vertex"));
5947+
ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
5948+
EXPECT_THAT(
5949+
getDiagnosticString(),
5950+
HasSubstr("DebugTypeComposite: operand Column End (32) is larger then "
5951+
"Line 1 column length of 31 found in the DebugSource text"));
5952+
}
5953+
58205954
} // namespace
58215955
} // namespace val
58225956
} // namespace spvtools

0 commit comments

Comments
 (0)