[MLIR][OpenMP] Bail early in sortMapIndices if indices are the same #169474
Conversation
If we are given the same index in the comparator callback, simply return false. Otherwise we will end up adding invalid items to occludedChildren, causing extra items to get removed that should not be, resulting in failures that manifest in different forms (assertions, asan failures, ubsan failures, etc.).
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-flang-openmp Author: Aiden Grossman (boomanaiden154) ChangesIf we are given the same index in the comparator callback, simply return false. Otherwise we will end up adding invalid items to occludedChildren, causing extra items to get removed that should not be, resulting in failures that manifest in different forms (assertions, asan failures, ubsan failures, etc.). Full diff: https://github.com/llvm/llvm-project/pull/169474.diff 1 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 5511f4f1af1ad..98775c2d18bd4 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -4095,6 +4095,12 @@ static void sortMapIndices(llvm::SmallVectorImpl<size_t> &indices,
llvm::SmallVector<size_t> occludedChildren;
llvm::sort(
indices.begin(), indices.end(), [&](const size_t a, const size_t b) {
+ // Bail early if we are asked to look at the same index. If we do not
+ // bail early, we can end up mistakenly adding indices to
+ // occludedChildren. This can occur with some types of libc++ hardening.
+ if (a == b)
+ return false;
+
auto memberIndicesA = cast<ArrayAttr>(indexAttr[a]);
auto memberIndicesB = cast<ArrayAttr>(indexAttr[b]);
|
|
@llvm/pr-subscribers-mlir-openmp Author: Aiden Grossman (boomanaiden154) ChangesIf we are given the same index in the comparator callback, simply return false. Otherwise we will end up adding invalid items to occludedChildren, causing extra items to get removed that should not be, resulting in failures that manifest in different forms (assertions, asan failures, ubsan failures, etc.). Full diff: https://github.com/llvm/llvm-project/pull/169474.diff 1 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 5511f4f1af1ad..98775c2d18bd4 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -4095,6 +4095,12 @@ static void sortMapIndices(llvm::SmallVectorImpl<size_t> &indices,
llvm::SmallVector<size_t> occludedChildren;
llvm::sort(
indices.begin(), indices.end(), [&](const size_t a, const size_t b) {
+ // Bail early if we are asked to look at the same index. If we do not
+ // bail early, we can end up mistakenly adding indices to
+ // occludedChildren. This can occur with some types of libc++ hardening.
+ if (a == b)
+ return false;
+
auto memberIndicesA = cast<ArrayAttr>(indexAttr[a]);
auto memberIndicesB = cast<ArrayAttr>(indexAttr[b]);
|
|
@agozillon This is the fix I mentioned in #119589. This seems right to me, but I'm not very familiar with this code, so let me know what you think. |
agozillon
left a comment
agozillon
left a comment
There was a problem hiding this comment.
LGTM, thank you for the fix!
4 participants
If we are given the same index in the comparator callback, simply return false. Otherwise we will end up adding invalid items to occludedChildren, causing extra items to get removed that should not be, resulting in failures that manifest in different forms (assertions, asan failures, ubsan failures, etc.).