[flang-commits] [flang] e6bef08 - [flang] Avoid double free in bufferize pass (#93922)

[via flang-commits]

Author: Valentin Clement (バレンタイン クレメン)
Date: 2024-05-31T08:23:27-07:00
New Revision: e6bef08e22ac52aca0d6364f972cfb49c483adb3

URL: https://github.com/llvm/llvm-project/commit/e6bef08e22ac52aca0d6364f972cfb49c483adb3
DIFF: https://github.com/llvm/llvm-project/commit/e6bef08e22ac52aca0d6364f972cfb49c483adb3.diff

LOG: [flang] Avoid double free in bufferize pass (#93922)

In some cases where we have an `hlfir.no_reassoc` operation, the
bufferization pass could not earse the hlfir.destroy op during the
`hlfir.associate` op conversion as show in the example below.

```
func.func @double_free(%arg0: !fir.boxchar<1>) {
  %c5 = arith.constant 5 : index
  %true = arith.constant true
  %0 = hlfir.as_expr %arg0 move %true : (!fir.boxchar<1>, i1) -> !hlfir.expr<!fir.char<1,?>>
  %1 = hlfir.no_reassoc %0 : !hlfir.expr<!fir.char<1,?>>
  %2:3 = hlfir.associate %1 typeparams %c5 {adapt.valuebyref} : (!hlfir.expr<!fir.char<1,?>>, index) -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>, i1)
  fir.call @noop(%2#0) : (!fir.boxchar<1>) -> ()
  hlfir.end_associate %2#1, %2#2 : !fir.ref<!fir.char<1,?>>, i1
  hlfir.destroy %0 : !hlfir.expr<!fir.char<1,?>>
  return
} 
func.func private @noop(!fir.boxchar<1>)
```

The bufferization pass is looking at uses of its source `%1` that is the
result of an `hlfir.no_reassoc` operation. In order to avoid double free
generation, also look at the indirection in presence of
`hlfir.no_reassoc`.

Added: 
    

Modified: 
    flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
    flang/test/HLFIR/bufferize01.fir

Removed: 
    


################################################################################
diff  --git a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
index dd4f4c42414f3..e292b562763cc 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
@@ -534,6 +534,11 @@ struct AssociateOpConversion
       mlir::Value firBase = hlfir::Entity{bufferizedExpr}.getFirBase();
       replaceWith(bufferizedExpr, firBase, mustFree);
       eraseAllUsesInDestroys(associate.getSource(), rewriter);
+      // Make sure to erase the hlfir.destroy if there is an indirection through
+      // a hlfir.no_reassoc operation.
+      if (auto noReassoc = mlir::dyn_cast_or_null<hlfir::NoReassocOp>(
+              associate.getSource().getDefiningOp()))
+        eraseAllUsesInDestroys(noReassoc.getVal(), rewriter);
       return mlir::success();
     }
     if (isTrivialValue) {

diff  --git a/flang/test/HLFIR/bufferize01.fir b/flang/test/HLFIR/bufferize01.fir
index 89af0c2766f94..02ac6076268af 100644
--- a/flang/test/HLFIR/bufferize01.fir
+++ b/flang/test/HLFIR/bufferize01.fir
@@ -143,3 +143,31 @@ fir.global linkonce @_QQclXce30ef70ff16a711a97719fb946c0b3d constant : !fir.char
   fir.has_value %0 : !fir.char<1,1>
 }
 func.func private @_FortranAPushArrayConstructorValue(!fir.llvm_ptr<i8>, !fir.box<none>) -> none attributes {fir.runtime}
+
+// -----
+
+// Test that only a single freemem is generated.
+
+func.func @double_free(%arg0: !fir.boxchar<1>) {
+  %c5 = arith.constant 5 : index
+  %true = arith.constant true
+  %0 = hlfir.as_expr %arg0 move %true : (!fir.boxchar<1>, i1) -> !hlfir.expr<!fir.char<1,?>>
+  %1 = hlfir.no_reassoc %0 : !hlfir.expr<!fir.char<1,?>>
+  %2:3 = hlfir.associate %1 typeparams %c5 {adapt.valuebyref} : (!hlfir.expr<!fir.char<1,?>>, index) -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>, i1)
+  fir.call @noop(%2#0) : (!fir.boxchar<1>) -> ()
+  hlfir.end_associate %2#1, %2#2 : !fir.ref<!fir.char<1,?>>, i1
+  hlfir.destroy %0 : !hlfir.expr<!fir.char<1,?>>
+  return
+} 
+func.func private @noop(!fir.boxchar<1>)
+
+// CHECK-LABEL: func.func @double_free(
+// CHECK-SAME:  %[[ARG0:.*]]: !fir.boxchar<1>) {
+// CHECK: %[[NO_REASSOC:.*]] = hlfir.no_reassoc %[[ARG0]] : !fir.boxchar<1>
+// CHECK: %[[BOX_ADDR:.*]] = fir.box_addr %[[NO_REASSOC]] : (!fir.boxchar<1>) -> !fir.ref<!fir.char<1,?>>
+// CHECK: fir.call @noop
+// CHECK: %[[CONV:.*]] = fir.convert %[[BOX_ADDR]] : (!fir.ref<!fir.char<1,?>>) -> !fir.heap<!fir.char<1,?>>
+// CHECK: fir.freemem %[[CONV]] : !fir.heap<!fir.char<1,?>>
+// CHECK-NOT: fir.freemem
+
+