[Lldb-commits] [PATCH] D113930: [LLDB][NativePDB] Fix function decl creation for class methods

Tue Dec 7 00:40:00 PST 2021

labath accepted this revision.
labath added a comment.

I am still not sure this is the right solution, but a) I don't know what *is* the right solution; and (b) I don't think this makes the situation worse; so I am hitting accept.

In D113930#3175022 <https://reviews.llvm.org/D113930#3175022>, @zequanwu wrote:

> In D113930#3174406 <https://reviews.llvm.org/D113930#3174406>, @labath wrote:
>
>> So I have a feeling that the real solution here is to avoid creating an ast entry for the function in the first phase. Then you should be able to create the full class declaration in the second phase, as you will have complete information there.
>
> If we don't create ast entries at `ParseFunctions`, non-class-member functions will not have ast decls. If we just create ast decls for non-class-member functions in `ParseFunctions`, we are expecting that `ParseTypes` is always called after `ParseFunctions` to complete the decl creation for member functions, which is not right.

I don't think this is as bad as it may sound at first. For lldb-test and ParseAllDebugSymbols purposes, I would even say its perfectly fine.

Overall I am not too worried about what happens in the `ParseAllDebugSymbols` scenario. I am more interested in what happens during "normal" operation when all of this is parsed lazily, in response to FindFunctions/FindTypes/... queries. (*) If everything works fine there then I think this would even be preferable, since you generally want to parse only the minimal amount of information. But I don't really even know how all of this works in DWARF, so I don't want to force you to redesign everything.

(*) A corollary of this is that I don't really trust that SymbolFileDWARF is doing "the right thing" in response to `ParseAllDebugSymbols`, since the main use case is lazy parsing, and `ParseAllDebugSymbols` only started being used relatively recently.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113930