[flang][debug] Set scope of internal functions correctly.

[abidh]

The functions internal to subroutine should have the scope set to the parent function. This allows a user to evaluate local variables of parent function when control is stopped in the child.

Fixes #96314

[llvmbot]

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

Author: Abid Qadeer (abidh)

Changes

The functions internal to subroutine should have the scope set to the parent function. This allows a user to evaluate local variables of parent function when control is stopped in the child.

Fixes #96314

---

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

2 Files Affected:

• (modified) flang/lib/Optimizer/Transforms/AddDebugInfo.cpp (+21-1)
• (added) flang/test/Transforms/debug-96314.fir (+26)

diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 8bb24fb6c8078..7c9555dd737ab 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -299,9 +299,29 @@ void AddDebugInfoPass::runOnOperation() {
           subprogramFlags | mlir::LLVM::DISubprogramFlags::Definition;
     }
     unsigned line = getLineFromLoc(l);
-    if (!result.second.modules.empty())
+    if (fir::isInternalProcedure(funcOp)) {
+      // For contained functions, the scope is the parent subroutine.
+      mlir::SymbolRefAttr sym = mlir::cast<mlir::SymbolRefAttr>(
+          funcOp->getAttr(fir::getHostSymbolAttrName()));
+      if (sym) {
+        if (auto func = symbolTable.lookup<mlir::func::FuncOp>(
+                sym.getLeafReference())) {
+          // FIXME: Can there be situation where we process contained function
+          // before the parent?
+          if (debugInfoIsAlreadySet(func.getLoc())) {
+            if (auto fusedLoc = mlir::cast<mlir::FusedLoc>(func.getLoc())) {
+              if (auto spAttr =
+                      mlir::dyn_cast_if_present<mlir::LLVM::DISubprogramAttr>(
+                          fusedLoc.getMetadata()))
+                Scope = spAttr;
+            }
+          }
+        }
+      }
+    } else if (!result.second.modules.empty()) {
       Scope = getOrCreateModuleAttr(result.second.modules[0], fileAttr, cuAttr,
                                     line - 1, false);
+    }
 
     auto spAttr = mlir::LLVM::DISubprogramAttr::get(
         context, id, compilationUnit, Scope, funcName, fullName, funcFileAttr,
diff --git a/flang/test/Transforms/debug-96314.fir b/flang/test/Transforms/debug-96314.fir
new file mode 100644
index 0000000000000..e2d0f24a1105c
--- /dev/null
+++ b/flang/test/Transforms/debug-96314.fir
@@ -0,0 +1,26 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s -o - | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+  func.func @_QMhelperPmod_sub(%arg0: !fir.ref<i32> {fir.bindc_name = "a"} ) {
+    return
+  } loc(#loc1)
+  func.func private @_QMhelperFmod_subPchild1(%arg0: !fir.ref<i32> {fir.bindc_name = "b"} ) attributes {fir.host_symbol = @_QMhelperPmod_sub, llvm.linkage = #llvm.linkage<internal>} {
+    return
+  } loc(#loc2)
+  func.func @global_sub_(%arg0: !fir.ref<i32> {fir.bindc_name = "n"} ) attributes {fir.internal_name = "_QPglobal_sub"} {
+    return
+  } loc(#loc3)
+  func.func private @_QFglobal_subPchild2(%arg0: !fir.ref<i32> {fir.bindc_name = "c"}) attributes {fir.host_symbol = @global_sub_, llvm.linkage = #llvm.linkage<internal>} {
+    return
+  } loc(#loc4)
+}
+
+#loc1 = loc("test.f90":5:1)
+#loc2 = loc("test.f90":15:1)
+#loc3 = loc("test.f90":25:1)
+#loc4 = loc("test.f90":35:1)
+
+// CHECK-DAG: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "mod_sub"{{.*}}>
+// CHECK-DAG: #llvm.di_subprogram<{{.*}}scope = #[[SP1]], name = "child1"{{.*}}>
+// CHECK-DAG: #[[SP2:.*]] = #llvm.di_subprogram<{{.*}}linkageName = "global_sub_"{{.*}}>
+// CHECK-DAG: #llvm.di_subprogram<{{.*}}scope = #[[SP2]], name = "child2"{{.*}}>

[tblah]

I believe that the current lowering implementation visits (and creates a function declaration) for the parent before the contained function:

llvm-project/flang/lib/Lower/Bridge.cpp

Line 488 in cc2fb58

// Declare internal procedures

In MLIR, the module is just an operation with a nested region like any other operation. So I presume the order of functions inside of a module is stable (in the same way that the order of operations nested inside of a function is well defined).

However, I wonder if some current or future pass might erase and re-create functions (thereby possibly shuffling their order). Perhaps all of this could be abstracted out into a function which calls itself recursively if the parent is not yet processed. The recursion will be bounded by the level of function nesting and for the reasons outlined above won't happen at all in most cases.

[abidh]

Thanks for the review. I have moved the function processing in a separate function and now we can make sure that parent is processed before contained subroutine.

[tblah]

LGTM, thanks for the update!

[pawosm-arm]

/cherry-pick 626022b

[llvmbot]

/pull-request #100727