[clang][OpenMP] Add error for large expr in collapse clause

[AmrDeveloper]

Report error when OpenMP collapse clause has an expression that can't be represented in 64-bit

Issue #138445

[llvmbot]

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Report error when OpenMP SIMD collapse clause has an expression that can't be represented in 64-bit

Issue #138445

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
• (modified) clang/lib/Sema/SemaOpenMP.cpp (+7)
• (modified) clang/test/OpenMP/simd_collapse_messages.cpp (+2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 203958dab7430..93c296360aebe 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -503,6 +503,8 @@ Improvements to Clang's diagnostics - ``-Wreserved-identifier`` now fires on reserved parameter names in a function declaration which is not a definition. +- An error is now emitted when OpenMP SIMD ``collapse`` clause has expression larger than 64 bit. + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index e5a7cdc14a737..b0cbcf8d8a131 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11519,6 +11519,8 @@ def note_omp_collapse_ordered_expr : Note< "as specified in %select{'collapse'|'ordered'|'collapse' and 'ordered'}0 clause%select{||s}0">; def err_omp_negative_expression_in_clause : Error< "argument to '%0' clause must be a %select{non-negative|strictly positive}1 integer value">; +def err_omp_large_expression_in_clause : Error< + "argument to '%0' clause cannot have more than 64 bits size">; def err_omp_not_integral : Error< "expression must have integral or unscoped enumeration " "type, not %0">; diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 835dba22a858d..52cb5b1fb5d71 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -15901,6 +15901,13 @@ ExprResult SemaOpenMP::VerifyPositiveIntegerConstantInClause( << E->getSourceRange(); return ExprError(); } + + if (!Result.isRepresentableByInt64()) { + Diag(E->getExprLoc(), diag::err_omp_large_expression_in_clause) + << getOpenMPClauseNameForDiag(CKind) << E->getSourceRange(); + return ExprError(); + } + if (CKind == OMPC_collapse && DSAStack->getAssociatedLoops() == 1) DSAStack->setAssociatedLoops(Result.getExtValue()); else if (CKind == OMPC_ordered) diff --git a/clang/test/OpenMP/simd_collapse_messages.cpp b/clang/test/OpenMP/simd_collapse_messages.cpp index 1ce3bef3535ce..3ae0a818284e9 100644 --- a/clang/test/OpenMP/simd_collapse_messages.cpp +++ b/clang/test/OpenMP/simd_collapse_messages.cpp @@ -43,6 +43,8 @@ T tmain(T argc, S **argv) { for (int i = ST; i < N; i++) argv[0][i] = argv[0][i] - argv[0][i-ST]; #pragma omp simd collapse (S) // expected-error {{'S' does not refer to a value}} for (int i = ST; i < N; i++) argv[0][i] = argv[0][i] - argv[0][i-ST]; + #pragma omp simd collapse (0xFFFFFFFFFFFFFFFF) // expected-error {{argument to 'collapse' clause cannot have more than 64 bits size}} + for (int i = ST; i < N; i++) argv[0][i] = argv[0][i] - argv[0][i-ST]; #if __cplusplus <= 199711L // expected-error@+4 2 {{integral constant expression}} expected-note@+4 0+{{constant expression}} #else 

[k-arrows]

I am not a developer, so I won't review the implementation, but I have one comment. The way I originally wrote the bug report was poor, but it is not necessary to limit the fix and its test to OpenMP SIMD. For example, the code below also crashes:
https://godbolt.org/z/7T8MbE5j5

void f(void) { #pragma omp for collapse(0xFFFFFFFFFFFFFFFF) for (int i = 0; i < 10; i++) ; }

[AmrDeveloper]

I am not a developer, so I won't review the implementation, but I have one comment. The way I originally wrote the bug report was poor, but it is not necessary to limit the fix and its test to OpenMP SIMD. For example, the code below also crashes: https://godbolt.org/z/7T8MbE5j5

void f(void) { #pragma omp for collapse(0xFFFFFFFFFFFFFFFF) for (int i = 0; i < 10; i++) ; }

Thank you, that makes sense. I will extend the test with more cases

[k-arrows]

In addition to the collapse clause, the ordered clause also seems to have the same problem. For example, does your patch also fix the following crash? If so, you also need tests regarding the ordered clause.
https://godbolt.org/z/96sa5jxff

void f(void) { #pragma omp for ordered(0xFFFFFFFFFFFFFFFF) for (int i = 0; i < 10; i++) ; }

[AmrDeveloper]

In addition to the collapse clause, the ordered clause also seems to have the same problem. For example, does your patch also fix the following crash? If so, you also need tests regarding the ordered clause. https://godbolt.org/z/96sa5jxff

void f(void) { #pragma omp for ordered(0xFFFFFFFFFFFFFFFF) for (int i = 0; i < 10; i++) ; }

Thank you, yes, it will fix this case too

[AaronBallman]

Thank you for the fix! I've added a few more reviewers. This is a pretty general problem, so I think we may want additional test coverage for basically any of the OpenMP clauses which accept an integer argument. For example, this is another related issue: #139268 -- I expect they're all resolved by your fix, perhaps?

[AmrDeveloper]

Thank you for the fix! I've added a few more reviewers. This is a pretty general problem, so I think we may want additional test coverage for basically any of the OpenMP clauses which accept an integer argument. For example, this is another related issue: #139268 -- I expect they're all resolved by your fix, perhaps?

Thank you for adding reviewers.

It will not fix the other issue because ActOnOpenMPUnrollDirective uses getZValue directly and doesn't call VerifyPositiveIntegerConstantInClause, the solution will be similar, but inside ActOnOpenMPUnrollDirective.

I will check for cases that fixed because they are using VerifyPositiveIntegerConstantInClause and add them to the test

[shafik]

So these are always 64 bit integers? This: https://www.openmp.org/spec-html/5.2/openmpsu30.html#x60-620004.4.3, "expression of integer type"

[AmrDeveloper]

In our implementation, yes, because we const evaluate the value to APInt and it store the value in i64

[shafik]

Suggested change

	"argument to '%0' clause cannot have more than 64 bits size">;
	"argument to '%0' clause cannot have more than 64 bits">;

[AaronBallman]

LGTM, but I'd appreciate confirmation from an OpenMP code owner too.