From cb2be16144cf7104fbcced63320d5bafd561fc3c Mon Sep 17 00:00:00 2001 From: Tim Corringham Date: Mon, 13 May 2024 19:29:53 +0100 Subject: [PATCH 1/2] Amend template specialization DXASSERT conditions Clang suppresses template specialization if a fatal error has been reported in order to reduce the risk of a cascade of secondary error diagnostics. However, DXC DXASSERTs if template specialization fails - even if that is due to an unrelated fatal error - which has the unintended result of hiding the fatal error and hence providing no indication of what the problem is. The DXASSERT conditions have been amended so they are no longer raised if a fatal error has been registered. --- tools/clang/lib/Sema/SemaHLSL.cpp | 34 ++++++++++++------- .../DXC/specialization_with_fatal_error.hlsl | 22 ++++++++++++ 2 files changed, 43 insertions(+), 13 deletions(-) create mode 100644 tools/clang/test/DXC/specialization_with_fatal_error.hlsl diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp index 6e58c0e872..92969a653b 100644 --- a/tools/clang/lib/Sema/SemaHLSL.cpp +++ b/tools/clang/lib/Sema/SemaHLSL.cpp @@ -850,11 +850,12 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema, if (specializationDecl->getInstantiatedFrom().isNull()) { // InstantiateClassTemplateSpecialization returns true if it finds an // error. - DXVERIFY_NOMSG(false == - sema.InstantiateClassTemplateSpecialization( - NoLoc, specializationDecl, - TemplateSpecializationKind::TSK_ImplicitInstantiation, - true)); + bool errorFound = sema.InstantiateClassTemplateSpecialization( + NoLoc, specializationDecl, + TemplateSpecializationKind::TSK_ImplicitInstantiation, true); + // Template specialization is suppressed if a fatal error has already been + // registered so don't assert in such cases. + DXVERIFY_NOMSG(sema.Diags.hasFatalErrorOccurred() || !errorFound); } return context.getTemplateSpecializationType( TemplateName(templateDecl), templateArgs.data(), templateArgs.size(), @@ -866,11 +867,12 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema, templateDecl, templateArgsForDecl.data(), templateArgsForDecl.size(), nullptr); // InstantiateClassTemplateSpecialization returns true if it finds an error. - DXVERIFY_NOMSG(false == - sema.InstantiateClassTemplateSpecialization( - NoLoc, specializationDecl, - TemplateSpecializationKind::TSK_ImplicitInstantiation, - true)); + bool errorFound = sema.InstantiateClassTemplateSpecialization( + NoLoc, specializationDecl, + TemplateSpecializationKind::TSK_ImplicitInstantiation, true); + // Template specialization is suppressed if a fatal error has already been + // registered so don't assert in such cases. + DXVERIFY_NOMSG(sema.Diags.hasFatalErrorOccurred() || !errorFound); templateDecl->AddSpecialization(specializationDecl, InsertPos); specializationDecl->setImplicit(true); @@ -918,7 +920,9 @@ static QualType GetOrCreateMatrixSpecialization( DeclContext::lookup_result lookupResult = matrixSpecializationType->getAsCXXRecordDecl()->lookup( DeclarationName(&context.Idents.get(StringRef("h")))); - DXASSERT(!lookupResult.empty(), + // Template specialization is suppressed if a fatal error has been registered + // so only assert if lookup failed for some other reason. + DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(), "otherwise matrix handle cannot be looked up"); #endif @@ -953,7 +957,9 @@ GetOrCreateVectorSpecialization(ASTContext &context, Sema *sema, DeclContext::lookup_result lookupResult = vectorSpecializationType->getAsCXXRecordDecl()->lookup( DeclarationName(&context.Idents.get(StringRef("h")))); - DXASSERT(!lookupResult.empty(), + // Template specialization is suppressed if a fatal error has been registered + // so only assert if lookup failed for some other reason. + DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(), "otherwise vector handle cannot be looked up"); #endif @@ -981,7 +987,9 @@ GetOrCreateNodeOutputRecordSpecialization(ASTContext &context, Sema *sema, DeclContext::lookup_result lookupResult = specializationType->getAsCXXRecordDecl()->lookup( DeclarationName(&context.Idents.get(StringRef("h")))); - DXASSERT(!lookupResult.empty(), + // Template specialization is suppressed if a fatal error has been registered + // so only assert if lookup failed for some other reason. + DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(), "otherwise *NodeOutputRecords handle cannot be looked up"); #endif diff --git a/tools/clang/test/DXC/specialization_with_fatal_error.hlsl b/tools/clang/test/DXC/specialization_with_fatal_error.hlsl new file mode 100644 index 0000000000..bab0907977 --- /dev/null +++ b/tools/clang/test/DXC/specialization_with_fatal_error.hlsl @@ -0,0 +1,22 @@ +// RUN: %dxc -T lib_6_8 -verify %s + +// Clang suppresses template specialization if a fatal error has been +// registered (this reduces the risk of a cascade of secondary errors). +// However, DXC DXASSERTs if a template specialization fails - which +// prevents the error diagnostic being generated. +// We check here that a DXASSERT is no longer raised if a fatal error +// has been registered, and that the error diagnostic is generated. + +float a; + +// the include file doesn't exist - this should produce a fatal error diagnostic +// expected-error@+1 {{'a.h' file not found}} +#include "a.h" + +void b() {}; + +int3 c(int X) { + // DXASSERT was triggered if include file a.h doesn't exist, and the error + // diagnostic was not produced. + return X.xxx; +} From 9ea306ff87efff0517b744ebc06de4ae675303ab Mon Sep 17 00:00:00 2001 From: Tim Corringham Date: Mon, 19 Aug 2024 14:19:53 +0100 Subject: [PATCH 2/2] Update template specialization assert handling Amend the fix for DXASSERTS in template specialization by incorporating changes from review comments. --- tools/clang/lib/Sema/SemaHLSL.cpp | 87 +++++++++---------- .../DXC/specialization_with_fatal_error.hlsl | 12 +-- 2 files changed, 47 insertions(+), 52 deletions(-) diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp index 92969a653b..16fbb9b5d9 100644 --- a/tools/clang/lib/Sema/SemaHLSL.cpp +++ b/tools/clang/lib/Sema/SemaHLSL.cpp @@ -850,12 +850,10 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema, if (specializationDecl->getInstantiatedFrom().isNull()) { // InstantiateClassTemplateSpecialization returns true if it finds an // error. - bool errorFound = sema.InstantiateClassTemplateSpecialization( - NoLoc, specializationDecl, - TemplateSpecializationKind::TSK_ImplicitInstantiation, true); - // Template specialization is suppressed if a fatal error has already been - // registered so don't assert in such cases. - DXVERIFY_NOMSG(sema.Diags.hasFatalErrorOccurred() || !errorFound); + if (sema.InstantiateClassTemplateSpecialization( + NoLoc, specializationDecl, + TemplateSpecializationKind::TSK_ImplicitInstantiation, true)) + return QualType(); } return context.getTemplateSpecializationType( TemplateName(templateDecl), templateArgs.data(), templateArgs.size(), @@ -866,19 +864,20 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema, context, TagDecl::TagKind::TTK_Class, currentDeclContext, NoLoc, NoLoc, templateDecl, templateArgsForDecl.data(), templateArgsForDecl.size(), nullptr); - // InstantiateClassTemplateSpecialization returns true if it finds an error. - bool errorFound = sema.InstantiateClassTemplateSpecialization( - NoLoc, specializationDecl, - TemplateSpecializationKind::TSK_ImplicitInstantiation, true); - // Template specialization is suppressed if a fatal error has already been - // registered so don't assert in such cases. - DXVERIFY_NOMSG(sema.Diags.hasFatalErrorOccurred() || !errorFound); + // template specialization isn't performed if a fatal error has occurred + if (!sema.Diags.hasFatalErrorOccurred()) { + // InstantiateClassTemplateSpecialization returns true if it finds an error. + [[maybe_unused]] bool errorFound = + sema.InstantiateClassTemplateSpecialization( + NoLoc, specializationDecl, + TemplateSpecializationKind::TSK_ImplicitInstantiation, true); + assert(!errorFound && "template specialization failed"); + } templateDecl->AddSpecialization(specializationDecl, InsertPos); specializationDecl->setImplicit(true); - QualType canonType = context.getTypeDeclType(specializationDecl); - DXASSERT(isa(canonType), - "type of non-dependent specialization is not a RecordType"); + assert(isa(canonType) && + "type of non-dependent specialization is not a RecordType"); TemplateArgumentListInfo templateArgumentList(NoLoc, NoLoc); TemplateArgumentLocInfo NoTemplateArgumentLocInfo; for (unsigned i = 0; i < templateArgs.size(); i++) { @@ -913,18 +912,17 @@ static QualType GetOrCreateMatrixSpecialization( context, *sema, matrixTemplateDecl, ArrayRef(templateArgs)); -#ifndef NDEBUG - // Verify that we can read the field member from the template record. - DXASSERT(matrixSpecializationType->getAsCXXRecordDecl(), + if (!matrixSpecializationType.isNull() && + !sema->Diags.hasFatalErrorOccurred()) { + assert(matrixSpecializationType->getAsCXXRecordDecl() && "type of non-dependent specialization is not a RecordType"); - DeclContext::lookup_result lookupResult = - matrixSpecializationType->getAsCXXRecordDecl()->lookup( - DeclarationName(&context.Idents.get(StringRef("h")))); - // Template specialization is suppressed if a fatal error has been registered - // so only assert if lookup failed for some other reason. - DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(), + // Verify that we can read the field member from the template record. + [[maybe_unused]] DeclContext::lookup_result lookupResult = + matrixSpecializationType->getAsCXXRecordDecl()->lookup( + DeclarationName(&context.Idents.get(StringRef("h")))); + assert(!lookupResult.empty() && "otherwise matrix handle cannot be looked up"); -#endif + } return matrixSpecializationType; } @@ -950,18 +948,17 @@ GetOrCreateVectorSpecialization(ASTContext &context, Sema *sema, context, *sema, vectorTemplateDecl, ArrayRef(templateArgs)); -#ifndef NDEBUG - // Verify that we can read the field member from the template record. - DXASSERT(vectorSpecializationType->getAsCXXRecordDecl(), + if (!vectorSpecializationType.isNull() && + !sema->Diags.hasFatalErrorOccurred()) { + assert(vectorSpecializationType->getAsCXXRecordDecl() && "type of non-dependent specialization is not a RecordType"); - DeclContext::lookup_result lookupResult = - vectorSpecializationType->getAsCXXRecordDecl()->lookup( - DeclarationName(&context.Idents.get(StringRef("h")))); - // Template specialization is suppressed if a fatal error has been registered - // so only assert if lookup failed for some other reason. - DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(), + // Verify that we can read the field member from the template record. + [[maybe_unused]] DeclContext::lookup_result lookupResult = + vectorSpecializationType->getAsCXXRecordDecl()->lookup( + DeclarationName(&context.Idents.get(StringRef("h")))); + assert(!lookupResult.empty() && "otherwise vector handle cannot be looked up"); -#endif + } return vectorSpecializationType; } @@ -980,18 +977,16 @@ GetOrCreateNodeOutputRecordSpecialization(ASTContext &context, Sema *sema, QualType specializationType = GetOrCreateTemplateSpecialization( context, *sema, templateDecl, ArrayRef(templateArgs)); -#ifdef DBG - // Verify that we can read the field member from the template record. - DXASSERT(specializationType->getAsCXXRecordDecl(), + if (!specializationType.isNull() && !sema->Diags.hasFatalErrorOccurred()) { + assert(specializationType->getAsCXXRecordDecl() && "type of non-dependent specialization is not a RecordType"); - DeclContext::lookup_result lookupResult = - specializationType->getAsCXXRecordDecl()->lookup( - DeclarationName(&context.Idents.get(StringRef("h")))); - // Template specialization is suppressed if a fatal error has been registered - // so only assert if lookup failed for some other reason. - DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(), + // Verify that we can read the field member from the template record. + [[maybe_unused]] DeclContext::lookup_result lookupResult = + specializationType->getAsCXXRecordDecl()->lookup( + DeclarationName(&context.Idents.get(StringRef("h")))); + assert(!lookupResult.empty() && "otherwise *NodeOutputRecords handle cannot be looked up"); -#endif + } return specializationType; } diff --git a/tools/clang/test/DXC/specialization_with_fatal_error.hlsl b/tools/clang/test/DXC/specialization_with_fatal_error.hlsl index bab0907977..c5e20d9bb7 100644 --- a/tools/clang/test/DXC/specialization_with_fatal_error.hlsl +++ b/tools/clang/test/DXC/specialization_with_fatal_error.hlsl @@ -2,9 +2,9 @@ // Clang suppresses template specialization if a fatal error has been // registered (this reduces the risk of a cascade of secondary errors). -// However, DXC DXASSERTs if a template specialization fails - which -// prevents the error diagnostic being generated. -// We check here that a DXASSERT is no longer raised if a fatal error +// However, DXC asserted if a template specialization failed - which +// prevented the error diagnostic being generated. +// We check here that an assert is no longer raised if a fatal error // has been registered, and that the error diagnostic is generated. float a; @@ -16,7 +16,7 @@ float a; void b() {}; int3 c(int X) { - // DXASSERT was triggered if include file a.h doesn't exist, and the error - // diagnostic was not produced. - return X.xxx; + // an assert was triggered for the expression below when include file a.h + // doesn't exist, and the error diagnostic expected above was not produced. + return X.xxx; }