[Lldb-commits] [PATCH] D124370: [lldb] Fix PR54761

Tue Apr 26 08:36:04 PDT 2022

labath planned changes to this revision.
labath added a comment.

In D124370#3472394 <https://reviews.llvm.org/D124370#3472394>, @clayborg wrote:

> So I believe the reason we need to be able to add methods to a class is for templates. Templates in DWARF are really poorly emitted all in the name of saving space. There is no full definition of template classes that get emitted, just a class definition with _only_ the methods that the user used in the current compile unit. DWARF doesn't really support emitting a templatized definition of a class in terms of a type T, it will always emit instantiations of the type T directly. So in LLDB we must create a template class like "std::vector<int>" and say it has no member functions, and then add each member function as a type specific specialization due to how the types must be created in the clang::ASTContext.
>
> in one DWARF compile unit you end up having say "std::vector<int>::clear()" and in another you would have "std::vector<int>::erase()". In order to make the most complete definition of template types we need to find all "std::vector<int>" definitions and manually add any method that we haven't already seen, otherwise we end up not being able to call methods that might exist. Of course calling template functions is already fraught with danger since most are inlined if they come from the STL, but if the user asks for the class definition for "std::vector<int>", we should show all that we can. Can you make sure this patch doesn't hurt that functionality? We must have a test for it.
>
> So we can't just say we will accept just one class definition if we have template types. Given this information, let me know what you think of your current patch

I think I'm confused. I know we're doing some funny stuff with class templates, and I'm certain I don't fully understand how it works. However, I am also pretty sure your description is not correct either.  A simple snippet like `std::vector<int> v;` will produce a definition for the `vector<int>` class, and that definition will include the declarations for `erase`, `clear`, and any other non-template member function, even though the code definitely does not call them. And this makes perfect sense to me given how DWARF (and clang) describes only template instantiations -- so the template instantiation `vector<int>` is treated the same way as a class `vector_int` with the same interface.

What you're describing resembles the treatment of template member functions -- those are only emitted in the compile units that they are used. This is very unfortunate for us, though I think it still makes sense from the DWARF perspective. However:
a) In line with the previous paragraph -- this behavior is the same for template **class** instantiations and regular classes. IOW, the important question is whether the method is a template, not whether its enclosing class is
b) (precisely for this reason) we currently  completely ignore <https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L980> member function templates when parsing classes
Given all of that,  I don't see how templates are relevant for this patch. *However*, please read on.

In D124370#3472555 <https://reviews.llvm.org/D124370#3472555>, @dblaikie wrote:

> I take it no tests fail upon removing this condition? Any hints in version history about its purpose?

No tests broke with that. My first archaeology expedition did not find anything, but now I've tried it once more, and I found D13224 <https://reviews.llvm.org/D13224>. That diff is adding this condition (to support -flimit-debug-info, no less) and it even comes with a test. That test still exists, but it was broken over time (in several ways). Once I fix it so that it tests what it was supposed to test, I see that my patch breaks it.

I'm now going to try to figure out how does that code actually work...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124370