-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[flang] fix unsafe memory access using mlir::ValueRange #78435
Conversation
@llvm/pr-subscribers-flang-fir-hlfir Author: None (madanial0) ChangesWhen running the
The issue is caused by the "Merge disjoint stack slots" pass and the error is not present if the source is built with Thanks to investigation by @stefanp-ibm we found that "the initializer_list {inputIndices[1], inputIndices[0]} has a lifetime that only exists for the range of the constructor for ValueRange. Once we get to stack coloring we merge the stack slot for that element with another stack slot and then it gets overwritten which corrupts transposedIndices" The changes below prevents the corruption of transposedIndices and passes the test case. Full diff: https://github.com/llvm/llvm-project/pull/78435.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp b/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
index 5f065056bac00c..c15ea737366c71 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
@@ -50,7 +50,8 @@ class TransposeAsElementalConversion
auto genKernel = [&array](mlir::Location loc, fir::FirOpBuilder &builder,
mlir::ValueRange inputIndices) -> hlfir::Entity {
assert(inputIndices.size() == 2 && "checked in TransposeOp::validate");
- mlir::ValueRange transposedIndices{{inputIndices[1], inputIndices[0]}};
+ const std::initializer_list<mlir::Value> initList = {inputIndices[1], inputIndices[0]};
+ mlir::ValueRange transposedIndices(initList);
hlfir::Entity element =
hlfir::getElementAt(loc, builder, array, transposedIndices);
hlfir::Entity val = hlfir::loadTrivialScalar(loc, builder, element);
diff --git a/flang/test/HLFIR/simplify-hlfir-intrinsics.fir b/flang/test/HLFIR/simplify-hlfir-intrinsics.fir
index b63ddf17515281..aeea8bfc973266 100644
--- a/flang/test/HLFIR/simplify-hlfir-intrinsics.fir
+++ b/flang/test/HLFIR/simplify-hlfir-intrinsics.fir
@@ -1,6 +1,3 @@
-// XFail the following test case on AIX due to potential miscompilation
-// TODO: Crash fir-opt on AIX
-// XFAIL: system-aix
// RUN: fir-opt --simplify-hlfir-intrinsics %s | FileCheck %s
// box with known extents
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice find.
Please could you change the commit title/message. From what I can tell, stack merging or anything AIX specific are not the issues here (the problem just happens to only currently show up under these conditions). This fixes undefined behavior on all platforms.
As @stefanp-ibm said, mlir::ValueRange
does not own the data it allows iteration over. Therefore, once the initializer_list goes out of scope (after the constructor returns), any access to the pointed-to memory (via the ValueRange
) is unsafe.
The fix is correct because it extends the lifetime of the initializer_list
to last as long as the ValueRange
.
When running the
flang/test/HLFIR/simplify-hlfir-intrinsics.fir
test case on AIX we encounter issues building op as they are not found in the mlir context:The issue is caused by the "Merge disjoint stack slots" pass and the error is not present if the source is built with
-mllvm --no-stack-coloring
Thanks to investigation by @stefanp-ibm we found that "the initializer_list {inputIndices[1], inputIndices[0]} has a lifetime that only exists for the range of the constructor for ValueRange. Once we get to stack coloring we merge the stack slot for that element with another stack slot and then it gets overwritten which corrupts transposedIndices"
The changes below prevents the corruption of transposedIndices and passes the test case.