Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HLSL] error on out of bounds vector accesses #128952

Merged
merged 9 commits into from
Mar 11, 2025
Merged

Conversation

spall
Copy link
Contributor

@spall spall commented Feb 26, 2025

Add Sema checking and diagnostics to error on out of bounds vector accesses
Add tests
Closes #91640

@spall spall marked this pull request as ready for review February 27, 2025 00:00
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Feb 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2025

@llvm/pr-subscribers-hlsl

Author: Sarah Spall (spall)

Changes

Add Sema checking and diagnostics to error on out of bounds vector accesses
Warns if language is not HLSL, adds new warning flag -Wvector-out-of-range
Add tests
Closes #91640


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

5 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6)
  • (modified) clang/include/clang/Sema/Sema.h (+1)
  • (modified) clang/lib/Sema/SemaChecking.cpp (+24)
  • (added) clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl (+19)
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 05e39899e6f25..d36f6086edd40 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -930,6 +930,7 @@ def SuperSubClassMismatch : DiagGroup<"super-class-method-mismatch">;
 def OverridingMethodMismatch : DiagGroup<"overriding-method-mismatch">;
 def VariadicMacros : DiagGroup<"variadic-macros">;
 def VectorConversion : DiagGroup<"vector-conversion">;      // clang specific
+def VectorOutOfRange : DiagGroup<"vector-out-of-range">;
 def VexingParse : DiagGroup<"vexing-parse">;
 def VLAUseStaticAssert : DiagGroup<"vla-extension-static-assert">;
 def VLACxxExtension : DiagGroup<"vla-cxx-extension", [VLAUseStaticAssert]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 51301d95e55b9..7ff37406b459a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10635,6 +10635,12 @@ def err_block_on_vm : Error<
 def err_sizeless_nonlocal : Error<
   "non-local variable with sizeless type %0">;
 
+def err_vector_index_out_of_range : Error<
+  "vector element index %0 is out of bounds">;
+def warn_vector_index_out_of_range : Warning<
+  "vector element index %0 is out of bounds">,
+  InGroup<VectorOutOfRange>, DefaultIgnore;
+
 def err_vec_builtin_non_vector : Error<
  "%select{first two|all}1 arguments to %0 must be vectors">;
 def err_vec_builtin_incompatible_vector : Error<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 476abe86cb2d2..fad3cbddc555b 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2468,6 +2468,7 @@ class Sema final : public SemaBase {
                         const ArraySubscriptExpr *ASE = nullptr,
                         bool AllowOnePastEnd = true, bool IndexNegated = false);
   void CheckArrayAccess(const Expr *E);
+  void CheckVectorAccess(const Expr *BaseExpr, const Expr *IndexExpr);
 
   bool CheckPointerCall(NamedDecl *NDecl, CallExpr *TheCall,
                         const FunctionProtoType *Proto);
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index f9926c6b4adab..1cbf428f18366 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -14017,6 +14017,24 @@ void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
     << TRange << Op->getSourceRange();
 }
 
+void Sema::CheckVectorAccess(const Expr *BaseExpr, const Expr *IndexExpr) {
+  const VectorType *VTy = BaseExpr->getType()->getAs<VectorType>();
+  if (!VTy)
+    return;
+
+  Expr::EvalResult Result;
+  if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects))
+    return;
+
+  unsigned DiagID = getLangOpts().HLSL ? diag::err_vector_index_out_of_range
+                                       : diag::warn_vector_index_out_of_range;
+
+  llvm::APSInt index = Result.Val.getInt();
+  if (index.isNegative() || index >= VTy->getNumElements())
+    Diag(BaseExpr->getBeginLoc(), DiagID) << toString(index, 10, true);
+  return;
+}
+
 void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
                             const ArraySubscriptExpr *ASE,
                             bool AllowOnePastEnd, bool IndexNegated) {
@@ -14031,6 +14049,12 @@ void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
   const Type *EffectiveType =
       BaseExpr->getType()->getPointeeOrArrayElementType();
   BaseExpr = BaseExpr->IgnoreParenCasts();
+
+  if (BaseExpr->getType()->isVectorType()) {
+    CheckVectorAccess(BaseExpr, IndexExpr);
+    return;
+  }
+
   const ConstantArrayType *ArrayTy =
       Context.getAsConstantArrayType(BaseExpr->getType());
 
diff --git a/clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl b/clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl
new file mode 100644
index 0000000000000..b4d423fb24bd2
--- /dev/null
+++ b/clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.6-library %s -verify
+
+export void fn1() {
+  int2 A = {1,2};
+  int X = A[-1];
+  // expected-error@-1 {{vector element index -1 is out of bounds}}
+}
+
+export void fn2() {
+  int4 A = {1,2,3,4};
+  int X = A[4];
+  // expected-error@-1 {{vector element index 4 is out of bounds}}
+}
+
+export void fn3() {
+  bool2 A = {true,true};
+  bool X = A[-1];
+  // expected-error@-1 {{vector element index -1 is out of bounds}}
+}

@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2025

@llvm/pr-subscribers-clang

Author: Sarah Spall (spall)

Changes

Add Sema checking and diagnostics to error on out of bounds vector accesses
Warns if language is not HLSL, adds new warning flag -Wvector-out-of-range
Add tests
Closes #91640


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

5 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6)
  • (modified) clang/include/clang/Sema/Sema.h (+1)
  • (modified) clang/lib/Sema/SemaChecking.cpp (+24)
  • (added) clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl (+19)
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 05e39899e6f25..d36f6086edd40 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -930,6 +930,7 @@ def SuperSubClassMismatch : DiagGroup<"super-class-method-mismatch">;
 def OverridingMethodMismatch : DiagGroup<"overriding-method-mismatch">;
 def VariadicMacros : DiagGroup<"variadic-macros">;
 def VectorConversion : DiagGroup<"vector-conversion">;      // clang specific
