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

Mon Mar 13 12:53:19 PDT 2023

dblaikie accepted this revision.
dblaikie added a comment.

In D143985#4188825 <https://reviews.llvm.org/D143985#4188825>, @krisb wrote:

> @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:

Ah, thanks for checking!

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

Ah, OK - thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143985