From 92f21de580e16a37600f082c0968913111f5ef91 Mon Sep 17 00:00:00 2001 From: Yong He Date: Tue, 12 Dec 2023 14:07:35 -0800 Subject: [PATCH] Add check for invalid use of modifiers. (#3402) * Add check for invalid use of modifiers. * Fixes. * Add test. --------- Co-authored-by: Yong He --- source/compiler-core/slang-diagnostic-sink.h | 3 + source/slang/hlsl.meta.slang | 14 +- source/slang/slang-ast-modifier.cpp | 12 ++ source/slang/slang-ast-modifier.h | 2 - source/slang/slang-check-modifier.cpp | 201 +++++++++++++++++- source/slang/slang-diagnostic-defs.h | 3 +- source/slang/slang-parser.cpp | 2 - tests/diagnostics/modifier-check.slang | 9 + .../nv-ray-tracing-motion-blur.slang | 2 +- tests/vkray/raygen.slang | 2 +- 10 files changed, 232 insertions(+), 18 deletions(-) create mode 100644 tests/diagnostics/modifier-check.slang diff --git a/source/compiler-core/slang-diagnostic-sink.h b/source/compiler-core/slang-diagnostic-sink.h index 1969e66b6b..6e3c4ccb88 100644 --- a/source/compiler-core/slang-diagnostic-sink.h +++ b/source/compiler-core/slang-diagnostic-sink.h @@ -102,6 +102,9 @@ void printDiagnosticArg(StringBuilder& sb, Token const& token); struct IRInst; void printDiagnosticArg(StringBuilder& sb, IRInst* irObject); + +class Modifier; +void printDiagnosticArg(StringBuilder& sb, Modifier* modifier); template void printDiagnosticArg(StringBuilder& sb, RefPtr ptr) diff --git a/source/slang/hlsl.meta.slang b/source/slang/hlsl.meta.slang index ac8c59ac6e..5bb5ce0387 100644 --- a/source/slang/hlsl.meta.slang +++ b/source/slang/hlsl.meta.slang @@ -10599,9 +10599,7 @@ struct HitObject case glsl: { // Save the attributes - __ref attr_t attr = __hitObjectAttributes(); - - attr = attributes; + __hitObjectAttributes() = attributes; __glslMakeHit( __return_val, @@ -10670,9 +10668,7 @@ struct HitObject case glsl: { // Save the attributes - __ref attr_t attr = __hitObjectAttributes(); - - attr = attributes; + __hitObjectAttributes() = attributes; __glslMakeMotionHit( __return_val, @@ -10760,8 +10756,7 @@ struct HitObject case glsl: { // Save the attributes - __ref attr_t attr = __hitObjectAttributes(); - attr = attributes; + __hitObjectAttributes() = attributes; __glslMakeHitWithIndex( __return_val, @@ -10824,8 +10819,7 @@ struct HitObject case glsl: { // Save the attributes - __ref attr_t attr = __hitObjectAttributes(); - attr = attributes; + __hitObjectAttributes() = attributes; __glslMakeMotionHitWithIndex( __return_val, diff --git a/source/slang/slang-ast-modifier.cpp b/source/slang/slang-ast-modifier.cpp index 84046a601f..ba30a547db 100644 --- a/source/slang/slang-ast-modifier.cpp +++ b/source/slang/slang-ast-modifier.cpp @@ -10,4 +10,16 @@ const OrderedDictionary& DifferentiableAttribute: m_mapToIDifferentiableWitness.add(m_typeToIDifferentiableWitnessMappings[i].key, m_typeToIDifferentiableWitnessMappings[i].value); return m_mapToIDifferentiableWitness; } + +void printDiagnosticArg(StringBuilder& sb, Modifier* modifier) +{ + if (!modifier) + return; + if (modifier->keywordName && modifier->keywordName->text.getLength()) + sb << modifier->keywordName->text; + if (auto hlslSemantic = as(modifier)) + sb << hlslSemantic->name.getContent(); + return; +} + } // namespace Slang diff --git a/source/slang/slang-ast-modifier.h b/source/slang/slang-ast-modifier.h index ae87c4e10b..2b55c34149 100644 --- a/source/slang/slang-ast-modifier.h +++ b/source/slang/slang-ast-modifier.h @@ -13,7 +13,6 @@ namespace Slang { class InModifier : public Modifier { SLANG_AST_CLASS(InModifier)}; class OutModifier : public Modifier { SLANG_AST_CLASS(OutModifier)}; class ConstModifier : public Modifier { SLANG_AST_CLASS(ConstModifier)}; -class InstanceModifier : public Modifier { SLANG_AST_CLASS(InstanceModifier)}; class BuiltinModifier : public Modifier { SLANG_AST_CLASS(BuiltinModifier)}; class InlineModifier : public Modifier { SLANG_AST_CLASS(InlineModifier)}; class VisibilityModifier : public Modifier {SLANG_AST_CLASS(VisibilityModifier)}; @@ -24,7 +23,6 @@ class RequireModifier : public Modifier { SLANG_AST_CLASS(RequireModifier)}; class ParamModifier : public Modifier { SLANG_AST_CLASS(ParamModifier)}; class ExternModifier : public Modifier { SLANG_AST_CLASS(ExternModifier)}; class HLSLExportModifier : public Modifier { SLANG_AST_CLASS(HLSLExportModifier) }; -class InputModifier : public Modifier { SLANG_AST_CLASS(InputModifier)}; class TransparentModifier : public Modifier { SLANG_AST_CLASS(TransparentModifier)}; class FromStdLibModifier : public Modifier { SLANG_AST_CLASS(FromStdLibModifier)}; class PrefixModifier : public Modifier { SLANG_AST_CLASS(PrefixModifier)}; diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp index 20a393ff5c..53d121b4b3 100644 --- a/source/slang/slang-check-modifier.cpp +++ b/source/slang/slang-check-modifier.cpp @@ -919,6 +919,179 @@ namespace Slang return attr; } + ASTNodeType getModifierConflictGroupKind(ASTNodeType modifierType) + { + switch (modifierType) + { + // Allowed only on parameters and global variables. + case ASTNodeType::InModifier: + return modifierType; + case ASTNodeType::OutModifier: + case ASTNodeType::RefModifier: + case ASTNodeType::ConstRefModifier: + case ASTNodeType::InOutModifier: + return ASTNodeType::OutModifier; + + // Modifiers that are their own exclusive group. + case ASTNodeType::GLSLLayoutModifier: + case ASTNodeType::GLSLParsedLayoutModifier: + case ASTNodeType::GLSLConstantIDLayoutModifier: + case ASTNodeType::GLSLLocationLayoutModifier: + case ASTNodeType::GLSLUnparsedLayoutModifier: + case ASTNodeType::GLSLLayoutModifierGroupMarker: + case ASTNodeType::GLSLLayoutModifierGroupBegin: + case ASTNodeType::GLSLLayoutModifierGroupEnd: + case ASTNodeType::GLSLBufferModifier: + case ASTNodeType::GLSLWriteOnlyModifier: + case ASTNodeType::GLSLReadOnlyModifier: + case ASTNodeType::GLSLPatchModifier: + case ASTNodeType::RayPayloadAccessSemantic: + case ASTNodeType::RayPayloadReadSemantic: + case ASTNodeType::RayPayloadWriteSemantic: + case ASTNodeType::GloballyCoherentModifier: + case ASTNodeType::PreciseModifier: + case ASTNodeType::IntrinsicOpModifier: + case ASTNodeType::InlineModifier: + case ASTNodeType::ExternModifier: + case ASTNodeType::HLSLExportModifier: + case ASTNodeType::ExternCppModifier: + case ASTNodeType::ExportedModifier: + case ASTNodeType::ConstModifier: + case ASTNodeType::ConstExprModifier: + case ASTNodeType::MatrixLayoutModifier: + case ASTNodeType::RowMajorLayoutModifier: + case ASTNodeType::HLSLRowMajorLayoutModifier: + case ASTNodeType::GLSLColumnMajorLayoutModifier: + case ASTNodeType::ColumnMajorLayoutModifier: + case ASTNodeType::HLSLColumnMajorLayoutModifier: + case ASTNodeType::GLSLRowMajorLayoutModifier: + case ASTNodeType::HLSLEffectSharedModifier: + case ASTNodeType::HLSLGroupSharedModifier: + case ASTNodeType::HLSLVolatileModifier: + case ASTNodeType::GLSLPrecisionModifier: + return modifierType; + + case ASTNodeType::HLSLStaticModifier: + case ASTNodeType::ActualGlobalModifier: + case ASTNodeType::HLSLUniformModifier: + return ASTNodeType::HLSLStaticModifier; + + case ASTNodeType::HLSLNoInterpolationModifier: + case ASTNodeType::HLSLNoPerspectiveModifier: + case ASTNodeType::HLSLLinearModifier: + case ASTNodeType::HLSLSampleModifier: + case ASTNodeType::HLSLCentroidModifier: + case ASTNodeType::PerVertexModifier: + return ASTNodeType::InterpolationModeModifier; + + case ASTNodeType::PrefixModifier: + case ASTNodeType::PostfixModifier: + return ASTNodeType::PrefixModifier; + + case ASTNodeType::BuiltinModifier: + case ASTNodeType::PublicModifier: + case ASTNodeType::PrivateModifier: + case ASTNodeType::InternalModifier: + return ASTNodeType::VisibilityModifier; + + default: + return ASTNodeType::NodeBase; + } + } + + bool isModifierAllowedOnDecl(ASTNodeType modifierType, Decl* decl) + { + switch (modifierType) + { + // Allowed only on parameters and global variables. + case ASTNodeType::InModifier: + case ASTNodeType::OutModifier: + case ASTNodeType::InOutModifier: + case ASTNodeType::RefModifier: + case ASTNodeType::ConstRefModifier: + case ASTNodeType::GLSLLayoutModifier: + case ASTNodeType::GLSLParsedLayoutModifier: + case ASTNodeType::GLSLConstantIDLayoutModifier: + case ASTNodeType::GLSLLocationLayoutModifier: + case ASTNodeType::GLSLUnparsedLayoutModifier: + case ASTNodeType::GLSLLayoutModifierGroupMarker: + case ASTNodeType::GLSLLayoutModifierGroupBegin: + case ASTNodeType::GLSLLayoutModifierGroupEnd: + case ASTNodeType::GLSLBufferModifier: + case ASTNodeType::GLSLWriteOnlyModifier: + case ASTNodeType::GLSLReadOnlyModifier: + case ASTNodeType::GLSLPatchModifier: + case ASTNodeType::RayPayloadAccessSemantic: + case ASTNodeType::RayPayloadReadSemantic: + case ASTNodeType::RayPayloadWriteSemantic: + case ASTNodeType::GloballyCoherentModifier: + return (as(decl) && isGlobalDecl(decl)) || as(decl) || as(decl); + + // Allowed only on parameters, struct fields and global variables. + case ASTNodeType::InterpolationModeModifier: + case ASTNodeType::HLSLNoInterpolationModifier: + case ASTNodeType::HLSLNoPerspectiveModifier: + case ASTNodeType::HLSLLinearModifier: + case ASTNodeType::HLSLSampleModifier: + case ASTNodeType::HLSLCentroidModifier: + case ASTNodeType::PerVertexModifier: + case ASTNodeType::HLSLUniformModifier: + return (as(decl) && (isGlobalDecl(decl) || as(getParentDecl(decl)))) || as(decl); + + case ASTNodeType::HLSLSemantic: + case ASTNodeType::HLSLLayoutSemantic: + case ASTNodeType::HLSLRegisterSemantic: + case ASTNodeType::HLSLPackOffsetSemantic: + case ASTNodeType::HLSLSimpleSemantic: + return (as(decl) && (isGlobalDecl(decl) || as(getParentDecl(decl)))) || as(decl) || as(decl); + + // Allowed only on functions + case ASTNodeType::IntrinsicOpModifier: + case ASTNodeType::SpecializedForTargetModifier: + case ASTNodeType::InlineModifier: + case ASTNodeType::PrefixModifier: + case ASTNodeType::PostfixModifier: + return as(decl); + + case ASTNodeType::BuiltinModifier: + case ASTNodeType::PublicModifier: + case ASTNodeType::PrivateModifier: + case ASTNodeType::InternalModifier: + case ASTNodeType::ExternModifier: + case ASTNodeType::HLSLExportModifier: + case ASTNodeType::ExternCppModifier: + return as(decl) || as(decl) || as(decl) || as(decl) + || as(decl) || as(decl) || as(decl) || as(decl); + + case ASTNodeType::ExportedModifier: + return as(decl); + + case ASTNodeType::ConstModifier: + case ASTNodeType::HLSLStaticModifier: + case ASTNodeType::ConstExprModifier: + case ASTNodeType::PreciseModifier: + return as(decl) || as(decl); + + case ASTNodeType::ActualGlobalModifier: + case ASTNodeType::MatrixLayoutModifier: + case ASTNodeType::RowMajorLayoutModifier: + case ASTNodeType::HLSLRowMajorLayoutModifier: + case ASTNodeType::GLSLColumnMajorLayoutModifier: + case ASTNodeType::ColumnMajorLayoutModifier: + case ASTNodeType::HLSLColumnMajorLayoutModifier: + case ASTNodeType::GLSLRowMajorLayoutModifier: + case ASTNodeType::HLSLEffectSharedModifier: + case ASTNodeType::HLSLGroupSharedModifier: + case ASTNodeType::HLSLVolatileModifier: + return as(decl) || as(decl); + + case ASTNodeType::GLSLPrecisionModifier: + return as(decl) || as(decl) || as(decl); + default: + return true; + } + } + Modifier* SemanticsVisitor::checkModifier( Modifier* m, ModifiableSyntaxNode* syntaxNode) @@ -935,6 +1108,15 @@ namespace Slang return checkAttribute(hlslUncheckedAttribute, syntaxNode); } + if (auto decl = as(syntaxNode)) + { + if (!isModifierAllowedOnDecl(m->astNodeType, decl)) + { + getSink()->diagnose(m, Diagnostics::modifierNotAllowed, m); + return m; + } + } + if (auto hlslSemantic = as(m)) { if (hlslSemantic->name.getName() == getSession()->getCompletionRequestTokenName()) @@ -1164,9 +1346,25 @@ namespace Slang Modifier* resultModifiers = nullptr; Modifier** resultModifierLink = &resultModifiers; + // We will keep track of the modifiers for each conflict group. + Dictionary mapExclusiveGroupToModifier; + Modifier* modifier = syntaxNode->modifiers.first; - while(modifier) + while (modifier) { + // Check if a modifier belonging to the same conflict group is already + // defined. + Modifier* existingModifier = nullptr; + auto conflictGroup = getModifierConflictGroupKind(modifier->astNodeType); + if (conflictGroup != ASTNodeType::NodeBase) + { + if (mapExclusiveGroupToModifier.tryGetValue(conflictGroup, existingModifier)) + { + getSink()->diagnose(modifier->loc, Diagnostics::duplicateModifier, modifier, existingModifier); + } + mapExclusiveGroupToModifier[conflictGroup] = modifier; + } + // Because we are rewriting the list in place, we need to extract // the next modifier here (not at the end of the loop). auto next = modifier->next; @@ -1177,6 +1375,7 @@ namespace Slang modifier->next = nullptr; auto checkedModifier = checkModifier(modifier, syntaxNode); + if(checkedModifier) { // If checking gave us a modifier to add, then we diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h index 5f28e2af2f..429865b757 100644 --- a/source/slang/slang-diagnostic-defs.h +++ b/source/slang/slang-diagnostic-defs.h @@ -421,7 +421,8 @@ DIAGNOSTIC(31153, Error, cannotUseInterfaceRequirementAsDerivative, "cannot use DIAGNOSTIC(31154, Error, customDerivativeSignatureThisParamMismatch, "custom derivative does not match expected signature on `this`. Either both the original and the derivative function are static, or they must have the same `this` type.") DIAGNOSTIC(31155, Error, customDerivativeNotAllowedForMemberFunctionsOfDifferentiableType, "custom derivative is not allowed for non-static member functions of a differentiable type.") DIAGNOSTIC(31200, Warning, deprecatedUsage, "$0 has been deprecated: $1") - +DIAGNOSTIC(31201, Error, modifierNotAllowed, "modifier '$0' is not allowed here.") +DIAGNOSTIC(31202, Error, duplicateModifier, "modifier '$0' is redundant or conflicting with existing modifier '$1'") // Enums DIAGNOSTIC(32000, Error, invalidEnumTagType, "invalid tag type for 'enum': '$0'") diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp index 9914612ac5..361786b6f9 100644 --- a/source/slang/slang-parser.cpp +++ b/source/slang/slang-parser.cpp @@ -7756,13 +7756,11 @@ namespace Slang // a new AST node of the corresponding type. _makeParseModifier("in", InModifier::kReflectClassInfo), - _makeParseModifier("input", InputModifier::kReflectClassInfo), _makeParseModifier("out", OutModifier::kReflectClassInfo), _makeParseModifier("inout", InOutModifier::kReflectClassInfo), _makeParseModifier("__ref", RefModifier::kReflectClassInfo), _makeParseModifier("__constref", ConstRefModifier::kReflectClassInfo), _makeParseModifier("const", ConstModifier::kReflectClassInfo), - _makeParseModifier("instance", InstanceModifier::kReflectClassInfo), _makeParseModifier("__builtin", BuiltinModifier::kReflectClassInfo), _makeParseModifier("highp", GLSLPrecisionModifier::kReflectClassInfo), _makeParseModifier("lowp", GLSLPrecisionModifier::kReflectClassInfo), diff --git a/tests/diagnostics/modifier-check.slang b/tests/diagnostics/modifier-check.slang new file mode 100644 index 0000000000..e42b2ae0e2 --- /dev/null +++ b/tests/diagnostics/modifier-check.slang @@ -0,0 +1,9 @@ +//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK): + +// CHECK: ([[# @LINE+1]]): error 31201 +layout(rgba8); +Texture2D tex; + +// CHECK: ([[# @LINE+1]]): error 31202 +public internal int v() +{} diff --git a/tests/nv-extensions/nv-ray-tracing-motion-blur.slang b/tests/nv-extensions/nv-ray-tracing-motion-blur.slang index 6c3a7e8c1c..c66e69d149 100644 --- a/tests/nv-extensions/nv-ray-tracing-motion-blur.slang +++ b/tests/nv-extensions/nv-ray-tracing-motion-blur.slang @@ -20,7 +20,7 @@ struct Uniforms }; ConstantBuffer ubo; -layout(rgba8); +layout(rgba8) RWTexture2D outputImage; RaytracingAccelerationStructure as; diff --git a/tests/vkray/raygen.slang b/tests/vkray/raygen.slang index 84260611b5..9ba88095db 100644 --- a/tests/vkray/raygen.slang +++ b/tests/vkray/raygen.slang @@ -22,7 +22,7 @@ struct Uniforms ConstantBuffer ubo; -layout(rgba8); +layout(rgba8) RWTexture2D outputImage; RaytracingAccelerationStructure as;