[HLSL] Make sure global resources and resource arrays cannot be assigned to

[hekota]

Global resources are read-only. The compiler needs to report an error when somebody attempts to assign a value to a global resource, a global resource array element or the whole array.

Test update in static-local-ctor.hlsl includes the use of the llvm-cxxfilt tool which takes care of demangling of function names for a more readable test baseline.

Closes #154390

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Helena Kotas (hekota)

Changes

Global resources are read-only. The compiler needs to report an error when somebody attempts to assign a value to a global resource, a global resource array element or the whole array.

Closes #154390

---

Full diff: https://github.com/llvm/llvm-project/pull/157772.diff

6 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
• (modified) clang/include/clang/Sema/SemaHLSL.h (+2)
• (modified) clang/lib/Sema/SemaExpr.cpp (+6)
• (modified) clang/lib/Sema/SemaHLSL.cpp (+28)
• (modified) clang/test/CodeGenHLSL/static-local-ctor.hlsl (+11-8)
• (added) clang/test/SemaHLSL/prohibit_resource_edits.hlsl (+27)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 711efbe727892..591802bb294de 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -13207,6 +13207,8 @@ def note_hlsl_resource_range_here: Note<"overlapping resource range here">;
 
 def err_hlsl_incomplete_resource_array_in_function_param: Error<
   "incomplete resource array in a function parameter">;
+def err_hlsl_assign_to_global_resource: Error<
+  "assignment to global resource variable %0 is not allowed">;
 
 // Layout randomization diagnostics.
 def err_non_designated_init_used : Error<
