Skip to content

Commit

Permalink
printf: Add Basic DebugPrintf string validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@spencer-lunarg
spencer-lunarg committed Jul 18, 2024
1 parent 352d246 commit 58428b0
Show file tree
Hide file tree
Showing 5 changed files with 638 additions and 5 deletions.
262 changes: 262 additions & 0 deletions layers/core_checks/cc_spirv.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2884,6 +2884,267 @@ bool CoreChecks::ValidateEmitMeshTasksSize(const spirv::Module &module_state, co
return skip;
}

bool CoreChecks::ValidateDebugPrint(const spirv::Module &module_state, const spirv::StatelessData &stateless_data,
const Location &loc) const {
bool skip = false;
if (stateless_data.debug_printf_import_id == 0) return skip;

// Strictly speaking - the format given in GLSL_EXT_debug_printf is a client side implementation of SPIR-V
// NonSemantic.DebugPrintf There is nothing someone from creating a debug printf implementation that goes `printf("Use this &q
// to print int", myInt)` But this requires both having a different HLL and Tool consuming it. Currently RenderDoc and the
// Validation Layers both follow the same syntax.
//
// For now, all of these are marked as "warning" until we feel more confident to make "errors"
//
// If in case someone is hitting this with their full custom implementation stack, two quick suggestions are
// 1. Just go `VK_LAYER_MESSAGE_ID_FILTER=DEBUG-PRINTF-FORMATTING`
// 2. Create a NonSemantic.CustomDebugPrint as that is why the NonSemantic extension was created
const char *vuid = "DEBUG-PRINTF-FORMATTING";

struct ParamInfo {
bool is_float = false; // else int (don't attempt to validate unsigned vs signed here)
bool is_64_bit = false;
uint32_t vector_size = 0; // zero == scalar
char modifier[32];
};

const uint32_t first_argument_offset = 6;
for (const spirv::Instruction *inst : stateless_data.debug_printf_inst) {
ASSERT_AND_CONTINUE(inst->Length() >= first_argument_offset); // spirv-val should be catching this
uint32_t string_id = inst->Word(5);

const char *op_string = nullptr;
for (const spirv::Instruction *string_inst : stateless_data.string_inst) {
if (string_inst->ResultId() == string_id) {
op_string = string_inst->GetAsString(2);
break;
}
}
ASSERT_AND_CONTINUE(op_string);

const size_t op_string_len = strlen(op_string);
if (op_string_len == 0) {
skip |= LogWarning(vuid, module_state.handle(), loc, "OpString is empty (string was found, but is empty)");
continue;
}

bool valid = true;
std::vector<ParamInfo> param_infos;

// No reason to start checking at the last character, since always need % and something following it
for (size_t i = 0; i < op_string_len - 1; i++) {
if (op_string[i] != '%') continue;
const size_t starting_i = i;
i++;
char modifier = op_string[i];
if (modifier == '%') continue; // skip "%%"

if (modifier == ' ') {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a isolated %% which is missing the modifier (to escape use %%%%)",
op_string);
valid = false;
break;
}

bool found_specifier = false;
ParamInfo param_info;
while (i < op_string_len && modifier != ' ' && valid && !found_specifier) {
switch (modifier) {
case 'i':
case 'd':
case 'o':
case 'X':
case 'x':
found_specifier = true;
break;
case 'u':
if (i + 1 < op_string_len && op_string[i + 1] == 'l') {
param_info.is_64_bit = true;
}
found_specifier = true;
break;
case 'a':
case 'A':
case 'e':
case 'E':
case 'f':
case 'F':
case 'g':
case 'G':
found_specifier = true;
param_info.is_float = true;
break;
case 'l':
param_info.is_64_bit = true;
break;
case '0':
case '1':
case '2':
case '3':
case '4':
case '5':
case '6':
case '7':
case '8':
case '9':
case '*':
case '.':
break; // expected for precision
case 'v': {
if (i + 1 >= op_string_len) {
valid = false;
skip |= LogWarning(
vuid, module_state.handle(), loc,
"OpString \"%s\" contains a %%v at the end, but vectors require a width and type after it",
op_string);
} else {
i++;
const char vec_size = op_string[i];
if (vec_size == '2') {
param_info.vector_size = 2;
} else if (vec_size == '3') {
param_info.vector_size = 3;
} else if (vec_size == '4') {
param_info.vector_size = 4;
} else {
skip |=
LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a %%v%c needs to be valid vector width (v2, v3, or v4)",
op_string, vec_size);
valid = false;
}
}
break;
}
default:
// Warning for now incase there are other specifiers in the wild we are missing
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a \"%c\" modifier which is unknown.", op_string, modifier);
valid = false;
break; // unknown
};

i++;
modifier = op_string[i];
}

