-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[clang] Report Diagnostic when builtin vector has negative size #166055
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
[clang] Report Diagnostic when builtin vector has negative size #166055
Conversation
|
@llvm/pr-subscribers-clang Author: Amr Hesham (AmrDeveloper) ChangesReport a diagnostic in case vector_size or ext_vector_type attributes are used with a negative size. The same evaluation result can be used for other checks, for example, the too big a size. Issue #165463 Full diff: https://github.com/llvm/llvm-project/pull/166055.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 92fc9381a5868..f3a8367ad16e5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -390,6 +390,9 @@ Improvements to Clang's diagnostics
that were previously incorrectly accepted in case of other irrelevant
conditions are now consistently diagnosed, identical to C++ mode.
+- Clang now emits a diagnostic in case `vector_size` or `ext_vector_type`
+ attributes are used with a negative size (#GH165463).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 4e369be0bbb92..fa509536bf021 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3510,6 +3510,8 @@ def err_init_method_bad_return_type : Error<
"init methods must return an object pointer type, not %0">;
def err_attribute_invalid_size : Error<
"vector size not an integral multiple of component size">;
+def err_attribute_vec_negative_size
+ : Error<"vector must have non-negative size">;
def err_attribute_zero_size : Error<"zero %0 size">;
def err_attribute_size_too_large : Error<"%0 size too large">;
def err_typecheck_sve_rvv_ambiguous : Error<
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 280b3c92cce14..3345ed0dbbc0f 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8285,10 +8285,20 @@ static void HandleVectorSizeAttr(QualType &CurType, const ParsedAttr &Attr,
Expr *SizeExpr = Attr.getArgAsExpr(0);
QualType T = S.BuildVectorType(CurType, SizeExpr, Attr.getLoc());
- if (!T.isNull())
- CurType = T;
- else
+ if (T.isNull()) {
+ Attr.setInvalid();
+ return;
+ }
+
+ std::optional<llvm::APSInt> VecSize =
+ SizeExpr->getIntegerConstantExpr(S.Context);
+ if (VecSize && VecSize->isNegative()) {
+ S.Diag(SizeExpr->getExprLoc(), diag::err_attribute_vec_negative_size);
Attr.setInvalid();
+ return;
+ }
+
+ CurType = T;
}
/// Process the OpenCL-like ext_vector_type attribute when it occurs on
@@ -8306,6 +8316,14 @@ static void HandleExtVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
QualType T = S.BuildExtVectorType(CurType, SizeExpr, Attr.getLoc());
if (!T.isNull())
CurType = T;
+
+ std::optional<llvm::APSInt> VecSize =
+ SizeExpr->getIntegerConstantExpr(S.Context);
+ if (VecSize && VecSize->isNegative()) {
+ S.Diag(SizeExpr->getExprLoc(), diag::err_attribute_vec_negative_size);
+ Attr.setInvalid();
+ return;
+ }
}
static bool isPermittedNeonBaseType(QualType &Ty, VectorKind VecKind, Sema &S) {
diff --git a/clang/test/SemaCXX/vector.cpp b/clang/test/SemaCXX/vector.cpp
index 808bdb679b09c..eea0b4756fae3 100644
--- a/clang/test/SemaCXX/vector.cpp
+++ b/clang/test/SemaCXX/vector.cpp
@@ -786,3 +786,8 @@ const long long e = *0; // expected-error {{indirection requires pointer operand
double f = a - e; // expected-error {{cannot initialize a variable of type 'double' with an rvalue of type '__attribute__((__vector_size__(1 * sizeof(double)))) double' (vector of 1 'double' value)}}
int h = c - e; // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type '__attribute__((__vector_size__(1 * sizeof(long)))) long' (vector of 1 'long' value)}}
}
+
+typedef int v_neg_size __attribute__((vector_size(-8))); // expected-error{{vector must have non-negative size}}
+typedef int v_neg_size_2 __attribute__((vector_size(-1 * 8))); // expected-error{{vector must have non-negative size}}
+typedef int v_ext_neg_size __attribute__((ext_vector_type(-8))); // expected-error{{vector must have non-negative size}}
+typedef int v_ext_neg_size2 __attribute__((ext_vector_type(-1 * 8))); // expected-error{{vector must have non-negative size}}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
clang/lib/Sema/SemaType.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| std::optional<llvm::APSInt> VecSize = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic should live in BuildVectorType() instead, near where we emit err_attribute_invalid_size.
clang/lib/Sema/SemaType.cpp
Outdated
|
|
||
| std::optional<llvm::APSInt> VecSize = | ||
| SizeExpr->getIntegerConstantExpr(S.Context); | ||
| if (VecSize && VecSize->isNegative()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar, I think this should live in BuildExtVectorType().
| typedef int v_neg_size __attribute__((vector_size(-8))); // expected-error{{vector must have non-negative size}} | ||
| typedef int v_neg_size_2 __attribute__((vector_size(-1 * 8))); // expected-error{{vector must have non-negative size}} | ||
| typedef int v_ext_neg_size __attribute__((ext_vector_type(-8))); // expected-error{{vector must have non-negative size}} | ||
| typedef int v_ext_neg_size2 __attribute__((ext_vector_type(-1 * 8))); // expected-error{{vector must have non-negative size}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test case to add which I think won't be handled currently but will after moving into the Build functions is: https://godbolt.org/z/avobhW3Yx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works. I just need to write this test in another file that allows C++11 extension to be able to use using
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot i can use ifdef to make it for specific version :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why the parameter name is ArraySize, not VectorSize or Size 🤔 ?
QualType Sema::BuildExtVectorType(QualType T, Expr *ArraySize, <---- SourceLocation AttrLoc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's just a think-o through refactoring, probably. Would be fine to rename in an NFC commit, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will create NFC for that soon @AaronBallman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done #166208
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
| typedef int v_neg_size __attribute__((vector_size(-8))); // expected-error{{vector must have non-negative size}} | ||
| typedef int v_neg_size_2 __attribute__((vector_size(-1 * 8))); // expected-error{{vector must have non-negative size}} | ||
| typedef int v_ext_neg_size __attribute__((ext_vector_type(-8))); // expected-error{{vector must have non-negative size}} | ||
| typedef int v_ext_neg_size2 __attribute__((ext_vector_type(-1 * 8))); // expected-error{{vector must have non-negative size}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's just a think-o through refactoring, probably. Would be fine to rename in an NFC commit, IMO.
Fix the parameter name in the BuildExtVectorType function, also updating the code style to be consistent with BuildVectorType Discovered in #166055
Report a diagnostic in case vector_size or ext_vector_type attributes are used with a negative size. The same evaluation result can be used for other checks, for example, the too big a size.
Issue #165463