From 93ac152bf8283eff1d1b2ec0f7df897a4e96464f Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Fri, 16 Mar 2018 07:01:35 -0700 Subject: Overhaul implementation of [attributes] (#443) The existing code parsed all of the square-bracket `[attributes]` into `HLSLUncheckedAttribute`, and then went on to hand-convert some of them to specialized subclasses of `HLSLAttribute`. When attributes didn't check, they were left as-is, and no error message was issued, because at the time the compiler was focused on accepting arbitrary input. This change greatly overhauls the handling of `[attributes]`. Attributes are now declared in the stdlib, with declarations like: ```hlsl __attributeTarget(LoopStmt) attribute_syntax [unroll(count: int = 0)] : UnrollAttribute; ``` In this syntax, the `unroll` part is giving the attribute name (the `[]` are just for flavor, to make the declaration look like a use site; we could drop it if we don't like the clutter), the `count` is a parameter of the attribute, which we expect to be of type `int`, and which has a default value of `0` if unspecified. The `: UnrollAttribute` part specifies the meta-level C++ class that will implement this attribute (and corresponds to a class in `modifier-defs.h`). This syntax is similar to our current `syntax` declarations. I'm starting to think we should change it to something like a `__meta_class(UnrollAttribute)` modifier, and then use that uniformly across all cases (e.g., also replacing the curreent `__magic_type(Foo)` syntax). The `__attributeTarget(LoopStmt)` is a modifier that specifies the meta-level C++ class for syntax that this attribute is allowed to attach to. It is legal to have more than one of these. Attributes continue to be parsed in an unchecked form, so that we don't tie up semantic analysis and parsing more than necessary. During checking, we look up the attribute name in the current scope, and then replace the unchecked attribute with a more specific one *if* the checking passes. Checking proceeds in generic and attribute-specific phases. The generic phase includes checking the number of arguments against those specified in the attribute declaration (I don't currently check types, or handle default arguments), and then checking that at least one `__attributeTarget(...)` modifier applies to the syntax node being modified. The attribute-specific phase then applies to the specialized C++ subclass of `Attribute`, and does the actual checking right now (e.g., that step is responsible for actually type-checking things at present). This can obviously be improved over time. With this support I went ahead and added declarations for all the HLSL attributes I could find documented on MSDN. I also added a provisional declaration for the `[shader(...)]` attribute that has been added to dxc, but which is not yet documented. One important detail here is that lookup of attribute names needs to be done carefully, so that we don't let, e.g., local variables shadow an attribute declaration: ```hlsl int unroll = 5; // This attribute should *not* get confused by the local variable `unroll` [unroll] for(...) { .. } ``` The lookup logic already has a notion of a `LookupMask` that can be used to filter declarations out of the result. In this change I surfaced that mask through the main lookup API (rather than requiring a second pass to "refine" lookup results), and made is so that the default lookup mask does *not* include attributes, while an explicit mask can be used to look up *only* attributes. (An alternatie design we discussed was to follow the approach of C# and have the declaration of an attribute like `[unroll]` actually be `unrollAttribute`, with a suffix. I decided not to follow that approach for now because it seemed like printing good error messages in that case could require us to carefully trim the `Attribute` suffix off of names at times, and using the existing mask behavior seemed simpler.) To verify that the shadowing behavior is indeed correct, I modified the `loop-unroll.slang` test case. Smaller notes: * Removed the `HLSL` prefix from several of the C++ attribute classes * Made sure to actually validate the modifiers on statements * Special-cased checking for `ParamDecl` with a null type, because I'm re-using `ParamDecl` for attribute parameters, but can't give a concrete type to some of them right now * Deleting some old, dead emit-from-AST logic around attributes, rather than try to "fix" code that doesn't run (a more complete scrub of that code is still needed) * Fixed AST inheritance hierarchy so that a `Modifier` is a `SyntaxNode` rather than a `SyntaxNodeBase`. I have *no* idea why we have both of those, and we need to clean that up soon. --- source/slang/check.cpp | 263 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 205 insertions(+), 58 deletions(-) (limited to 'source/slang/check.cpp') diff --git a/source/slang/check.cpp b/source/slang/check.cpp index f7cb3b575..1af91e9fd 100644 --- a/source/slang/check.cpp +++ b/source/slang/check.cpp @@ -1412,6 +1412,11 @@ namespace Slang // These are only used in the stdlib, so no checking is needed } + void visitAttributeDecl(AttributeDecl*) + { + // These are only used in the stdlib, so no checking is needed + } + void visitGenericTypeParamDecl(GenericTypeParamDecl*) { // These are only used in the stdlib, so no checking is needed for now @@ -1427,71 +1432,209 @@ namespace Slang // Do nothing with modifiers for now } - RefPtr checkModifier( - RefPtr m, - Decl* /*decl*/) + AttributeDecl* lookUpAttributeDecl(Name* attributeName, Scope* scope) { - if(auto hlslUncheckedAttribute = m.As()) + // Look up the name and see what we find. + // + // TODO: This needs to have some special filtering or naming + // rules to keep us from seeing shadowing variable declarations. + auto lookupResult = lookUp(getSession(), this, attributeName, scope, LookupMask::Attribute); + + // If we didn't find anything, or the result was overloaded, + // then we aren't going to be able to extract a single decl. + if(!lookupResult.isValid() || lookupResult.isOverloaded()) + return nullptr; + + auto decl = lookupResult.item.declRef.getDecl(); + if( auto attributeDecl = dynamic_cast(decl) ) { - // We have an HLSL `[name(arg,...)]` attribute, and we'd like - // to check that it is provides all the expected arguments - // - // For now we will do this in a completely ad hoc fashion, - // but it would be nice to have some generic routine to - // do the needed type checking/coercion. - auto attribText = getText(hlslUncheckedAttribute->getName()); - - if(attribText == "numthreads") - { - if(hlslUncheckedAttribute->args.Count() != 3) - return m; + return attributeDecl; + } + else + { + return nullptr; + } + } - auto xVal = checkConstantIntVal(hlslUncheckedAttribute->args[0]); - auto yVal = checkConstantIntVal(hlslUncheckedAttribute->args[1]); - auto zVal = checkConstantIntVal(hlslUncheckedAttribute->args[2]); + bool validateAttribute(RefPtr attr) + { + if(auto numThreadsAttr = attr.As()) + { + SLANG_ASSERT(attr->args.Count() == 3); + auto xVal = checkConstantIntVal(attr->args[0]); + auto yVal = checkConstantIntVal(attr->args[1]); + auto zVal = checkConstantIntVal(attr->args[2]); - if(!xVal) return m; - if(!yVal) return m; - if(!zVal) return m; + if(!xVal) return false; + if(!yVal) return false; + if(!zVal) return false; - auto hlslNumThreadsAttribute = new HLSLNumThreadsAttribute(); + numThreadsAttr->x = (int32_t) xVal->value; + numThreadsAttr->y = (int32_t) yVal->value; + numThreadsAttr->z = (int32_t) zVal->value; + } + else if (auto maxVertexCountAttr = attr.As()) + { + SLANG_ASSERT(attr->args.Count() == 1); + auto val = checkConstantIntVal(attr->args[0]); - hlslNumThreadsAttribute->loc = hlslUncheckedAttribute->loc; - hlslNumThreadsAttribute->name = hlslUncheckedAttribute->getName(); - hlslNumThreadsAttribute->args = hlslUncheckedAttribute->args; - hlslNumThreadsAttribute->x = (int32_t) xVal->value; - hlslNumThreadsAttribute->y = (int32_t) yVal->value; - hlslNumThreadsAttribute->z = (int32_t) zVal->value; + maxVertexCountAttr->value = (int32_t)val->value; + } + else if(auto instanceAttr = attr.As()) + { + SLANG_ASSERT(attr->args.Count() == 1); + auto val = checkConstantIntVal(attr->args[0]); - return hlslNumThreadsAttribute; + instanceAttr->value = (int32_t)val->value; + } + else + { + if(attr->args.Count() == 0) + { + // If the attribute took no arguments, then we will + // assume it is valid as written. + } + else + { + // We should be special-casing the checking of any attribute + // with a non-zero number of arguments. + SLANG_DIAGNOSE_UNEXPECTED(getSink(), attr, "unhandled attribute"); + return false; + } } - else if (attribText == "maxvertexcount") + + return true; + } + + RefPtr checkAttribute( + UncheckedAttribute* uncheckedAttr, + ModifiableSyntaxNode* attrTarget) + { + auto attrName = uncheckedAttr->getName(); + auto attrDecl = lookUpAttributeDecl( + attrName, + uncheckedAttr->scope); + + if(!attrDecl) + { + getSink()->diagnose(uncheckedAttr, Diagnostics::unknownAttributeName, attrName); + return uncheckedAttr; + } + + if(!attrDecl->syntaxClass.isSubClassOf()) + { + SLANG_DIAGNOSE_UNEXPECTED(getSink(), attrDecl, "attribute declaration does not reference an attribute class"); + return uncheckedAttr; + } + + RefPtr attrObj = attrDecl->syntaxClass.createInstance(); + auto attr = attrObj.As(); + if(!attr) + { + SLANG_DIAGNOSE_UNEXPECTED(getSink(), attrDecl, "attribute class did not yield an attribute object"); + return uncheckedAttr; + } + + // We are going to replace the unchecked attribute with the checked one. + + // First copy all of the state over from the original attribute. + attr->name = uncheckedAttr->name; + attr->args = uncheckedAttr->args; + attr->loc = uncheckedAttr->loc; + + // We will start with checking steps that can be applied independent + // of the concrete attribute type that was selected. These only need + // us to look at the attribute declaration itself. + // + // Start by doing argument/parameter matching + UInt argCount = attr->args.Count(); + UInt paramCounter = 0; + bool mismatch = false; + for(auto paramDecl : attrDecl->getMembersOfType()) + { + UInt paramIndex = paramCounter++; + if( paramIndex < argCount ) { - if (hlslUncheckedAttribute->args.Count() != 1) - return m; - auto val = checkConstantIntVal(hlslUncheckedAttribute->args[0]); - auto hlslMaxVertexCountAttrib = new HLSLMaxVertexCountAttribute(); + auto arg = attr->args[paramIndex]; + + // TODO: support checking the argument against the declared + // type for the parameter. - hlslMaxVertexCountAttrib->loc = hlslUncheckedAttribute->loc; - hlslMaxVertexCountAttrib->name = hlslUncheckedAttribute->getName(); - hlslMaxVertexCountAttrib->args = hlslUncheckedAttribute->args; - hlslMaxVertexCountAttrib->value = (int32_t)val->value; - return hlslMaxVertexCountAttrib; } - else if (attribText == "instance") + else { - if (hlslUncheckedAttribute->args.Count() != 1) - return m; - auto val = checkConstantIntVal(hlslUncheckedAttribute->args[0]); - auto attrib = new HLSLInstanceAttribute(); + // We didn't have enough arguments for the + // number of parameters declared. + if(auto defaultArg = paramDecl->initExpr) + { + // The attribute declaration provided a default, + // so we should use that. + // + // TODO: we need to figure out how to hook up + // default arguments as needed. + } + else + { + mismatch = true; + } + } + } + UInt paramCount = paramCounter; + + if(mismatch) + { + getSink()->diagnose(attr, Diagnostics::attributeArgumentCountMismatch, attrName, paramCount, argCount); + return uncheckedAttr; + } - attrib->loc = hlslUncheckedAttribute->loc; - attrib->name = hlslUncheckedAttribute->getName(); - attrib->args = hlslUncheckedAttribute->args; - attrib->value = (int32_t)val->value; - return attrib; + // The next bit of validation that we can apply semi-generically + // is to validate that the target for this attribute is a valid + // one for the chosen attribute. + // + // The attribute declaration will have one or more `AttributeTargetModifier`s + // that each specify a syntax class that the attribute can be applied to. + // If any of these match `attrTarget`, then we are good. + // + bool validTarget = false; + for(auto attrTargetMod : attrDecl->GetModifiersOfType()) + { + if(attrTarget->getClass().isSubClassOf(attrTargetMod->syntaxClass)) + { + validTarget = true; + break; } } + if(!validTarget) + { + getSink()->diagnose(attr, Diagnostics::attributeNotApplicable, attrName); + return uncheckedAttr; + } + + // Now apply type-specific validation to the attribute. + if(!validateAttribute(attr)) + { + return uncheckedAttr; + } + + + return attr; + } + + RefPtr checkModifier( + RefPtr m, + ModifiableSyntaxNode* syntaxNode) + { + if(auto hlslUncheckedAttribute = m.As()) + { + // We have an HLSL `[name(arg,...)]` attribute, and we'd like + // to check that it is provides all the expected arguments + // + // First, look up the attribute name in the current scope to find + // the right syntax class to instantiate. + // + + return checkAttribute(hlslUncheckedAttribute, syntaxNode); + } // Default behavior is to leave things as they are, // and assume that modifiers are mostly already checked. // @@ -1505,7 +1648,7 @@ namespace Slang } - void checkModifiers(Decl* decl) + void checkModifiers(ModifiableSyntaxNode* syntaxNode) { // TODO(tfoley): need to make sure this only // performs semantic checks on a `SharedModifier` once... @@ -1516,7 +1659,7 @@ namespace Slang RefPtr resultModifiers; RefPtr* resultModifierLink = &resultModifiers; - RefPtr modifier = decl->modifiers.first; + RefPtr modifier = syntaxNode->modifiers.first; while(modifier) { // Because we are rewriting the list in place, we need to extract @@ -1528,7 +1671,7 @@ namespace Slang // be to return a single unlinked modifier. modifier->next = nullptr; - auto checkedModifier = checkModifier(modifier, decl); + auto checkedModifier = checkModifier(modifier, syntaxNode); if(checkedModifier) { // If checking gave us a modifier to add, then we @@ -1552,7 +1695,7 @@ namespace Slang // Whether we actually re-wrote anything or note, lets // install the new list of modifiers on the declaration - decl->modifiers.first = resultModifiers; + syntaxNode->modifiers.first = resultModifiers; } void visitModuleDecl(ModuleDecl* programNode) @@ -2182,6 +2325,7 @@ namespace Slang { if (!stmt) return; StmtVisitor::dispatch(stmt); + checkModifiers(stmt); } void visitFuncDecl(FuncDecl *functionNode) @@ -2610,11 +2754,14 @@ namespace Slang { // TODO: This needs to bottleneck through the common variable checks - para->type = CheckUsableType(para->type); - - if (para->type.Equals(getSession()->getVoidType())) + if(para->type.exp) { - getSink()->diagnose(para, Diagnostics::parameterCannotBeVoid); + para->type = CheckUsableType(para->type); + + if (para->type.Equals(getSession()->getVoidType())) + { + getSink()->diagnose(para, Diagnostics::parameterCannotBeVoid); + } } } -- cgit v1.2.3