if (valid) {
// Get for other error messages
strncpy(param_info.modifier, &op_string[starting_i], i - starting_i);
param_info.modifier[i - starting_i] = '\0';

if (!found_specifier) {
// Warning for now incase there are other specifiers in the wild we are missing
skip |=
LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains \"%s\" which is missing a valid specifier (d, i, o, u, x, X, a, A, e, "
"E, f, F, g, or G).",
op_string, param_info.modifier);
valid = false;
}
}

if (!valid) break;
param_infos.push_back(param_info);
}
if (!valid) continue;

const uint32_t argument_count = inst->Length() - first_argument_offset;
if (argument_count > param_infos.size()) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains only %zu modifiers, but %" PRIu32
" arguments were passed in and some will be ignored",
op_string, param_infos.size(), argument_count);
} else if (argument_count < param_infos.size()) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains %zu modifiers, but only %" PRIu32
" arguments were passed in and garbage data might start to occur",
op_string, param_infos.size(), argument_count);
continue;
}

const uint32_t count = std::min(argument_count, (uint32_t)param_infos.size());
for (uint32_t i = 0; i < count; i++) {
const ParamInfo &param = param_infos[i];
const uint32_t argument_id = inst->Word(first_argument_offset + i);

const spirv::Instruction *argument_inst = module_state.FindDef(argument_id);
if (!argument_inst) continue;
const uint32_t type_id = argument_inst->TypeId();
const spirv::Instruction *type_inst = module_state.FindDef(type_id);
if (!type_inst) continue;

// first strip/validate vectors
if (param.vector_size != 0) {
if (type_inst->Opcode() != spv::OpTypeVector) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a vector modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is not a vector",
op_string, param.modifier, argument_id);
continue;
}
const uint32_t vector_size = type_inst->Word(3);
if (vector_size != param.vector_size) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a %" PRIu32
"-wide vector modifier \"%s\", but the argument (SPIR-V Id %" PRIu32 ") is a %" PRIu32
"-wide vector (values might be truncated or padded)",
op_string, param.vector_size, param.modifier, argument_id, vector_size);
}

// Get the underlying type (float or int)
type_inst = module_state.FindDef(type_inst->Word(2));
if (!type_inst) continue;

} else {
if (type_inst->Opcode() == spv::OpTypeVector) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a non-vector modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is a vector",
op_string, param.modifier, argument_id);
continue;
}
}

// this is after stripping the vector
const uint32_t type_inst_opcode = type_inst->Opcode();
if (type_inst_opcode != spv::OpTypeFloat && type_inst_opcode != spv::OpTypeInt && type_inst_opcode != spv::OpTypeBool) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is not a float, int, or bool",
op_string, param.modifier, argument_id);
continue;
}

