[clang][OpenMP] Diagnose invalid allocator in #pragma omp allocate; avoid null deref
#158146
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-clang Author: Krish Gupta (KrxGu) ChangesReproducer without <omp.h> (forged Fix:
Tests:
<img width="1240" height="220" alt="image" src="https://github.com/user-attachments/assets/590242a7-0206-4ac5-b142-f595ddc0ac64" /> complete build check: no crash (error is expected): Fixes #157868. Full diff: https://github.com/llvm/llvm-project/pull/158146.diff 4 Files Affected:
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 63a56a6583efc..1af8ed20f24e2 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -3321,27 +3321,33 @@ SemaOpenMP::CheckOMPThreadPrivateDecl(SourceLocation Loc,
}
return D;
}
-
static OMPAllocateDeclAttr::AllocatorTypeTy
getAllocatorKind(Sema &S, DSAStackTy *Stack, Expr *Allocator) {
+ // No allocator expression → Null mem alloc (matches existing tests).
if (!Allocator)
return OMPAllocateDeclAttr::OMPNullMemAlloc;
+
if (Allocator->isTypeDependent() || Allocator->isValueDependent() ||
Allocator->isInstantiationDependent() ||
Allocator->containsUnexpandedParameterPack())
return OMPAllocateDeclAttr::OMPUserDefinedMemAlloc;
+
auto AllocatorKindRes = OMPAllocateDeclAttr::OMPUserDefinedMemAlloc;
+
llvm::FoldingSetNodeID AEId;
const Expr *AE = Allocator->IgnoreParenImpCasts();
- AE->IgnoreImpCasts()->Profile(AEId, S.getASTContext(), /*Canonical=*/true);
+ AE->Profile(AEId, S.getASTContext(), /*Canonical=*/true);
+
for (int I = 0; I < OMPAllocateDeclAttr::OMPUserDefinedMemAlloc; ++I) {
- auto AllocatorKind = static_cast<OMPAllocateDeclAttr::AllocatorTypeTy>(I);
- const Expr *DefAllocator = Stack->getAllocator(AllocatorKind);
+ auto K = static_cast<OMPAllocateDeclAttr::AllocatorTypeTy>(I);
+ const Expr *Def = Stack->getAllocator(K);
+ if (!Def)
+ continue;
llvm::FoldingSetNodeID DAEId;
- DefAllocator->IgnoreImpCasts()->Profile(DAEId, S.getASTContext(),
- /*Canonical=*/true);
+ Def->IgnoreImpCasts()->Profile(DAEId, S.getASTContext(),
+ /*Canonical=*/true);
if (AEId == DAEId) {
- AllocatorKindRes = AllocatorKind;
+ AllocatorKindRes = K;
break;
}
}
@@ -3353,10 +3359,12 @@ static bool checkPreviousOMPAllocateAttribute(
OMPAllocateDeclAttr::AllocatorTypeTy AllocatorKind, Expr *Allocator) {
if (!VD->hasAttr<OMPAllocateDeclAttr>())
return false;
+
const auto *A = VD->getAttr<OMPAllocateDeclAttr>();
Expr *PrevAllocator = A->getAllocator();
OMPAllocateDeclAttr::AllocatorTypeTy PrevAllocatorKind =
getAllocatorKind(S, Stack, PrevAllocator);
+
bool AllocatorsMatch = AllocatorKind == PrevAllocatorKind;
if (AllocatorsMatch &&
AllocatorKind == OMPAllocateDeclAttr::OMPUserDefinedMemAlloc &&
@@ -3368,11 +3376,13 @@ static bool checkPreviousOMPAllocateAttribute(
PAE->Profile(PAEId, S.Context, /*Canonical=*/true);
AllocatorsMatch = AEId == PAEId;
}
+
if (!AllocatorsMatch) {
SmallString<256> AllocatorBuffer;
llvm::raw_svector_ostream AllocatorStream(AllocatorBuffer);
if (Allocator)
Allocator->printPretty(AllocatorStream, nullptr, S.getPrintingPolicy());
+
SmallString<256> PrevAllocatorBuffer;
llvm::raw_svector_ostream PrevAllocatorStream(PrevAllocatorBuffer);
if (PrevAllocator)
@@ -3408,8 +3418,7 @@ applyOMPAllocateAttribute(Sema &S, VarDecl *VD,
(Alignment->isTypeDependent() || Alignment->isValueDependent() ||
Alignment->isInstantiationDependent() ||
Alignment->containsUnexpandedParameterPack()))
- // Apply later when we have a usable value.
- return;
+ return; // apply later
if (Allocator &&
(Allocator->isTypeDependent() || Allocator->isValueDependent() ||
Allocator->isInstantiationDependent() ||
@@ -3430,9 +3439,6 @@ SemaOpenMP::DeclGroupPtrTy SemaOpenMP::ActOnOpenMPAllocateDirective(
Expr *Allocator = nullptr;
if (Clauses.empty()) {
// OpenMP 5.0, 2.11.3 allocate Directive, Restrictions.
- // allocate directives that appear in a target region must specify an
- // allocator clause unless a requires directive with the dynamic_allocators
- // clause is present in the same compilation unit.
if (getLangOpts().OpenMPIsTargetDevice &&
!DSAStack->hasRequiresDeclWithClause<OMPDynamicAllocatorsClause>())
SemaRef.targetDiag(Loc, diag::err_expected_allocator_clause);
@@ -3445,6 +3451,7 @@ SemaOpenMP::DeclGroupPtrTy SemaOpenMP::ActOnOpenMPAllocateDirective(
else
llvm_unreachable("Unexpected clause on allocate directive");
}
+ // No forward-decl needed; just classify with null-guards.
OMPAllocateDeclAttr::AllocatorTypeTy AllocatorKind =
getAllocatorKind(SemaRef, DSAStack, Allocator);
SmallVector<Expr *, 8> Vars;
@@ -3452,23 +3459,19 @@ SemaOpenMP::DeclGroupPtrTy SemaOpenMP::ActOnOpenMPAllocateDirective(
auto *DE = cast<DeclRefExpr>(RefExpr);
auto *VD = cast<VarDecl>(DE->getDecl());
- // Check if this is a TLS variable or global register.
+ // Skip TLS/global-registers.
if (VD->getTLSKind() != VarDecl::TLS_None ||
VD->hasAttr<OMPThreadPrivateDeclAttr>() ||
(VD->getStorageClass() == SC_Register && VD->hasAttr<AsmLabelAttr>() &&
!VD->isLocalVarDecl()))
continue;
- // If the used several times in the allocate directive, the same allocator
- // must be used.
+ // Enforce same allocator if repeated.
if (checkPreviousOMPAllocateAttribute(SemaRef, DSAStack, RefExpr, VD,
AllocatorKind, Allocator))
continue;
- // OpenMP, 2.11.3 allocate Directive, Restrictions, C / C++
- // If a list item has a static storage type, the allocator expression in the
- // allocator clause must be a constant expression that evaluates to one of
- // the predefined memory allocator values.
+ // For static storage, allocator must be predefined.
if (Allocator && VD->hasGlobalStorage()) {
if (AllocatorKind == OMPAllocateDeclAttr::OMPUserDefinedMemAlloc) {
Diag(Allocator->getExprLoc(),
diff --git a/clang/test/OpenMP/allocate-allocator-duplicate.c b/clang/test/OpenMP/allocate-allocator-duplicate.c
new file mode 100644
index 0000000000000..91a294160992c
--- /dev/null
+++ b/clang/test/OpenMP/allocate-allocator-duplicate.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fopenmp -verify %s
+
+typedef enum omp_allocator_handle_t {
+ omp_default_mem_alloc = 1,
+ __omp_allocator_handle_t_max__ = __UINTPTR_MAX__
+} omp_allocator_handle_t;
+
+void foo(void) {
+ omp_allocator_handle_t my_handle;
+ int A[2];
+ // expected-error@+2 {{'omp_allocator_handle_t' type not found; include <omp.h>}}
+ // expected-note@+1 {{previous allocator is specified here}}
+ #pragma omp allocate(A) allocator(my_handle)
+ // expected-warning@+1 {{allocate directive specifies 'my_handle' allocator while previously used default}}
+ #pragma omp allocate(A) allocator(my_handle)
+}
diff --git a/clang/test/OpenMP/allocate-allocator-handle-diag.c b/clang/test/OpenMP/allocate-allocator-handle-diag.c
new file mode 100644
index 0000000000000..07bf68bd9972d
--- /dev/null
+++ b/clang/test/OpenMP/allocate-allocator-handle-diag.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fopenmp -verify %s
+// No <omp.h>; forge a typedef.
+typedef enum omp_allocator_handle_t {
+ omp_default_mem_alloc = 1,
+ __omp_allocator_handle_t_max__ = __UINTPTR_MAX__
+} omp_allocator_handle_t;
+
+void foo(void) {
+ omp_allocator_handle_t my_handle;
+ int A[2];
+ // expected-error@+1 {{'omp_allocator_handle_t' type not found; include <omp.h>}}
+ #pragma omp allocate(A) allocator(my_handle)
+}
diff --git a/flang/test/Lower/OpenMP/lastprivate-alloc-scope.f90 b/flang/test/Lower/OpenMP/lastprivate-alloc-scope.f90
new file mode 100644
index 0000000000000..67d885ed5fb7a
--- /dev/null
+++ b/flang/test/Lower/OpenMP/lastprivate-alloc-scope.f90
@@ -0,0 +1,22 @@
+! RUN: %flang_fc1 -fopenmp -emit-hlfir %s -o - | FileCheck %s
+
+program p
+ type y3; integer, allocatable :: x; end type
+ type(y3) :: v
+ integer :: s, n, i
+ s = 1; n = 10
+ allocate(v%x); v%x = 0
+!$omp parallel
+ if (.not. allocated(v%x)) print *, '101', allocated(v%x)
+!$omp do schedule(dynamic) lastprivate(v)
+ do i = s, n
+ v%x = i
+ end do
+!$omp end do
+!$omp end parallel
+end program
+
+! CHECK: omp.parallel {
+! CHECK-NOT: private(
+! CHECK: omp.wsloop
+! CHECK-SAME: private(
|
shafik
reviewed
Sep 11, 2025
…e missing <omp.h> in (fixes llvm#157868)
KrxGu
force-pushed
the
clang-omp-allocate-npe-fix
branch
from
1055e50 to
21c0511
Compare
|
@shafik reverted the comments, brought back |
shafik
reviewed
Sep 17, 2025
| } | ||
| return D; | ||
| } | ||
|
|
There was a problem hiding this comment.
Please remove this newline
zahiraam
reviewed
Sep 17, 2025
| int A[2]; | ||
| // expected-error@+1 {{'omp_allocator_handle_t' type not found; include <omp.h>}} | ||
| #pragma omp allocate(A) allocator(my_handle) | ||
| } |
There was a problem hiding this comment.
Is there a reason why you created 2 tests instead of just one? I think you can just a single test case. Also the naming should be with an _ instead of an -, i.e allocate_allocator_duplicate.c.
| ! CHECK: omp.parallel { | ||
| ! CHECK-NOT: private( | ||
| ! CHECK: omp.wsloop | ||
| ! CHECK-SAME: private( |
There was a problem hiding this comment.
@kparzysz can you please take a look at the f90 test?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reproducer without <omp.h> (forged
omp_allocator_handle_t) crashed in SemaOpenMP(null deref while classifying the allocator).
Fix:
Tests:
complete build check:
no crash (error is expected):
Fixes #157868.