[Clang] fix crash in codegen caused by deferred asm diagnostics under -fopenmp

[a-tarasyuk]

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 (using SemaDiagnosticBuilder::K_Deferred) in OpenMP target regions that may not be emitted, which allowed malformed asm statements to reach codegen and cause a crash.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

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 (using SemaDiagnosticBuilder::K_Deferred) in OpenMP target regions that may not be emitted, which allowed malformed asm statements to reach codegen and cause a crash.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaStmtAsm.cpp (+14-20)
• (added) clang/test/OpenMP/openmp_asm.c (+28)

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;
+}

[AaronBallman]

The changes look correct to me, but I added Eli and Alexey for some other opinions to be sure.

[efriedma-quic]

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.

[AaronBallman]

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 CodeGeneratorImpl::HandleTopLevelDecl() looks to see if an unrecoverable error occurred. It does not look at whether any of the top-level decls are invalid. Perhaps that's too fragile?

[efriedma-quic]

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.