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

[DianQK via Phabricator via llvm-commits]

DianQK marked 4 inline comments as done.
DianQK added a comment.

Thank you very much for your help!



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1016
                                                    LexicalScope *Scope) {
-  DIE &ScopeDIE = updateSubprogramScopeDIE(Sub);
+  DIE *SPDie = getOrCreateSubprogramDIE(Sub, includeMinimalInlineScopes());
+  DwarfCompileUnit *ContextCU =
----------------
dblaikie wrote:
> 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.
Thanks, I have a patch added a `constructSubprogramScopeDIEImplused` to reduce `ContextCU` using that was not published, Because I found some code add `*Impl` doing similar things.
But only two locations need `ContextCU`, so I removed this.

> But why did I forget to use this on `updateSubprogramScopeDIE`? I'm being foolish.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136039