[flang] Fixed out-of-bounds access in SimplifyIntrinsics.

[vzakhari]

When the mask is scalar, it is incorrect to cast it to
!fir.box<!fir.array<1xlogical<>>>, because the coordinate
operation will try to read the dim-1 stride from the box
to get the address of the first element. Even though
the stride value will be multiplied by 0, and does not matter,
it is still a read past the allocated box object.
Instead, we should just use box_addr to get the address
of the scalar mask.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: Slava Zakharin (vzakhari)

Changes

When the mask is scalar, it is incorrect to cast it to
!fir.box<!fir.array<1xlogical<>>>, because the coordinate
operation will try to read the dim-1 stride from the box
to get the address of the first element. Even though
the stride value will be multiplied by 0, and does not matter,
it is still a read past the allocated box object.
Instead, we should just use box_addr to get the address
of the scalar mask.

---

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

2 Files Affected:

• (modified) flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp (+1-9)
• (modified) flang/test/Transforms/simplifyintrinsics.fir (+4-8)

diff --git a/flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp b/flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
index dd4d1783dac37..4d25a02bf18ba 100644
--- a/flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
+++ b/flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
@@ -823,17 +823,9 @@ static void genRuntimeMinMaxlocBody(fir::FirOpBuilder &builder,
   if (maskRank == 0) {
     mlir::Type i1Type = builder.getI1Type();
     mlir::Type logical = maskElemType;
-    mlir::IndexType idxTy = builder.getIndexType();
-
-    fir::SequenceType::Shape singleElement(1, 1);
-    mlir::Type arrTy = fir::SequenceType::get(singleElement, logical);
-    mlir::Type boxArrTy = fir::BoxType::get(arrTy);
-    mlir::Value array = builder.create<fir::ConvertOp>(loc, boxArrTy, mask);
-
-    mlir::Value indx = builder.createIntegerConstant(loc, idxTy, 0);
     mlir::Type logicalRefTy = builder.getRefType(logical);
     mlir::Value condAddr =
-        builder.create<fir::CoordinateOp>(loc, logicalRefTy, array, indx);
+        builder.create<fir::BoxAddrOp>(loc, logicalRefTy, mask);
     mlir::Value cond = builder.create<fir::LoadOp>(loc, condAddr);
     mlir::Value condI1 = builder.create<fir::ConvertOp>(loc, i1Type, cond);
 
diff --git a/flang/test/Transforms/simplifyintrinsics.fir b/flang/test/Transforms/simplifyintrinsics.fir
index 1a66e077165b0..1cfda08e39694 100644
--- a/flang/test/Transforms/simplifyintrinsics.fir
+++ b/flang/test/Transforms/simplifyintrinsics.fir
@@ -1996,10 +1996,8 @@ func.func @_QPtestminloc_works1d_scalarmask_f64(%arg0: !fir.ref<!fir.array<10xf6
 // CHECK:           %[[OUTARR_IDX0:.*]] = arith.constant 0 : index
 // CHECK:           %[[OUTARR_ITEM0:.*]] = fir.coordinate_of %[[BOX_OUTARR]], %[[OUTARR_IDX0]] : (!fir.box<!fir.heap<!fir.array<1xi32>>>, index) -> !fir.ref<i32>
 // CHECK:           fir.store %[[INIT_OUT_IDX]] to %[[OUTARR_ITEM0]] : !fir.ref<i32>
-// CHECK:           %[[BOX_MASK:.*]] = fir.convert %[[BOX_MASK_NONE]] : (!fir.box<none>) -> !fir.box<!fir.array<1x!fir.logical<4>>>
-// CHECK:           %[[MASK_IDX0:.*]] = arith.constant 0 : index
-// CHECK:           %[[MASK_ITEM:.*]] = fir.coordinate_of %[[BOX_MASK]], %[[MASK_IDX0]] : (!fir.box<!fir.array<1x!fir.logical<4>>>, index) -> !fir.ref<!fir.logical<4>>
-// CHECK:           %[[MASK_LOGICAL:.*]] = fir.load %[[MASK_ITEM]] : !fir.ref<!fir.logical<4>>
+// CHECK:           %[[MASK_ADDR:.*]] = fir.box_addr %[[BOX_MASK_NONE]] : (!fir.box<none>) -> !fir.ref<!fir.logical<4>>
+// CHECK:           %[[MASK_LOGICAL:.*]] = fir.load %[[MASK_ADDR]] : !fir.ref<!fir.logical<4>>
 // CHECK:           %[[MASK:.*]] = fir.convert %[[MASK_LOGICAL]] : (!fir.logical<4>) -> i1
 // CHECK:           %[[INIT_RES:.*]] = fir.if %[[MASK]] -> (f64) {
 // CHECK:             %[[C_INDEX0:.*]] = arith.constant 0 : index
@@ -2574,10 +2572,8 @@ func.func @_QPtestmaxloc_works1d_scalarmask_f64(%arg0: !fir.ref<!fir.array<10xf6
 // CHECK:           %[[OUTARR_IDX0:.*]] = arith.constant 0 : index
 // CHECK:           %[[OUTARR_ITEM0:.*]] = fir.coordinate_of %[[BOX_OUTARR]], %[[OUTARR_IDX0]] : (!fir.box<!fir.heap<!fir.array<1xi32>>>, index) -> !fir.ref<i32>
 // CHECK:           fir.store %[[INIT_OUT_IDX]] to %[[OUTARR_ITEM0]] : !fir.ref<i32>
-// CHECK:           %[[BOX_MASK:.*]] = fir.convert %[[BOX_MASK_NONE]] : (!fir.box<none>) -> !fir.box<!fir.array<1x!fir.logical<4>>>
-// CHECK:           %[[MASK_IDX0:.*]] = arith.constant 0 : index
-// CHECK:           %[[MASK_ITEM:.*]] = fir.coordinate_of %[[BOX_MASK]], %[[MASK_IDX0]] : (!fir.box<!fir.array<1x!fir.logical<4>>>, index) -> !fir.ref<!fir.logical<4>>
-// CHECK:           %[[MASK_LOGICAL:.*]] = fir.load %[[MASK_ITEM]] : !fir.ref<!fir.logical<4>>
+// CHECK:           %[[MASK_ADDR:.*]] = fir.box_addr %[[BOX_MASK_NONE]] : (!fir.box<none>) -> !fir.ref<!fir.logical<4>>
+// CHECK:           %[[MASK_LOGICAL:.*]] = fir.load %[[MASK_ADDR]] : !fir.ref<!fir.logical<4>>
 // CHECK:           %[[MASK:.*]] = fir.convert %[[MASK_LOGICAL]] : (!fir.logical<4>) -> i1
 // CHECK:           %[[INIT_RES:.*]] = fir.if %[[MASK]] -> (f64) {
 // CHECK:             %[[C_INDEX0:.*]] = arith.constant 0 : index

[clementval]

LGTM

[ashermancinelli]

Thanks!