const bool type_is_64 = type_inst_opcode != spv::OpTypeBool && type_inst->Word(2) == 64;
if (!param.is_64_bit && type_is_64) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a non-64-bit modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is a 64-bit",
op_string, param.modifier, argument_id);
} else if (param.is_64_bit && !type_is_64) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a 64-bit modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is not 64-bit",
op_string, param.modifier, argument_id);
} else if (!param.is_float && type_inst_opcode == spv::OpTypeFloat) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a non-float modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is a float",
op_string, param.modifier, argument_id);
} else if (param.is_float && type_inst_opcode != spv::OpTypeFloat) {
skip |= LogWarning(vuid, module_state.handle(), loc,
"OpString \"%s\" contains a float modifier \"%s\", but the argument (SPIR-V Id %" PRIu32
") is not a float",
op_string, param.modifier, argument_id);
}
}
}

return skip;
}

// stateless spirv == doesn't require pipeline state and/or shader object info
// Originally the goal was to move more validation to vkCreateShaderModule time in case the driver decided to parse an invalid
// SPIR-V here, while that is likely not the case anymore, a bigger reason for checking here is to save on memory. There is a lot of
Expand All @@ -2895,6 +3156,7 @@ bool CoreChecks::ValidateSpirvStateless(const spirv::Module &module_state, const

skip |= ValidateShaderClock(module_state, stateless_data, loc);
skip |= ValidateAtomicsTypes(module_state, stateless_data, loc);
skip |= ValidateDebugPrint(module_state, stateless_data, loc);
skip |= ValidateVariables(module_state, loc);

if (enabled_features.transformFeedback) {
Expand Down
2 changes: 2 additions & 0 deletions layers/core_checks/core_validation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,8 @@ class CoreChecks : public ValidationStateTracker {
const vvl::Pipeline* pipeline, const VkShaderStageFlagBits stage, const Location& loc) const;
bool ValidateAtomicsTypes(const spirv::Module& module_state, const spirv::StatelessData& stateless_data,
const Location& loc) const;
bool ValidateDebugPrint(const spirv::Module& module_state, const spirv::StatelessData& stateless_data,
const Location& loc) const;
bool ValidateShaderFloatControl(const spirv::Module& module_state, const spirv::EntryPoint& entrypoint,
const spirv::StatelessData& stateless_data, const Location& loc) const;
bool ValidateExecutionModes(const spirv::Module& module_state, const spirv::EntryPoint& entrypoint,
Expand Down
24 changes: 24 additions & 0 deletions layers/state_tracker/shader_module.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1072,6 +1072,30 @@ Module::StaticData::StaticData(const Module& module_state, StatelessData* statel
if (insn.Word(4) == GLSLstd450InterpolateAtSample) {
uses_interpolate_at_sample = true;
}
if (stateless_data) {
// From NonSemantic.DebugPrintf.html
const uint32_t debug_printf_insn_number = 1;
if (insn.Word(3) == stateless_data->debug_printf_import_id && insn.Word(4) == debug_printf_insn_number) {
stateless_data->debug_printf_inst.push_back(&insn);
}
}
break;
}

case spv::OpExtInstImport: {
if (stateless_data) {
const char* name = insn.GetAsString(2);
if (strcmp(name, "NonSemantic.DebugPrintf") == 0) {
stateless_data->debug_printf_import_id = insn.ResultId();
}
}
break;
}

case spv::OpString: {
if (stateless_data) {
stateless_data->string_inst.push_back(&insn);
}
break;
}

Expand Down
5 changes: 5 additions & 0 deletions layers/state_tracker/shader_module.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,11 @@ struct StatelessData {
// simpler to just track all OpExecutionModeId and parse things needed later
std::vector<const Instruction *> execution_mode_id_inst;

// OpExtInstImport "NonSemantic.DebugPrintf"
uint32_t debug_printf_import_id = 0;
std::vector<const Instruction *> debug_printf_inst;
std::vector<const Instruction *> string_inst;

bool has_builtin_fully_covered{false};
bool has_invocation_repack_instruction{false};
bool has_group_decoration{false};
Expand Down
Loading

0 comments on commit 58428b0

Please sign in to comment.