[OpenMP][Flang] Fix OOB access for derived type mapping #140948
Merged
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
This patch fixes unintentional OOB acess when mapping members of derived type.
llvmbot
added
flang
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an out-of-bound access issue in derived type mapping used in OpenMP by altering the loop control flow.
- Replaces a combined conditional check with two separate branches: one to break immediately when processing the final member and another to handle non-descriptor members.
- Updates code comments to reflect the revised control flow in flang/lib/Lower/OpenMP/Utils.cpp.
Comments suppressed due to low confidence (2)
flang/lib/Lower/OpenMP/Utils.cpp:367
- Using 'break' here correctly exits the loop when processing the final member, preventing out-of-bound access. Confirm that this early exit does not bypass any required cleanup or finalization steps needed in the loop.
if (currentIndicesIdx == indices.size() - 1)
flang/lib/Lower/OpenMP/Utils.cpp:376
- Ensure that incrementing 'currentIndicesIdx' within this branch consistently maintains the expected mapping behavior, particularly when multiple non-descriptor members are encountered consecutively.
if (!fir::isTypeWithDescriptor(memberTy)) {
Member
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Akash Banerjee (TIFitis) ChangesThis patch fixes unintentional OOB acess when mapping members of derived type. When This fixes the following test case: Full diff: https://github.com/llvm/llvm-project/pull/140948.diff 1 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp
index 173dceb07b193..711d4af287691 100644
--- a/flang/lib/Lower/OpenMP/Utils.cpp
+++ b/flang/lib/Lower/OpenMP/Utils.cpp
@@ -362,16 +362,18 @@ mlir::Value createParentSymAndGenIntermediateMaps(
clauseLocation, firOpBuilder.getRefType(memberTy), curValue,
llvm::SmallVector<fir::IntOrValue, 1>{idxConst});
- // Skip mapping and the subsequent load if we're the final member or not
- // a type with a descriptor such as a pointer/allocatable. If we're a
- // final member, the map will be generated by the processMap call that
- // invoked this function, and if we're not a type with a descriptor then
- // we have no need of generating an intermediate map for it, as we only
- // need to generate a map if a member is a descriptor type (and thus
- // obscures the members it contains via a pointer in which it's data needs
- // mapped)
- if ((currentIndicesIdx == indices.size() - 1) ||
- !fir::isTypeWithDescriptor(memberTy)) {
+ // If we're a final member, the map will be generated by the processMap
+ // call that invoked this function.
+ if (currentIndicesIdx == indices.size() - 1)
+ break;
+
+ // Skip mapping and the subsequent load if we're not
+ // a type with a descriptor such as a pointer/allocatable. If we're not a
+ // type with a descriptor then we have no need of generating an
+ // intermediate map for it, as we only need to generate a map if a member
+ // is a descriptor type (and thus obscures the members it contains via a
+ // pointer in which it's data needs mapped).
+ if (!fir::isTypeWithDescriptor(memberTy)) {
currentIndicesIdx++;
continue;
}
|
agozillon
approved these changes
May 21, 2025
Contributor
agozillon
left a comment
agozillon
left a comment
There was a problem hiding this comment.
LGTM, thank you for the catch/fix! :-)
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/23576 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This patch fixes unintentional OOB acess when mapping members of derived type.
When
currentIndicesIdxismaxSize - 1, i.e, it is the last member mapping, we can break from the loop. This was already the intended behaviour but usingcontinueinstead ofbreakresulted in OOB access ofindices.This fixes the following test case: