Skip to content

Commit 5418da0

Browse files
spirv-val: Remove std::move of DiagnosticStream (KhronosGroup#6704)
Just the nit fixes I found in KhronosGroup#6702 Basically calling ```c++ DiagnosticStream ds = std::move(...); return ds; ``` doesn't an implicit clear and would not print the ShaderDebugInfo out and it was very subtle and annoying to debug, so rather just upstream this small fix first
1 parent 2acb87f commit 5418da0

2 files changed

Lines changed: 39 additions & 26 deletions

File tree

source/val/validate_annotation.cpp

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -110,21 +110,23 @@ bool IsNotMemberDecoration(spv::Decoration dec) {
110110
spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
111111
const Instruction* inst,
112112
const Instruction* target) {
113-
auto fail = [&_, dec, inst, target](uint32_t vuid) -> DiagnosticStream {
114-
DiagnosticStream ds = std::move(
115-
_.diag(SPV_ERROR_INVALID_ID, inst)
116-
<< _.VkErrorID(vuid) << _.SpvDecorationString(dec)
117-
<< " decoration on target <id> " << _.getIdName(target->id()) << " ");
118-
return ds;
113+
auto fail = [&_, dec, target]() -> std::string {
114+
std::ostringstream ss;
115+
ss << _.SpvDecorationString(dec) << " decoration on target <id> "
116+
<< _.getIdName(target->id()) << " ";
117+
return ss.str();
119118
};
119+
120120
switch (dec) {
121121
case spv::Decoration::SpecId:
122122
if (target->opcode() != spv::Op::OpSpecConstantTrue &&
123123
target->opcode() != spv::Op::OpSpecConstantFalse &&
124124
target->opcode() != spv::Op::OpSpecConstant &&
125125
target->opcode() != spv::Op::OpSpecConstantDataKHR) {
126-
return fail(0) << "must be OpSpecConstantTrue, OpSpecConstantFalse, "
127-
"OpSpecConstant, or OpSpecConstantDataKHR";
126+
return _.diag(SPV_ERROR_INVALID_ID, inst)
127+
<< fail()
128+
<< "must be OpSpecConstantTrue, OpSpecConstantFalse, "
129+
"OpSpecConstant, or OpSpecConstantDataKHR";
128130
}
129131
break;
130132
case spv::Decoration::Block:
@@ -133,15 +135,17 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
133135
case spv::Decoration::GLSLPacked:
134136
case spv::Decoration::CPacked:
135137
if (target->opcode() != spv::Op::OpTypeStruct) {
136-
return fail(0) << "must be a structure type";
138+
return _.diag(SPV_ERROR_INVALID_ID, inst)
139+
<< fail() << "must be a structure type";
137140
}
138141
break;
139142
case spv::Decoration::ArrayStride:
140143
if (target->opcode() != spv::Op::OpTypeArray &&
141144
target->opcode() != spv::Op::OpTypeRuntimeArray &&
142145
target->opcode() != spv::Op::OpTypePointer &&
143146
target->opcode() != spv::Op::OpTypeUntypedPointerKHR) {
144-
return fail(0) << "must be an array or pointer type";
147+
return _.diag(SPV_ERROR_INVALID_ID, inst)
148+
<< fail() << "must be an array or pointer type";
145149
}
146150
break;
147151
case spv::Decoration::BuiltIn:
@@ -155,11 +159,13 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
155159
if (_.HasCapability(spv::Capability::Shader) &&
156160
inst->GetOperandAs<spv::BuiltIn>(2) == spv::BuiltIn::WorkgroupSize) {
157161
if (!spvOpcodeIsConstant(target->opcode())) {
158-
return fail(0) << "must be a constant for WorkgroupSize";
162+
return _.diag(SPV_ERROR_INVALID_ID, inst)
163+
<< fail() << "must be a constant for WorkgroupSize";
159164
}
160165
} else if (target->opcode() != spv::Op::OpVariable &&
161166
target->opcode() != spv::Op::OpUntypedVariableKHR) {
162-
return fail(0) << "must be a variable";
167+
return _.diag(SPV_ERROR_INVALID_ID, inst)
168+
<< fail() << "must be a variable";
163169
}
164170
break;
165171
case spv::Decoration::NoPerspective:
@@ -185,10 +191,12 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
185191
target->opcode() != spv::Op::OpFunctionParameter &&
186192
target->opcode() != spv::Op::OpRawAccessChainNV &&
187193
target->opcode() != spv::Op::OpBufferPointerEXT) {
188-
return fail(0) << "must be a memory object declaration";
194+
return _.diag(SPV_ERROR_INVALID_ID, inst)
195+
<< fail() << "must be a memory object declaration";
189196
}
190197
if (!_.IsPointerType(target->type_id())) {
191-
return fail(0) << "must be a pointer type";
198+
return _.diag(SPV_ERROR_INVALID_ID, inst)
199+
<< fail() << "must be a pointer type";
192200
}
193201
break;
194202
case spv::Decoration::Invariant:
@@ -200,7 +208,8 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
200208
case spv::Decoration::InputAttachmentIndex:
201209
if (target->opcode() != spv::Op::OpVariable &&
202210
target->opcode() != spv::Op::OpUntypedVariableKHR) {
203-
return fail(0) << "must be a variable";
211+
return _.diag(SPV_ERROR_INVALID_ID, inst)
212+
<< fail() << "must be a variable";
204213
}
205214
break;
206215
default:
@@ -237,7 +246,8 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
237246
case spv::Decoration::Index:
238247
// Langauge from SPIR-V definition of Index
239248
if (sc != spv::StorageClass::Output) {
240-
return fail(0) << "must be in the Output storage class";
249+
return _.diag(SPV_ERROR_INVALID_ID, inst)
250+
<< fail() << "must be in the Output storage class";
241251
}
242252
break;
243253
case spv::Decoration::Binding:
@@ -246,26 +256,34 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, spv::Decoration dec,
246256
sc != spv::StorageClass::Uniform &&
247257
sc != spv::StorageClass::UniformConstant &&
248258
sc != spv::StorageClass::TileAttachmentQCOM) {
249-
return fail(6491) << "must be in the StorageBuffer, Uniform, or "
250-
"UniformConstant storage class";
259+
return _.diag(SPV_ERROR_INVALID_ID, inst)
260+
<< _.VkErrorID(6491) << fail()
261+
<< "must be in the StorageBuffer, Uniform, or "
262+
"UniformConstant storage class";
251263
}
252264
break;
253265
case spv::Decoration::InputAttachmentIndex:
254266
if (sc != spv::StorageClass::UniformConstant) {
255-
return fail(6678) << "must be in the UniformConstant storage class";
267+
return _.diag(SPV_ERROR_INVALID_ID, inst)
268+
<< _.VkErrorID(6678) << fail()
269+
<< "must be in the UniformConstant storage class";
256270
}
257271
break;
258272
case spv::Decoration::Flat:
259273
case spv::Decoration::NoPerspective:
260274
case spv::Decoration::Centroid:
261275
case spv::Decoration::Sample:
262276
if (sc != spv::StorageClass::Input && sc != spv::StorageClass::Output) {
263-
return fail(4670) << "storage class must be Input or Output";
277+
return _.diag(SPV_ERROR_INVALID_ID, inst)
278+
<< _.VkErrorID(4670) << fail()
279+
<< "storage class must be Input or Output";
264280
}
265281
break;
266282
case spv::Decoration::PerVertexKHR:
267283
if (sc != spv::StorageClass::Input) {
268-
return fail(6777) << "storage class must be Input";
284+
return _.diag(SPV_ERROR_INVALID_ID, inst)
285+
<< _.VkErrorID(6777) << fail()
286+
<< "storage class must be Input";
269287
}
270288
break;
271289
default:

test/val/val_shader_debug_info_test.cpp

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1136,11 +1136,6 @@ void main() {
11361136

11371137
TEST_F(ValidateShaderDebugInfo, FunctionParameter) {
11381138
const std::string str = R"(
1139-
; SPIR-V
1140-
; Version: 1.5
1141-
; Generator: Khronos Glslang Reference Front End; 11
1142-
; Bound: 75
1143-
; Schema: 0
11441139
OpCapability Shader
11451140
OpExtension "SPV_KHR_non_semantic_info"
11461141
%1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"

0 commit comments

Comments
 (0)