This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dzhidzhoev on Feb 14 2023, 12:35 AM.

Details

Summary

RFC https://discourse.llvm.org/t/rfc-dwarfdebug-fix-and-improve-handling-imported-entities-types-and-static-local-in-subprogram-and-lexical-block-scopes/68544

!Note! Extracted from the following patch for review purpose only, should
be squashed with the next patch (D144004) before committing.

Currently the back-end emits imported entities in DwarfDebug::beginModule().
However in case an imported declaration is a function, it must point to an
abstract subprogram if it exists (see PR51501). But in DwarfDebug::beginModule()
the DWARF generator doesn't have information to identify if an abstract
subprogram needs to be created.

Only by entering DwarfDebug::endModule() all subprograms are processed,
so it's clear which subprogram DIE should be referred to. Hence, the patch moves
the emission there.

The patch is need to fix PR51501, but it only does the preliminary
work. Since it changes the order of debug entities in emitted DWARF and
therefore affect many tests it's separated from the fix for the sake of
simplifying review.

Note that there are other issues with handling an imported declaration in
DwarfDebug::beginModule(). They are described in more details in D114705.

Diff Detail

Event Timeline

krisb created this revision.Feb 14 2023, 12:35 AM
krisb published this revision for review.Feb 17 2023, 6:26 AM
krisb edited the summary of this revision. (Show Details)
krisb added reviewers: dblaikie, jmmartinez, ellis.
krisb added projects: Restricted Project, debug-info.
ykhatav added a subscriber: ykhatav.Mar 3 2023, 8:56 AM
jmmartinez accepted this revision.Mar 10 2023, 6:09 AM
This revision is now accepted and ready to land.Mar 10 2023, 6:09 AM

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...

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1435–1444

Could we iterate the CUMap directly - to avoid needing to do lookups? like teh old code - *p.first would be the DICompileUnit* CUNode?

& 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?

krisb updated this revision to Diff 504604.Mar 13 2023, 5:29 AM

Apply comments & rebase.

krisb added a comment.Mar 13 2023, 5:29 AM

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

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.

& 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.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1435–1444

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.

dblaikie accepted this revision.Mar 13 2023, 12:53 PM

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

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:

Ah, thanks for checking!

& 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.

Ah, OK - thanks!

This revision was landed with ongoing or failed builds.Jun 14 2023, 5:47 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/DebugInfo/Generic/namespace.ll