+def VectorOutOfRange : DiagGroup<"vector-out-of-range">;
 def VexingParse : DiagGroup<"vexing-parse">;
 def VLAUseStaticAssert : DiagGroup<"vla-extension-static-assert">;
 def VLACxxExtension : DiagGroup<"vla-cxx-extension", [VLAUseStaticAssert]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 51301d95e55b9..7ff37406b459a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10635,6 +10635,12 @@ def err_block_on_vm : Error<
 def err_sizeless_nonlocal : Error<
   "non-local variable with sizeless type %0">;
 
+def err_vector_index_out_of_range : Error<
+  "vector element index %0 is out of bounds">;
+def warn_vector_index_out_of_range : Warning<
+  "vector element index %0 is out of bounds">,
+  InGroup<VectorOutOfRange>, DefaultIgnore;
+
 def err_vec_builtin_non_vector : Error<
  "%select{first two|all}1 arguments to %0 must be vectors">;
 def err_vec_builtin_incompatible_vector : Error<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 476abe86cb2d2..fad3cbddc555b 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2468,6 +2468,7 @@ class Sema final : public SemaBase {
                         const ArraySubscriptExpr *ASE = nullptr,
                         bool AllowOnePastEnd = true, bool IndexNegated = false);
   void CheckArrayAccess(const Expr *E);
+  void CheckVectorAccess(const Expr *BaseExpr, const Expr *IndexExpr);
 
   bool CheckPointerCall(NamedDecl *NDecl, CallExpr *TheCall,
                         const FunctionProtoType *Proto);
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index f9926c6b4adab..1cbf428f18366 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -14017,6 +14017,24 @@ void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
     << TRange << Op->getSourceRange();
 }
 
+void Sema::CheckVectorAccess(const Expr *BaseExpr, const Expr *IndexExpr) {
+  const VectorType *VTy = BaseExpr->getType()->getAs<VectorType>();
+  if (!VTy)
+    return;
+
+  Expr::EvalResult Result;
+  if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects))
+    return;
+
+  unsigned DiagID = getLangOpts().HLSL ? diag::err_vector_index_out_of_range
+                                       : diag::warn_vector_index_out_of_range;
+
+  llvm::APSInt index = Result.Val.getInt();
+  if (index.isNegative() || index >= VTy->getNumElements())
+    Diag(BaseExpr->getBeginLoc(), DiagID) << toString(index, 10, true);
+  return;
+}
+
 void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
                             const ArraySubscriptExpr *ASE,
                             bool AllowOnePastEnd, bool IndexNegated) {
@@ -14031,6 +14049,12 @@ void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
   const Type *EffectiveType =
       BaseExpr->getType()->getPointeeOrArrayElementType();
   BaseExpr = BaseExpr->IgnoreParenCasts();
+
+  if (BaseExpr->getType()->isVectorType()) {
+    CheckVectorAccess(BaseExpr, IndexExpr);
+    return;
+  }
+
   const ConstantArrayType *ArrayTy =
       Context.getAsConstantArrayType(BaseExpr->getType());
 
diff --git a/clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl b/clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl
new file mode 100644
index 0000000000000..b4d423fb24bd2
--- /dev/null
+++ b/clang/test/SemaHLSL/Language/VectorOutOfRange-errors.hlsl
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.6-library %s -verify
+
+export void fn1() {
+  int2 A = {1,2};
+  int X = A[-1];
+  // expected-error@-1 {{vector element index -1 is out of bounds}}
+}
+
+export void fn2() {
+  int4 A = {1,2,3,4};
+  int X = A[4];
+  // expected-error@-1 {{vector element index 4 is out of bounds}}
+}
+
+export void fn3() {
+  bool2 A = {true,true};
+  bool X = A[-1];
+  // expected-error@-1 {{vector element index -1 is out of bounds}}
+}

Copy link
Member

@hekota hekota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a release note to clang/docs/ReleaseNotes.rst so users know about the improvement.

@spall
Copy link
Contributor Author

spall commented Mar 10, 2025

The latest commits make it an error for all, add a release note, and modify a test which had an out of bounds vector access.

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion of different wording on the release note, but otherwise LGTM.

Copy link
Member

@hekota hekota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@spall spall merged commit f1e3675 into llvm:main Mar 11, 2025
12 checks passed
@zmodem
Copy link
Collaborator

zmodem commented Mar 12, 2025

This broke our builds. Here's a small repro:

$ cat /tmp/a.cc
#include <immintrin.h>
__m256i parens;
int f() {
  return ((__v32qi)parens)[31];
}
$ build/bin/clang -c /tmp/a.cc
/tmp/a.cc:4:20: error: vector element index 31 is out of bounds
    4 |   return ((__v32qi)parens)[31];
      |                    ^
1 error generated.

__v32qi is a 32-element vector of char.

I'll revert to green for now.

zmodem added a commit that referenced this pull request Mar 12, 2025
This caused false-positive errors, see comment on the PR.

> Add Sema checking and diagnostics to error on out of bounds vector
> accesses
> Add tests
> Closes #91640

This reverts commit f1e3675.
return;

Expr::EvalResult Result;
if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, if a value is not required to be a constant, we don't want different semantic rules depending on whether the value is in fact constant. (There are a few places that do in fact work like this, like array bounds, but it causes weird results. Especially when people do stuff with macros.) So I don't think we want to reject here.

A DiagRuntimeBehavior() is probably appropriate, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category HLSL HLSL Language Support
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[HLSL] vector subscript indexing
10 participants