[Lldb-commits] [PATCH] D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D124370#3474824 , @labath wrote:

> In 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" 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::clear()" 
>> and in another you would have "std::vector::erase()". In order to make 
>> the most complete definition of template types we need to find all 
>> "std::vector" 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", 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 v;` will produce a definition for the `vector` 
> 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` 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 
> 
>  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.

Yep, that mostly lines up with my understand.

Re: member function templates: These aren't the only things that vary between 
copies of a type in the DWARF. The other things that vary are implicit special 
member functions ({default,move,copy} ctor, dtor, {copy,move} assignment 
operator}) - these will only be present when instantiated - and nested types 
(though that's perhaps less of an issue for LLDB, not sure). I have made an 
argument that functions with "auto" return type should be treated this way 
(omitted unless the definition is instantiated and the return type is 
deduced/resolved) too - but for now they are not (they are always included, 
albeit with the "auto" return type on the declaration, and then the definition 
DIE has the real/deduced return type).

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

Should the cleanups you found for the previous test case be included in this 
patch (or a separate pre/post-co

[Lldb-commits] [PATCH] D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

@clayborg, @shafik, what do you make of this version?

In D124370#3481338 , @dblaikie wrote:

> Should the cleanups you found for the previous test case be included in this 
> patch (or a separate pre/post-commit?)? The description in the updated patch 
> sounds plausible - though delves into parts of lldb I'm not especially 
> qualified to make a firm assessment on.

I already checked those in --> rG089a1d9deba5 



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

[Lldb-commits] [PATCH] D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGae7fe65cf65d: [lldb/DWARF] Fix linking direction in 
CopyUniqueClassMethodTypes (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124370

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/incomplete-types/members/Makefile
  
lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py
  lldb/test/API/lang/cpp/incomplete-types/members/a.h
  lldb/test/API/lang/cpp/incomplete-types/members/f.cpp
  lldb/test/API/lang/cpp/incomplete-types/members/g.cpp
  lldb/test/API/lang/cpp/incomplete-types/members/main.cpp

Index: lldb/test/API/lang/cpp/incomplete-types/members/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/main.cpp
@@ -0,0 +1,9 @@
+#include "a.h"
+
+A::A() = default;
+void A::anchor() {}
+
+int main() {
+  A().f();
+  A().g();
+}
Index: lldb/test/API/lang/cpp/incomplete-types/members/g.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/g.cpp
@@ -0,0 +1,8 @@
+#include "a.h"
+
+int A::g() {
+  struct Ag {
+int a, b;
+  } ag{47, 42};
+  return ag.a + ag.b; // break here
+}
Index: lldb/test/API/lang/cpp/incomplete-types/members/f.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/f.cpp
@@ -0,0 +1,8 @@
+#include "a.h"
+
+int A::f() {
+  struct Af {
+int x, y;
+  } af{42, 47};
+  return af.x + af.y; // break here
+}
Index: lldb/test/API/lang/cpp/incomplete-types/members/a.h
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/a.h
@@ -0,0 +1,14 @@
+#ifndef A_H
+#define A_H
+
+class A {
+public:
+  A();
+  virtual void anchor();
+  int f();
+  int g();
+
+  int member = 47;
+};
+
+#endif
Index: lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py
@@ -0,0 +1,32 @@
+"""
+Test situations where we don't have a definition for a type, but we have (some)
+of its member functions.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCppIncompleteTypeMembers(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here",
+lldb.SBFileSpec("f.cpp"))
+
+# Sanity check that we really have to debug info for this type.
+this = self.expect_var_path("this", type="A *")
+self.assertEquals(this.GetType().GetPointeeType().GetNumberOfFields(),
+0, str(this))
+
+self.expect_var_path("af.x", value='42')
+
+lldbutil.run_break_set_by_source_regexp(self, "// break here",
+extra_options="-f g.cpp")
+self.runCmd("continue")
+
+self.expect_var_path("ag.a", value='47')
Index: lldb/test/API/lang/cpp/incomplete-types/members/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/Makefile
@@ -0,0 +1,10 @@
+CXX_SOURCES := main.cpp f.cpp g.cpp
+
+include Makefile.rules
+
+# Force main.cpp to be built with no debug information
+main.o: CFLAGS = $(CFLAGS_NO_DEBUG)
+
+# And force -flimit-debug-info on the rest.
+f.o: CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)
+g.o: CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1040,10 +1040,8 @@
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  bool alternate_defn = false;
   if (class_type->GetID() != decl_ctx_die.GetID() ||
   IsClangModuleFwdDecl(decl_ctx_die)) {
-alternate_defn = true;
 
 // We uniqued the parent class of this function to another
 // class so we now need to associate all dies under
@@ -1112,7 +1110,7 @@
 CompilerType class_opaque_type =
 class_type->GetForwardCompilerType();
 if (TypeSystemClang::IsCXXClassType(class_opaque_type)) {
-  if (class_opaque_type.IsBeingDefined() || alternate_defn) {
+  if (class_opaque_type.IsB