-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[Clang] fix crash in codegen caused by deferred asm diagnostics under -fopenmp #147163
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesFixes #140375 This patch addresses a crash in clang’s codegen stage triggered by invalid inline assembly statements under Full diff: https://github.com/llvm/llvm-project/pull/147163.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9a94c4bcd9980..dcf2ffe43edfd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -750,6 +750,8 @@ Bug Fixes in This Version
- Fixed an infinite recursion when checking constexpr destructors. (#GH141789)
- Fixed a crash when a malformed using declaration appears in a ``constexpr`` function. (#GH144264)
- Fixed a bug when use unicode character name in macro concatenation. (#GH145240)
+- Fixed a crash caused by deferred diagnostics under ``-fopenmp``,
+ which resulted in passing invalid asm statements to codegen. (#GH140375)
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaStmtAsm.cpp b/clang/lib/Sema/SemaStmtAsm.cpp
index 4507a21a4c111..b949178f6a938 100644
--- a/clang/lib/Sema/SemaStmtAsm.cpp
+++ b/clang/lib/Sema/SemaStmtAsm.cpp
@@ -309,10 +309,9 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
TargetInfo::ConstraintInfo Info(ConstraintStr, OutputName);
if (!Context.getTargetInfo().validateOutputConstraint(Info) &&
!(LangOpts.HIPStdPar && LangOpts.CUDAIsDevice)) {
- targetDiag(Constraint->getBeginLoc(),
- diag::err_asm_invalid_output_constraint)
- << Info.getConstraintStr();
- return CreateGCCAsmStmt();
+ return StmtError(targetDiag(Constraint->getBeginLoc(),
+ diag::err_asm_invalid_output_constraint)
+ << Info.getConstraintStr());
}
ExprResult ER = CheckPlaceholderExpr(Exprs[i]);
@@ -378,9 +377,9 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
FeatureMap,
GCCAsmStmt::ExtractStringFromGCCAsmStmtComponent(Constraint),
Size)) {
- targetDiag(OutputExpr->getBeginLoc(), diag::err_asm_invalid_output_size)
- << Info.getConstraintStr();
- return CreateGCCAsmStmt();
+ return StmtError(targetDiag(OutputExpr->getBeginLoc(),
+ diag::err_asm_invalid_output_size)
+ << Info.getConstraintStr());
}
}
@@ -399,10 +398,9 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
TargetInfo::ConstraintInfo Info(ConstraintStr, InputName);
if (!Context.getTargetInfo().validateInputConstraint(OutputConstraintInfos,
Info)) {
- targetDiag(Constraint->getBeginLoc(),
- diag::err_asm_invalid_input_constraint)
- << Info.getConstraintStr();
- return CreateGCCAsmStmt();
+ return StmtError(targetDiag(Constraint->getBeginLoc(),
+ diag::err_asm_invalid_input_constraint)
+ << Info.getConstraintStr());
}
ExprResult ER = CheckPlaceholderExpr(Exprs[i]);
@@ -504,13 +502,9 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
GCCAsmStmt::ExtractStringFromGCCAsmStmtComponent(ClobberExpr);
if (!Context.getTargetInfo().isValidClobber(Clobber)) {
- targetDiag(ClobberExpr->getBeginLoc(),
- diag::err_asm_unknown_register_name)
- << Clobber;
- return new (Context) GCCAsmStmt(
- Context, AsmLoc, IsSimple, IsVolatile, NumOutputs, NumInputs, Names,
- constraints.data(), Exprs.data(), asmString, NumClobbers,
- clobbers.data(), NumLabels, RParenLoc);
+ return StmtError(targetDiag(ClobberExpr->getBeginLoc(),
+ diag::err_asm_unknown_register_name)
+ << Clobber);
}
if (Clobber == "unwind") {
@@ -520,8 +514,8 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
// Using unwind clobber and asm-goto together is not supported right now.
if (UnwindClobberLoc && NumLabels > 0) {
- targetDiag(*UnwindClobberLoc, diag::err_asm_unwind_and_goto);
- return CreateGCCAsmStmt();
+ return StmtError(
+ targetDiag(*UnwindClobberLoc, diag::err_asm_unwind_and_goto));
}
GCCAsmStmt *NS = CreateGCCAsmStmt();
diff --git a/clang/test/OpenMP/openmp_asm.c b/clang/test/OpenMP/openmp_asm.c
new file mode 100644
index 0000000000000..f2705d1a8803f
--- /dev/null
+++ b/clang/test/OpenMP/openmp_asm.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -verify=fopenmp -emit-llvm -o - %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -verify -emit-llvm -o - %s
+
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -x c++ -fopenmp -verify=fopenmp -emit-llvm -o - %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -x c++ -verify -emit-llvm -o - %s
+
+// fopenmp-no-diagnostics
+
+void t1(int *a, int *b) {
+ asm volatile("" : "+&r"(a) : ""(b)); // expected-error {{invalid input constraint '' in asm}}
+}
+
+void t2() {
+ asm ("nop" : : : "foo"); // expected-error {{unknown register name 'foo' in asm}}
+}
+
+void t3() {
+ asm goto ("" ::: "unwind" : label); // expected-error {{unwind clobber cannot be used with asm goto}}
+label:
+ ;
+}
+
+typedef int vec256 __attribute__((ext_vector_type(8)));
+vec256 t4() {
+ vec256 out;
+ asm("something %0" : "=y"(out)); // expected-error {{invalid output size for constraint '=y'}}
+ return out;
+}
|
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.
The changes look correct to me, but I added Eli and Alexey for some other opinions to be sure.
I haven't really thought about what the right strategy is for diagnostics here, specifically, but it looks like the OpenMP diagnostic suppression isn't working correctly. If there's an error in a function, that should stop CodeGen from seeing the function at all. If it's crashing, that means we're doing codegen on invalid functions, which we don't want to do. |
I just ran into that logic earlier today on a different PR. The way it works is that |
We shouldn't have invalid decls unless we've emitted an error. If we're making some decision like "this function is an error if it's used as a host function", we should probably be marking the FunctionDecl explicitly with that decision, and CodeGen shouldn't try to emit such a function. Also, the decision whether a function counts as "used" should probably be driven by Sema. Currently you don't get errors at all with -fsyntax-only. |
Fixes #140375
This patch addresses a crash in clang’s codegen stage triggered by invalid inline assembly statements under
-fopenmp
. The root cause was deferred diagnostic emission (usingSemaDiagnosticBuilder::K_Deferred
) in OpenMP target regions that may not be emitted, which allowed malformed asm statements to reach codegen and cause a crash.