AuroraMiddleware/SPIRV-Tools
1
0
Fork 0
mirror of https://github.com/KhronosGroup/SPIRV-Tools synced 2025-01-11 09:00:06 +00:00
Code Issues Packages Projects Releases Wiki Activity

Change handling of unknown extentions in validtor. (#1951)

Browse Source 
This commit will change the message for unknown extensions from an error
to a warning.

Code was added to limit the number of warning messages so that consummer
of the messages are not overwhelmed.  This is standard practice in
compilers.

Many other issues were found at while looking into this. They have been
documented in #1950.

Fixes http://crbug.com/875547.
This commit is contained in:
Steven Perron 2018-10-03 15:59:40 -04:00 committed by GitHub
parent a8697b0612
commit 19c07731fc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 78 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
source/val/validate.cpp
View File

@ -42,6 +42,12 @@
#include "source/val/validation_state.h"
#include "spirv-tools/libspirv.h"
namespace {
// TODO(issue 1950): The validator only returns a single message anyway, so no
// point in generating more than 1 warning.
static uint32_t kDefaultMaxNumOfWarnings = 1;
} // namespace
namespace spvtools {
namespace val {
namespace {
@ -369,8 +375,8 @@ spv_result_t ValidateBinaryAndKeepValidationState(
UseDiagnosticAsMessageConsumer(&hijack_context, pDiagnostic);
}
vstate->reset(
new ValidationState_t(&hijack_context, options, words, num_words));
vstate->reset(new ValidationState_t(&hijack_context, options, words,
num_words, kDefaultMaxNumOfWarnings));
return ValidateBinaryUsingContextAndValidationState(
hijack_context, words, num_words, pDiagnostic, vstate->get());
@ -400,7 +406,8 @@ spv_result_t spvValidateBinary(const spv_const_context context,
// Create the ValidationState using the context and default options.
spvtools::val::ValidationState_t vstate(&hijack_context, default_options,
words, num_words);
words, num_words,
kDefaultMaxNumOfWarnings);
spv_result_t result =
spvtools::val::ValidateBinaryUsingContextAndValidationState(
@ -422,7 +429,8 @@ spv_result_t spvValidateWithOptions(const spv_const_context context,
// Create the ValidationState using the context.
spvtools::val::ValidationState_t vstate(&hijack_context, options,
binary->code, binary->wordCount);
binary->code, binary->wordCount,
kDefaultMaxNumOfWarnings);
return spvtools::val::ValidateBinaryUsingContextAndValidationState(
hijack_context, binary->code, binary->wordCount, pDiagnostic, &vstate);

4
source/val/validate.h
View File

@ -63,7 +63,7 @@ spv_result_t UpdateIdUse(ValidationState_t& _, const Instruction* inst);
/// @param[in] _ the validation state of the module
///
/// @return SPV_SUCCESS if no errors are found. SPV_ERROR_INVALID_ID otherwise
spv_result_t CheckIdDefinitionDominateUse(const ValidationState_t& _);
spv_result_t CheckIdDefinitionDominateUse(ValidationState_t& _);
/// @brief This function checks for preconditions involving the adjacent
/// instructions.
@ -135,7 +135,7 @@ spv_result_t InstructionPass(ValidationState_t& _, const Instruction* inst);
spv_result_t ValidateDecorations(ValidationState_t& _);
/// Performs validation of built-in variables.
spv_result_t ValidateBuiltIns(const ValidationState_t& _);
spv_result_t ValidateBuiltIns(ValidationState_t& _);
/// Validates type instructions.
spv_result_t TypePass(ValidationState_t& _, const Instruction* inst);

8
source/val/validate_builtins.cpp
View File

@ -55,7 +55,7 @@ std::string GetIdDesc(const Instruction& inst) {
// the Vulkan spec.
// TODO: If non-Vulkan validation rules are added then it might need
// to be refactored.
spv_result_t GetUnderlyingType(const ValidationState_t& _,
spv_result_t GetUnderlyingType(ValidationState_t& _,
const Decoration& decoration,
const Instruction& inst,
uint32_t* underlying_type) {
@ -106,7 +106,7 @@ SpvStorageClass GetStorageClass(const Instruction& inst) {
// ValidationState_t to be made available to other users.
class BuiltInsValidator {
public:
BuiltInsValidator(const ValidationState_t& vstate) : _(vstate) {}
BuiltInsValidator(ValidationState_t& vstate) : _(vstate) {}
// Run validation.
spv_result_t Run();
@ -375,7 +375,7 @@ class BuiltInsValidator {
// instruction.
void Update(const Instruction& inst);
const ValidationState_t& _;
ValidationState_t& _;
// Mapping id -> list of rules which validate instruction referencing the
// id. Rules can create new rules and add them to this container.
@ -2592,7 +2592,7 @@ spv_result_t BuiltInsValidator::Run() {
} // namespace
// Validates correctness of built-in variables.
spv_result_t ValidateBuiltIns(const ValidationState_t& _) {
spv_result_t ValidateBuiltIns(ValidationState_t& _) {
if (!spvIsVulkanEnv(_.context()->target_env)) {
// Early return. All currently implemented rules are based on Vulkan spec.
//

11
source/val/validate_cfg.cpp
View File

@ -296,9 +296,9 @@ std::string ConstructErrorString(const Construct& construct,
// |case_fall_through|. Returns SPV_ERROR_INVALID_CFG if the case construct
// headed by |target_block| branches to multiple case constructs.
spv_result_t FindCaseFallThrough(
const ValidationState_t& _, BasicBlock* target_block,
uint32_t* case_fall_through, const BasicBlock* merge,
const std::unordered_set<uint32_t>& case_targets, Function* function) {
ValidationState_t& _, BasicBlock* target_block, uint32_t* case_fall_through,
const BasicBlock* merge, const std::unordered_set<uint32_t>& case_targets,
Function* function) {
std::vector<BasicBlock*> stack;
stack.push_back(target_block);
std::unordered_set<const BasicBlock*> visited;
@ -352,8 +352,7 @@ spv_result_t FindCaseFallThrough(
return SPV_SUCCESS;
}
spv_result_t StructuredSwitchChecks(const ValidationState_t& _,
Function* function,
spv_result_t StructuredSwitchChecks(ValidationState_t& _, Function* function,
const Instruction* switch_inst,
const BasicBlock* header,
const BasicBlock* merge) {
@ -452,7 +451,7 @@ spv_result_t StructuredSwitchChecks(const ValidationState_t& _,
}
spv_result_t StructuredControlFlowChecks(
const ValidationState_t& _, Function* function,
ValidationState_t& _, Function* function,
const std::vector<std::pair<uint32_t, uint32_t>>& back_edges) {
/// Check all backedges target only loop headers and have exactly one
/// back-edge branching to it

2
source/val/validate_id.cpp
View File

@ -58,7 +58,7 @@ spv_result_t UpdateIdUse(ValidationState_t& _, const Instruction* inst) {
///
/// NOTE: This function does NOT check module scoped functions which are
/// checked during the initial binary parse in the IdPass below
spv_result_t CheckIdDefinitionDominateUse(const ValidationState_t& _) {
spv_result_t CheckIdDefinitionDominateUse(ValidationState_t& _) {
std::vector<const Instruction*> phi_instructions;
std::unordered_set<uint32_t> phi_ids;
for (const auto& inst : _.ordered_instructions()) {

11
source/val/validate_instruction.cpp
View File

@ -89,7 +89,7 @@ CapabilitySet EnablingCapabilitiesForOp(const ValidationState_t& state,
// Returns SPV_SUCCESS if the given operand is enabled by capabilities declared
// in the module. Otherwise issues an error message and returns
// SPV_ERROR_INVALID_CAPABILITY.
spv_result_t CheckRequiredCapabilities(const ValidationState_t& state,
spv_result_t CheckRequiredCapabilities(ValidationState_t& state,
const Instruction* inst,
size_t which_operand,
spv_operand_type_t type,
@ -430,14 +430,15 @@ spv_result_t LimitCheckNumVars(ValidationState_t& _, const uint32_t var_id,
}
// Parses OpExtension instruction and logs warnings if unsuccessful.
void CheckIfKnownExtension(ValidationState_t& _, const Instruction* inst) {
spv_result_t CheckIfKnownExtension(ValidationState_t& _,
const Instruction* inst) {
const std::string extension_str = GetExtensionString(&(inst->c_inst()));
Extension extension;
if (!GetExtensionFromString(extension_str.c_str(), &extension)) {
_.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "Found unrecognized extension " << extension_str;
return;
return _.diag(SPV_WARNING, inst)
<< "Found unrecognized extension " << extension_str;
}
return SPV_SUCCESS;
}
} // namespace

20
source/val/validation_state.cpp
View File

@ -151,7 +151,8 @@ spv_result_t CountInstructions(void* user_data,
ValidationState_t::ValidationState_t(const spv_const_context ctx,
const spv_const_validator_options opt,
const uint32_t* words,
const size_t num_words)
const size_t num_words,
const uint32_t max_warnings)
: context_(ctx),
options_(opt),
words_(words),
@ -170,7 +171,9 @@ ValidationState_t::ValidationState_t(const spv_const_context ctx,
grammar_(ctx),
addressing_model_(SpvAddressingModelMax),
memory_model_(SpvMemoryModelMax),
in_function_(false) {
in_function_(false),
num_of_warnings_(0),
max_num_of_warnings_(max_warnings) {
assert(opt && "Validator options may not be Null.");
const auto env = context_->target_env;
@ -291,7 +294,18 @@ bool ValidationState_t::IsOpcodeInCurrentLayoutSection(SpvOp op) {
}
DiagnosticStream ValidationState_t::diag(spv_result_t error_code,
const Instruction* inst) const {
const Instruction* inst) {
if (error_code == SPV_WARNING) {
if (num_of_warnings_ == max_num_of_warnings_) {
DiagnosticStream({0, 0, 0}, context_->consumer, "", error_code)
<< "Other warnings have been suppressed.\n";
}
if (num_of_warnings_ >= max_num_of_warnings_) {
return DiagnosticStream({0, 0, 0}, nullptr, "", error_code);
}
++num_of_warnings_;
}
std::string disassembly;
if (inst) disassembly = Disassemble(*inst);

9
source/val/validation_state.h
View File

@ -90,7 +90,8 @@ class ValidationState_t {
ValidationState_t(const spv_const_context context,
const spv_const_validator_options opt,
const uint32_t* words, const size_t num_words);
const uint32_t* words, const size_t num_words,
const uint32_t max_warnings);
/// Returns the context
spv_const_context context() const { return context_; }
@ -169,7 +170,7 @@ class ValidationState_t {
/// Determines if the op instruction is part of the current section
bool IsOpcodeInCurrentLayoutSection(SpvOp op);
DiagnosticStream diag(spv_result_t error_code, const Instruction* inst) const;
DiagnosticStream diag(spv_result_t error_code, const Instruction* inst);
/// Returns the function states
std::vector<Function>& functions();
@ -640,6 +641,10 @@ class ValidationState_t {
/// module which can (indirectly) call the function.
std::unordered_map<uint32_t, std::vector<uint32_t>> function_to_entry_points_;
const std::vector<uint32_t> empty_ids_;
/// Variables used to reduce the number of diagnostic messages.
uint32_t num_of_warnings_;
uint32_t max_num_of_warnings_;
};
} // namespace val

23
test/val/val_extensions_test.cpp
View File

@ -88,6 +88,29 @@ TEST_P(ValidateUnknownExtensions, FailSilently) {
EXPECT_THAT(getDiagnosticString(), HasSubstr(GetErrorString(extension)));
}
TEST_F(ValidateUnknownExtensions, HitMaxNumOfWarnings) {
const std::string str =
std::string("OpCapability Shader\n") + "OpCapability Linkage\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpExtension \"bad_ext\"\n" + "OpExtension \"bad_ext\"\n" +
"OpMemoryModel Logical GLSL450";
CompileSuccessfully(str.c_str());
ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
EXPECT_THAT(getDiagnosticString(),
HasSubstr("Other warnings have been suppressed."));
}
TEST_F(ValidateExtensionCapabilities, DeclCapabilitySuccess) {
const std::string str =
"OpCapability Shader\nOpCapability Linkage\nOpCapability DeviceGroup\n"

2
test/val/val_state_test.cpp
View File

@ -41,7 +41,7 @@ class ValidationStateTest : public testing::Test {
ValidationStateTest()
: context_(spvContextCreate(SPV_ENV_UNIVERSAL_1_0)),
options_(spvValidatorOptionsCreate()),
state_(context_, options_, kFakeBinary, 0) {}
state_(context_, options_, kFakeBinary, 0, 1) {}
~ValidationStateTest() {
spvContextDestroy(context_);