Skip to content

Commit

Permalink
Add check for invalid use of modifiers. (#3402)
Browse files Browse the repository at this point in the history 
* Add check for invalid use of modifiers.

* Fixes.

* Add test.

---------

Co-authored-by: Yong He <yhe@nvidia.com>
  • Loading branch information
@csyonghe
csyonghe and Yong He authored Dec 12, 2023
1 parent ec0224e commit 92f21de
Show file tree
Hide file tree
Showing 10 changed files with 232 additions and 18 deletions.
3 changes: 3 additions & 0 deletions source/compiler-core/slang-diagnostic-sink.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<typename T>
void printDiagnosticArg(StringBuilder& sb, RefPtr<T> ptr)
Expand Down
14 changes: 4 additions & 10 deletions source/slang/hlsl.meta.slang
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10599,9 +10599,7 @@ struct HitObject
case glsl:
{
// Save the attributes
__ref attr_t attr = __hitObjectAttributes<attr_t>();

attr = attributes;
__hitObjectAttributes<attr_t>() = attributes;

__glslMakeHit(
__return_val,
Expand Down Expand Up @@ -10670,9 +10668,7 @@ struct HitObject
case glsl:
{
// Save the attributes
__ref attr_t attr = __hitObjectAttributes<attr_t>();

attr = attributes;
__hitObjectAttributes<attr_t>() = attributes;

__glslMakeMotionHit(
__return_val,
Expand Down Expand Up @@ -10760,8 +10756,7 @@ struct HitObject
case glsl:
{
// Save the attributes
__ref attr_t attr = __hitObjectAttributes<attr_t>();
attr = attributes;
__hitObjectAttributes<attr_t>() = attributes;

__glslMakeHitWithIndex(
__return_val,
Expand Down Expand Up @@ -10824,8 +10819,7 @@ struct HitObject
case glsl:
{
// Save the attributes
__ref attr_t attr = __hitObjectAttributes<attr_t>();
attr = attributes;
__hitObjectAttributes<attr_t>() = attributes;

__glslMakeMotionHitWithIndex(
__return_val,
Expand Down
12 changes: 12 additions & 0 deletions source/slang/slang-ast-modifier.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,16 @@ const OrderedDictionary<DeclRefBase*, SubtypeWitness*>& 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<HLSLSemantic>(modifier))
sb << hlslSemantic->name.getContent();
return;
}

} // namespace Slang
2 changes: 0 additions & 2 deletions source/slang/slang-ast-modifier.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)};
Expand All @@ -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)};
Expand Down
201 changes: 200 additions & 1 deletion source/slang/slang-check-modifier.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<VarDeclBase>(decl) && isGlobalDecl(decl)) || as<ParamDecl>(decl) || as<GLSLInterfaceBlockDecl>(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<VarDeclBase>(decl) && (isGlobalDecl(decl) || as<StructDecl>(getParentDecl(decl)))) || as<ParamDecl>(decl);

case ASTNodeType::HLSLSemantic:
case ASTNodeType::HLSLLayoutSemantic:
case ASTNodeType::HLSLRegisterSemantic:
case ASTNodeType::HLSLPackOffsetSemantic:
case ASTNodeType::HLSLSimpleSemantic:
return (as<VarDeclBase>(decl) && (isGlobalDecl(decl) || as<StructDecl>(getParentDecl(decl)))) || as<ParamDecl>(decl) || as<FuncDecl>(decl);

// Allowed only on functions
case ASTNodeType::IntrinsicOpModifier:
case ASTNodeType::SpecializedForTargetModifier:
case ASTNodeType::InlineModifier:
case ASTNodeType::PrefixModifier:
case ASTNodeType::PostfixModifier:
return as<CallableDecl>(decl);

case ASTNodeType::BuiltinModifier:
case ASTNodeType::PublicModifier:
case ASTNodeType::PrivateModifier:
case ASTNodeType::InternalModifier:
case ASTNodeType::ExternModifier:
case ASTNodeType::HLSLExportModifier:
case ASTNodeType::ExternCppModifier:
return as<VarDeclBase>(decl) || as<AggTypeDeclBase>(decl) || as<NamespaceDeclBase>(decl) || as<CallableDecl>(decl)
|| as<TypeDefDecl>(decl) || as<PropertyDecl>(decl) || as<SyntaxDecl>(decl) || as<AttributeDecl>(decl);

case ASTNodeType::ExportedModifier:
return as<ImportDecl>(decl);

case ASTNodeType::ConstModifier:
case ASTNodeType::HLSLStaticModifier:
case ASTNodeType::ConstExprModifier:
case ASTNodeType::PreciseModifier:
return as<VarDeclBase>(decl) || as<CallableDecl>(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<VarDeclBase>(decl) || as<GLSLInterfaceBlockDecl>(decl);

case ASTNodeType::GLSLPrecisionModifier:
return as<VarDeclBase>(decl) || as<GLSLInterfaceBlockDecl>(decl) || as<CallableDecl>(decl);
default:
return true;
}
}

Modifier* SemanticsVisitor::checkModifier(
Modifier* m,
ModifiableSyntaxNode* syntaxNode)
Expand All @@ -935,6 +1108,15 @@ namespace Slang
return checkAttribute(hlslUncheckedAttribute, syntaxNode);
}

if (auto decl = as<Decl>(syntaxNode))
{
if (!isModifierAllowedOnDecl(m->astNodeType, decl))
{
getSink()->diagnose(m, Diagnostics::modifierNotAllowed, m);
return m;
}
}

if (auto hlslSemantic = as<HLSLSimpleSemantic>(m))
{
if (hlslSemantic->name.getName() == getSession()->getCompletionRequestTokenName())
Expand Down Expand Up @@ -1164,9 +1346,25 @@ namespace Slang
Modifier* resultModifiers = nullptr;
Modifier** resultModifierLink = &resultModifiers;

// We will keep track of the modifiers for each conflict group.
Dictionary<ASTNodeType, Modifier*> 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;
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion source/slang/slang-diagnostic-defs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'")
Expand Down
2 changes: 0 additions & 2 deletions source/slang/slang-parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
9 changes: 9 additions & 0 deletions tests/diagnostics/modifier-check.slang
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK):

// CHECK: ([[# @LINE+1]]): error 31201
layout(rgba8);
Texture2D<float4> tex;

// CHECK: ([[# @LINE+1]]): error 31202
public internal int v()
{}
2 changes: 1 addition & 1 deletion tests/nv-extensions/nv-ray-tracing-motion-blur.slang
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ struct Uniforms
};
ConstantBuffer<Uniforms> ubo;

layout(rgba8);
layout(rgba8)
RWTexture2D<float4> outputImage;

RaytracingAccelerationStructure as;
Expand Down
2 changes: 1 addition & 1 deletion tests/vkray/raygen.slang
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ struct Uniforms
ConstantBuffer<Uniforms> ubo;


layout(rgba8);
layout(rgba8)
RWTexture2D<float4> outputImage;

RaytracingAccelerationStructure as;
Expand Down

0 comments on commit 92f21de

Please sign in to comment.