[PATCH] D143985: [DwarfDebug] Move emission of imported entities from beginModule() to endModule() (2/7)

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added a comment.

@dblaikie thank you for looking at this! Hope this will be the final and successful iteration of the patchset.

In D143985#4185516 <https://reviews.llvm.org/D143985#4185516>, @dblaikie wrote:

> Hmm - have you checked/could you check if debuggers (gdb and lldb at least) care about where a namespace-scoped imported entity appears in the DWARF? It looks like this patch would move namespace-scoped imported entities to the end of the DWARF, and I could imagine an implementation trying to do name lookup might try to consider only the imported entities that appear lexically before in the DWARF? (admittedly we haven't put enough information in the IR to get these things in the /correct/ place, but maybe before is more likely to be correct than after) - though of course the same bug might exist in function-local imported entities (& other function local names) - and similarly we don't have enough info or ways to insert things in the right order anyway...

I haven't consulted with the debuggers code itself, but a simple example showed that neither gdb nor lldb have problems with finding imported entities if the order gets changed:
The example:

  $ cat test.cpp
  namespace A {
    struct C { int c; };
    C N = {1};
  }
  
  using T = A::C;
  using A::N;
  
  T foo(int a) {
    N.c = a;
    return N;
  }
  
  int main() {
    T t = foo(0);
    return t.c;
  }

lldb:

  $ lldb -- ./a.out 
  (lldb) b  main
  Breakpoint 1: where = a.out`main + 15 at test.cpp:15:9, address = 0x000000000000115f
  (lldb) r
  Process 1814813 launched: 'llvm-project/build/a.out' (x86_64)
  Process 1814813 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
      frame #0: 0x000055555555515f a.out`main at test.cpp:15:9
     12  	}
     13  	
     14  	int main() {
  -> 15  	  T t = foo(0);
     16  	  return t.c;
     17  	}
  (lldb) p N
  (A::C) $0 = (c = 1)
  (lldb) image lookup -t T
  Best match found in llvm-project/build/a.out:
  id = {0x00000084}, name = "T", byte-size = 4, decl = test.cpp:6, compiler_type = "typedef T"
       typedef 'T': id = {0x00000031}, name = "C", qualified = "A::C", byte-size = 4, decl = test.cpp:2, compiler_type = "struct C {
      int c;
  }"

gdb:

  $ gdb --args ./a.out 
  (gdb) b main
  Breakpoint 1 at 0x115f: file test.cpp, line 15.
  (gdb) r
  Starting program: llvm-project/build/a.out 
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
  
  Breakpoint 1, main () at test.cpp:15
  15	  T t = foo(0);
  (gdb) p N
  $1 = {c = 1}
  (gdb) ptype N
  type = struct A::C {
      int c;
  }
  (gdb) ptype T
  type = struct A::C {
      int c;
  }

I'd expect consumers to not rely on debug entities ordering since DWARF standard doesn't guarantee anything about it.

In D143985#4185520 <https://reviews.llvm.org/D143985#4185520>, @dblaikie wrote:

> & also - if we're moving local imported entities into the function-local retainedNodes list, do we need to change this code that should just be left constructing global/namespace-scoped imported entities? Perhaps we can leave this code/ordering alone?

It is still need for non-local imported entities to get PR51501 fixed. We cannot refer to a proper subprogram (abstract or concrete DIE) until we get all subprograms processed. PR51501 is supposed to be fully fixed for both local and global imported entities in D144004 <https://reviews.llvm.org/D144004>.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1434-1439
+  for (DICompileUnit *CUNode : MMI->getModule()->debug_compile_units()) {
+    DwarfCompileUnit *CU = CUMap.lookup(CUNode);
+    // Skip empty DICompileUnits.
+    if (!CU)
+      continue;
+
----------------
dblaikie wrote:
> Could we iterate the CUMap directly - to avoid needing to do lookups? like teh old code - *p.first would be the DICompileUnit* CUNode?
Well, in D144007 I'm moving the check for an emppy CU and CU emission here (from `DwarfDebug::beingModule()`), so, finally, I have no choice but using the lookup here.
But, yes, I can leave this unchanged until D144007, which makes it clear why this is needed. Reverted to CUMap. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143985