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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
