[Clang][OpenMP] fixed crash due to invalid binary expression in checking atomic semantics

[sun-jacobi]

This PR fixes #69069 .

[llvmbot]

@llvm/pr-subscribers-clang

Author: Chia (sun-jacobi)

Changes

This PR fixes #69069 .

---

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

1 Files Affected:

• (modified) clang/lib/Sema/SemaOpenMP.cpp (+7)

diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 1bd34f73e5f7e00..e05fa54d8118319 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -11605,6 +11605,9 @@ class OpenMPAtomicUpdateChecker {
     /// RHS binary operation does not have reference to the updated LHS
     /// part.
     NotAnUpdateExpression,
+    /// An expression contains semantical error not related to
+    /// 'omp atomic [update]'
+    NotAValidExpression,
     /// No errors is found.
     NoError
   };
@@ -11782,6 +11785,10 @@ bool OpenMPAtomicUpdateChecker::checkStatement(Stmt *S, unsigned DiagId,
         ErrorFound = NotABinaryOrUnaryExpression;
         NoteLoc = ErrorLoc = AtomicBody->getExprLoc();
         NoteRange = ErrorRange = AtomicBody->getSourceRange();
+      } else if (AtomicBody->containsErrors()) {
+        ErrorFound = NotAValidExpression;
+        NoteLoc = ErrorLoc = AtomicBody->getExprLoc();
+        NoteRange = ErrorRange = AtomicBody->getSourceRange();
       }
     } else {
       ErrorFound = NotAScalarType;

[shiltian]

This doesn't look like the right place to fix this issue to me. @alexey-bataev might have better suggestion.

[sun-jacobi]

This doesn't look like the right place to fix this issue to me. @alexey-bataev might have better suggestion.

The crash occurs at

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 13011 in d34a10a

Checker.getX()->Profile(XId, Context, /*Canonical=*/true);

The reason why I fixed the issue like this is that, I think here IsUpdateExprFound
https://github.com/llvm/llvm-project/blob/d34a10a47d25103725174c62ed5e84e8ca380249/clang/lib/Sema/SemaOpenMP.cpp#L13000C15-L13000C32
should not be true, since the second statement (or expression) is invalid expression.

The IsUpdateExprFound comes from

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 12994 in d34a10a

bool IsUpdateExprFound = !Checker.checkStatement(Second);

This function returns false, then the IsUpdateExprFound will be true. This is the reason why I fixed this function

[sun-jacobi]

Moreover, in the original implementation, the checkStatement return false if and only if there is no error in the statement.
The question is whether checkStatement should consider a statement, which contains semantic errors not related to the atomic directive, as a NoError statement.

In my opinion, for such a case, it should be considered as an invalid statement.

[alexey-bataev]

Looks like correct fix to me.

[shiltian]

I have merged it given our front end expert @alexey-bataev has approved it.