diff --git a/clang/include/clang/Sema/SemaHLSL.h b/clang/include/clang/Sema/SemaHLSL.h
index 5cbe1b658f5cd..41101b6e7a319 100644
--- a/clang/include/clang/Sema/SemaHLSL.h
+++ b/clang/include/clang/Sema/SemaHLSL.h
@@ -131,6 +131,8 @@ class SemaHLSL : public SemaBase {
   void CheckEntryPoint(FunctionDecl *FD);
   void CheckSemanticAnnotation(FunctionDecl *EntryPoint, const Decl *Param,
                                const HLSLAnnotationAttr *AnnotationAttr);
+  bool CheckResourceBinOp(BinaryOperatorKind Opc, Expr *LHSExpr, Expr *RHSExpr,
+                          SourceLocation Loc);
   void DiagnoseAttrStageMismatch(
       const Attr *A, llvm::Triple::EnvironmentType Stage,
       std::initializer_list<llvm::Triple::EnvironmentType> AllowedStages);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 317b7caec6fb7..f50019957e420 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -15692,6 +15692,12 @@ ExprResult Sema::BuildBinOp(Scope *S, SourceLocation OpLoc,
     RHSExpr = resolvedRHS.get();
   }
 
+  if (getLangOpts().HLSL && (LHSExpr->getType()->isHLSLResourceRecord() ||
+                             LHSExpr->getType()->isHLSLResourceRecordArray())) {
+    if (!HLSL().CheckResourceBinOp(Opc, LHSExpr, RHSExpr, OpLoc))
+      return ExprError();
+  }
+
   if (getLangOpts().CPlusPlus) {
     // Otherwise, build an overloaded op if either expression is type-dependent
     // or has an overloadable type.
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index fb8f131d1e11b..f97f5e50a0a01 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -3816,6 +3816,34 @@ bool SemaHLSL::ActOnUninitializedVarDecl(VarDecl *VD) {
   return false;
 }
 
+// Return true if everything is ok; returns false if there was an error.
+bool SemaHLSL::CheckResourceBinOp(BinaryOperatorKind Opc, Expr *LHSExpr,
+                                  Expr *RHSExpr, SourceLocation Loc) {
+  assert((LHSExpr->getType()->isHLSLResourceRecord() ||
+          LHSExpr->getType()->isHLSLResourceRecordArray()) &&
+         "expected LHS to be a resource record or array of resource records");
+  if (Opc != BO_Assign)
+    return true;
+
+  // If LHS is an array subscript, get the underlying declaration.
+  Expr *E = LHSExpr;
+  while (auto *ASE = dyn_cast<ArraySubscriptExpr>(E))
+    E = ASE->getBase()->IgnoreParenImpCasts();
+
+  // Report error if LHS is a resource declared at a global scope.
+  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParens())) {
+    if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+      if (VD->hasGlobalStorage()) {
+        // assignment to global resource is not allowed
+        SemaRef.Diag(Loc, diag::err_hlsl_assign_to_global_resource) << VD;
+        SemaRef.Diag(VD->getLocation(), diag::note_var_declared_here) << VD;
+        return false;
+      }
+    }
+  }
+  return true;
+}
+
 // Walks though the global variable declaration, collects all resource binding
 // requirements and adds them to Bindings
 void SemaHLSL::collectResourceBindingsOnVarDecl(VarDecl *VD) {
diff --git a/clang/test/CodeGenHLSL/static-local-ctor.hlsl b/clang/test/CodeGenHLSL/static-local-ctor.hlsl
index 9a4bf66f030ed..0bc088164ea24 100644
--- a/clang/test/CodeGenHLSL/static-local-ctor.hlsl
+++ b/clang/test/CodeGenHLSL/static-local-ctor.hlsl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -emit-llvm -o - -disable-llvm-passes %s | FileCheck %s
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -emit-llvm -o - -disable-llvm-passes %s | llvm-cxxfilt | FileCheck %s
 
 // Verify that no per variable _Init_thread instructions are emitted for non-trivial static locals
 // These would normally be emitted by the MicrosoftCXXABI, but the DirectX backend should exlude them
@@ -7,23 +7,27 @@
 
 RWBuffer<int> buf[10];
 
+// CHECK: @[[main_mybuf:.*]] = internal global %"class.hlsl::RWBuffer" zeroinitializer, align 4
+// CHECK: @[[main_mybuf_guard:.*]] = internal global i8 0, align 1
+
 void InitBuf(RWBuffer<int> buf) {
   for (unsigned int i = 0; i < 100; i++)
     buf[i] = 0;
 }
 
 // CHECK-NOT: _Init_thread_epoch
-// CHECK: define internal void @_Z4mainv
+// CHECK: define internal void @main()
 // CHECK-NEXT: entry:
 // CHECK-NEXT: [[Tmp0:%.*]] = alloca %"class.hlsl::RWBuffer"
 // CHECK-NEXT: [[Tmp1:%.*]] = alloca %"class.hlsl::RWBuffer"
-// CHECK-NEXT: [[Tmp2:%.*]] = load i8, ptr @_ZGVZ4mainvE5mybuf
-// CHECK-NEXT: [[Tmp3:%.*]] = icmp eq i8 [[Tmp2]], 0
+// CHECK-NEXT: [[GuardVar:%.*]] = load i8, ptr @[[main_mybuf_guard]]
+// CHECK-NEXT: [[Tmp3:%.*]] = icmp eq i8 [[GuardVar]], 0
 // CHECK-NEXT: br i1 [[Tmp3]]
 // CHECK-NOT: _Init_thread_header
 // CHECK: init.check:
-// CHECK-NEXT: call void @_ZN4hlsl8RWBufferIiEC1EjijjPKc(
-// CHECK-NEXT: store i8 1, ptr @_ZGVZ4mainvE5mybuf
+// CHECK-NEXT: call void @hlsl::RWBuffer<int>::RWBuffer(unsigned int, int, unsigned int, unsigned int, char const*)(ptr {{.*}} [[Tmp0]]
+// CHECK-NEXT: call void @hlsl::RWBuffer<int>::RWBuffer(hlsl::RWBuffer<int> const&)(ptr {{.*}} @[[main_mybuf]]
+// CHECK-NEXT: store i8 1, ptr @[[main_mybuf_guard]]
 // CHECK-NOT: _Init_thread_footer
 
 
@@ -31,7 +35,6 @@ void InitBuf(RWBuffer<int> buf) {
 [numthreads(1,1,1)]
 void main() {
   // A non-trivially constructed static local will get checks to verify that it is generated just once
-  static RWBuffer<int> mybuf;
-  mybuf = buf[0];
+  static RWBuffer<int> mybuf = buf[0];
   InitBuf(mybuf);
 }
diff --git a/clang/test/SemaHLSL/prohibit_resource_edits.hlsl b/clang/test/SemaHLSL/prohibit_resource_edits.hlsl
new file mode 100644
index 0000000000000..caffa32fd8085
--- /dev/null
+++ b/clang/test/SemaHLSL/prohibit_resource_edits.hlsl
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -o - -fsyntax-only %s -verify
+
+RWBuffer<float> A[10]; // expected-note {{variable 'A' is declared here}} expected-note {{variable 'A' is declared here}} // expected-note {{variable 'A' is declared here}}
+RWBuffer<float> B;     // expected-note {{variable 'B' is declared here}}
+RWBuffer<float> C[10];
+
+void test() {
+  // expected-error@+1{{assignment to global resource variable 'A' is not allowed}}
+  A = C; 
+
+  // expected-error@+1{{assignment to global resource variable 'B' is not allowed}}
+  B = C[0];
+
+  // expected-error@+1{{assignment to global resource variable 'A' is not allowed}}
+  A[1] = B;
+
+  // expected-error@+1{{assignment to global resource variable 'A' is not allowed}}
+  A[1] = C[2];
+
+  // local resources are assignable 
+  RWBuffer<float> LocalA[10] = A; // ok
+  RWBuffer<float> LocalB = B; // ok
+
+  // read-write resources can be written to
+  A[0][0] = 1.0f; // ok
+  B[0] = 2.0f; // ok
+}

[farzonl]

should we just make this one conditional so there is no fall through case?

[hekota]

You mean merge the if statements?