-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Sema] Fix type mismatch error when arguments to elementwise math builtin have different qualifiers, which should be well-formed #141485
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
Conversation
@llvm/pr-subscribers-clang Author: QiYue (QiYueFeiXue) ChangesFixes #141397 Full diff: https://github.com/llvm/llvm-project/pull/141485.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 930e9083365a1..61aeffde338d8 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2907,7 +2907,7 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
return ExprError();
}
- if (MagnitudeTy.getCanonicalType() != SignTy.getCanonicalType()) {
+ if (!Context.hasSameUnqualifiedType(MagnitudeTy, SignTy)) {
return Diag(Sign.get()->getBeginLoc(),
diag::err_typecheck_call_different_arg_types)
<< MagnitudeTy << SignTy;
@@ -5265,7 +5265,7 @@ bool Sema::BuiltinComplex(CallExpr *TheCall) {
Expr *Real = TheCall->getArg(0);
Expr *Imag = TheCall->getArg(1);
- if (!Context.hasSameType(Real->getType(), Imag->getType())) {
+ if (!Context.hasSameUnqualifiedType(Real->getType(), Imag->getType())) {
return Diag(Real->getBeginLoc(),
diag::err_typecheck_call_different_arg_types)
<< Real->getType() << Imag->getType()
@@ -15568,7 +15568,7 @@ Sema::BuiltinVectorMath(CallExpr *TheCall,
if (checkMathBuiltinElementType(*this, LocA, TyA, ArgTyRestr, 1))
return std::nullopt;
- if (TyA.getCanonicalType() != TyB.getCanonicalType()) {
+ if (!Context.hasSameUnqualifiedType(TyA, TyB)) {
Diag(LocA, diag::err_typecheck_call_different_arg_types) << TyA << TyB;
return std::nullopt;
}
@@ -15607,8 +15607,8 @@ bool Sema::BuiltinElementwiseTernaryMath(
}
for (int I = 1; I < 3; ++I) {
- if (Args[0]->getType().getCanonicalType() !=
- Args[I]->getType().getCanonicalType()) {
+ if (!Context.hasSameUnqualifiedType(Args[0]->getType(),
+ Args[I]->getType())) {
return Diag(Args[0]->getBeginLoc(),
diag::err_typecheck_call_different_arg_types)
<< Args[0]->getType() << Args[I]->getType();
diff --git a/clang/test/SemaCXX/bug141397.cpp b/clang/test/SemaCXX/bug141397.cpp
new file mode 100644
index 0000000000000..cb9f13a93f955
--- /dev/null
+++ b/clang/test/SemaCXX/bug141397.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+// expected-no-diagnostics
+
+// This example uncovered a bug in Sema::BuiltinVectorMath, where we should be
+// using ASTContext::hasSameUnqualifiedType().
+
+typedef float vec3 __attribute__((ext_vector_type(3)));
+
+typedef struct {
+ vec3 b;
+} struc;
+
+vec3 foo(vec3 a,const struc* hi) {
+ vec3 b = __builtin_elementwise_max((vec3)(0.0f), a);
+ return __builtin_elementwise_pow(b, hi->b.yyy);
+}
|
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.
This change needs a release note.
Please add an entry to clang/docs/ReleaseNotes.rst
in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!
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 the fix.
Please put more details in your summary. Something along the lines of "builtin functions such as __builtin_elementwise_max .... fail when their arguments have different qualifications but this should be well-formed ... the fix involves using hasSameUnqualifiedType to check..."
It is important to have detailed summaries for the reviewers so they have more context without having to leave the code review and for folks using git log to debug issues when their downstream fork has issues.
8152277
to
86e50b2
Compare
@QiYueFeiXue I don't think @shafik means to directly copy his words to the PR body/title. Please at least flesh out these dotted parts and make the sentences complete, thanks. |
c719295
to
0feb5eb
Compare
clang/docs/ReleaseNotes.rst
Outdated
@@ -669,6 +669,7 @@ Bug Fixes in This Version | |||
base classes. (GH139452) | |||
- Fixed an assertion failure in serialization of constexpr structs containing unions. (#GH140130) | |||
- Fixed duplicate entries in TableGen that caused the wrong attribute to be selected. (GH#140701) | |||
- Fixed type missmach error when 'builtin-elementwise-math' arguments have different qualifiers, this should be a well-formed. (#GH141397) |
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.
Missmach -> mismatch (and same in the test!)
A well-formed -> well-formed
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, thanks!
Yes, that is what I meant, thank you for clarifying my intent. |
@Acthink-Yang Do you have commit access? If not then let us know and one of us can merge for you. |
Hi @shafik Thanks for checking! I don't have commit access yet. |
Fixes #141397
Element-wise math builtins (e.g. __builtin_elementwise_max/__builtin_elementwise_pow etc.) fail when their arguments have different qualifications, but should be well-formed. The fix is to use hasSameUnqualifiedType to check if the arguments match.