[PATCH] D136039: [DebugInfo] Fix potential CU mismatch for attachRangesOrLowHighPC

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - thanks for your patience with the review.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1016
                                                    LexicalScope *Scope) {
-  DIE &ScopeDIE = updateSubprogramScopeDIE(Sub);
+  DIE *SPDie = getOrCreateSubprogramDIE(Sub, includeMinimalInlineScopes());
+  DwarfCompileUnit *ContextCU =
----------------
DianQK wrote:
> dblaikie wrote:
> > Hmm, I'm not following - can you help me understand why this got pulled out of `updateSubprogramScopeDIE`?
> I need get `SPDie` to get `ContextCU` before `attachRangesOrLowHighPC`.
> The `attachRangesOrLowHighPC` call in `updateSubprogramScopeDIE` and later `createAndAddScopeChildren`.
> 
> Maybe I could use the following code:
> ```
> DIE *SPDie = getOrCreateSubprogramDIE(Sub, includeMinimalInlineScopes());
> auto *ContextCU = static_cast<DwarfCompileUnit *>(SPDie->getUnit());
> DIE &ScopeDIE = ContextCU->updateSubprogramScopeDIE(Sub);
> ```
> 
> I tried. It fails at `cross-cu-inlining-2.ll` (maybe because call `getOrCreateSubprogramDIE` in different CU?).
> 
> Dwarf before the change is:
> ```
> 0x00000038:       DW_TAG_subprogram [5] * (0x0000002b)
>                     DW_AT_low_pc [DW_FORM_addr]	(0x0000000000000010 ".text")
>                     DW_AT_high_pc [DW_FORM_data4]	(0x00000001)
>                     DW_AT_frame_base [DW_FORM_exprloc]	(DW_OP_reg7 RSP)
>                     DW_AT_name [DW_FORM_strp]	( .debug_str[0x00000036] = "bar")
>                     DW_AT_decl_file [DW_FORM_data1]	("A.swift")
>                     DW_AT_decl_line [DW_FORM_data1]	(21)
>                     DW_AT_external [DW_FORM_flag_present]	(true)
> ```
> After:
> ```
> 0x00000038:       DW_TAG_subprogram [5]   (0x0000002b)
>                     DW_AT_name [DW_FORM_strp]	( .debug_str[0x00000036] = "bar")
>                     DW_AT_decl_file [DW_FORM_data1]	("A.swift")
>                     DW_AT_decl_line [DW_FORM_data1]	(21)
>                     DW_AT_external [DW_FORM_flag_present]	(true)
> 
> 0x0000003f:       DW_TAG_subprogram [6] * (0x0000002b)
>                     DW_AT_low_pc [DW_FORM_addr]	(0x0000000000000010 ".text")
>                     DW_AT_high_pc [DW_FORM_data4]	(0x00000001)
>                     DW_AT_frame_base [DW_FORM_exprloc]	(DW_OP_reg7 RSP)
> ```
> 
> I think it's ok. But the old version is better?
> 
Right, the old version is better. It could be made to work. Something like this:
```
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index f2551e1f3419..b2fe2cf396e1 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -443,8 +443,14 @@ void DwarfCompileUnit::attachLowHighPC(DIE &D, const MCSymbol *Begin,
 // Find DIE for the given subprogram and attach appropriate DW_AT_low_pc
 // and DW_AT_high_pc attributes. If there are global variables in this
 // scope then create and insert DIEs for these variables.
-DIE &DwarfCompileUnit::updateSubprogramScopeDIE(const DISubprogram *SP,
-                                                DIE *SPDie) {
+DIE &DwarfCompileUnit::updateSubprogramScopeDIE(const DISubprogram *SP) {
+  DIE *SPDie = getOrCreateSubprogramDIE(SP, includeMinimalInlineScopes());
+  auto *ContextCU = static_cast<DwarfCompileUnit *>(SPDie->getUnit());
+  return ContextCU->updateSubprogramScopeDIEImpl(SP, SPDie);
+}
+
+DIE &DwarfCompileUnit::updateSubprogramScopeDIEImpl(const DISubprogram *SP, DIE *SPDie) {
+
   SmallVector<RangeSpan, 2> BB_List;
   // If basic block sections are on, ranges for each basic block section has
   // to be emitted separately.
@@ -1013,9 +1019,8 @@ sortLocalVars(SmallVectorImpl<DbgVariable *> &Input) {
 
 DIE &DwarfCompileUnit::constructSubprogramScopeDIE(const DISubprogram *Sub,
                                                    LexicalScope *Scope) {
-  DIE *SPDie = getOrCreateSubprogramDIE(Sub, includeMinimalInlineScopes());
-  auto *ContextCU = static_cast<DwarfCompileUnit *>(SPDie->getUnit());
-  DIE &ScopeDIE = ContextCU->updateSubprogramScopeDIE(Sub, SPDie);
+  DIE &ScopeDIE = updateSubprogramScopeDIE(Sub);
+  auto *ContextCU = static_cast<DwarfCompileUnit *>(ScopeDIE.getUnit());
 
   if (Scope) {
     assert(!Scope->getInlinedAt());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
index 0b0d97e74bc4..d346c79b5f7b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
@@ -191,7 +191,8 @@ public:
   /// DW_AT_low_pc and DW_AT_high_pc attributes. If there are global
   /// variables in this scope then create and insert DIEs for these
   /// variables.
-  DIE &updateSubprogramScopeDIE(const DISubprogram *SP, DIE *SPDie);
+  DIE &updateSubprogramScopeDIE(const DISubprogram *SP);
+  DIE &updateSubprogramScopeDIEImpl(const DISubprogram *SP, DIE *D);
 
   void constructScopeDIE(LexicalScope *Scope, DIE &ParentScopeDIE);
 
```

But yeah, since this is the only caller, seems simpler to do it here as you've done. Thanks for walking me through it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136039/new/

https://reviews.llvm.org/D136039