[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138259#3947410 , @mstorsjo wrote:

> This seems to have caused build errors with GCC:
>
>   ../tools/lldb/source/Symbol/CompilerType.cpp: In member function ‘bool 
> lldb_private::CompilerType::IsForcefullyCompleted() const’:
>   ../tools/lldb/source/Symbol/CompilerType.cpp:99:25: error: base operand of 
> ‘->’ has non-pointer type ‘const TypeSystemWP’ {aka ‘const 
> std::weak_ptr’}
>  99 | return m_type_system->IsForcefullyCompleted(m_type);
> | ^~
>
> Any clues about what’s missing?

Someone fixed the build issue for me! Things should be working


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

This seems to have caused build errors with GCC:

  ../tools/lldb/source/Symbol/CompilerType.cpp: In member function ‘bool 
lldb_private::CompilerType::IsForcefullyCompleted() const’:
  ../tools/lldb/source/Symbol/CompilerType.cpp:99:25: error: base operand of 
‘->’ has non-pointer type ‘const TypeSystemWP’ {aka ‘const 
std::weak_ptr’}
 99 | return m_type_system->IsForcefullyCompleted(m_type);
| ^~

Any clues about what’s missing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd941fceca8e9: Add the ability to see when a type in 
incomplete. (authored by clayborg).

Changed prior to commit:
  https://reviews.llvm.org/D138259?vs=477314=477548#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

Files:
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/API/SBType.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
  lldb/unittests/Symbol/TestTypeSystemClang.cpp

Index: lldb/unittests/Symbol/TestTypeSystemClang.cpp
===
--- lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -405,7 +405,7 @@
 
   RecordDecl *empty_base_decl = TypeSystemClang::GetAsRecordDecl(empty_base);
   EXPECT_NE(nullptr, empty_base_decl);
-  EXPECT_FALSE(TypeSystemClang::RecordHasFields(empty_base_decl));
+  EXPECT_FALSE(m_ast->RecordHasFields(empty_base_decl));
 
   // Test that a record with direct fields returns true
   CompilerType non_empty_base = m_ast->CreateRecordType(
@@ -419,7 +419,7 @@
   TypeSystemClang::GetAsRecordDecl(non_empty_base);
   EXPECT_NE(nullptr, non_empty_base_decl);
   EXPECT_NE(nullptr, non_empty_base_field_decl);
-  EXPECT_TRUE(TypeSystemClang::RecordHasFields(non_empty_base_decl));
+  EXPECT_TRUE(m_ast->RecordHasFields(non_empty_base_decl));
 
   std::vector> bases;
 
@@ -440,10 +440,9 @@
   m_ast->GetAsCXXRecordDecl(empty_derived.GetOpaqueQualType());
   RecordDecl *empty_derived_non_empty_base_decl =
   TypeSystemClang::GetAsRecordDecl(empty_derived);
-  EXPECT_EQ(1u, TypeSystemClang::GetNumBaseClasses(
+  EXPECT_EQ(1u, m_ast->GetNumBaseClasses(
 empty_derived_non_empty_base_cxx_decl, false));
-  EXPECT_TRUE(
-  TypeSystemClang::RecordHasFields(empty_derived_non_empty_base_decl));
+  EXPECT_TRUE(m_ast->RecordHasFields(empty_derived_non_empty_base_decl));
 
   // Test that a record with no direct fields, but fields in a virtual base
   // returns true
@@ -463,10 +462,10 @@
   m_ast->GetAsCXXRecordDecl(empty_derived2.GetOpaqueQualType());
   RecordDecl *empty_derived_non_empty_vbase_decl =
   TypeSystemClang::GetAsRecordDecl(empty_derived2);
-  EXPECT_EQ(1u, TypeSystemClang::GetNumBaseClasses(
+  EXPECT_EQ(1u, m_ast->GetNumBaseClasses(
 empty_derived_non_empty_vbase_cxx_decl, false));
   EXPECT_TRUE(
-  TypeSystemClang::RecordHasFields(empty_derived_non_empty_vbase_decl));
+  m_ast->RecordHasFields(empty_derived_non_empty_vbase_decl));
 }
 
 TEST_F(TestTypeSystemClang, TemplateArguments) {
@@ -980,4 +979,3 @@
   ModuleSP module = t.GetExeModule();
   EXPECT_EQ(module.get(), nullptr);
 }
-
Index: lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
===
--- lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
+++ lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
@@ -16,10 +16,12 @@
 type_ = exe.FindFirstType(name)
 self.trace("type_: %s"%type_)
 self.assertTrue(type_)
+self.assertTrue(type_.IsTypeComplete())
 base = type_.GetDirectBaseClassAtIndex(0).GetType()
 self.trace("base:%s"%base)
 self.assertTrue(base)
 self.assertEquals(base.GetNumberOfFields(), 0)
+self.assertFalse(base.IsTypeComplete())
 
 def _check_debug_info_is_limited(self, target):
 # Without other shared libraries we should only see the member declared
@@ -28,6 +30,100 @@
 self._check_type(target, "InheritsFromOne")
 self._check_type(target, "InheritsFromTwo")
 
+def _check_incomplete_frame_variable_output(self):
+# Check that the display of the "frame variable" output identifies the
+# incomplete types. Currently the expression parser will find the real
+# definition for a type when running an expression for any forcefully
+# completed types, but "frame variable" won't. I hope to fix this with
+# a follow up patch, but if we don't find the actual definition we
+# should clearly show this to the user by showing which types were
+# incomplete. So this will test verifies the expected output for such
+# types. We also need to verify the standard "frame variable" output
+# which will inline all of the members on one line, versus the full
+# output from "frame variable --raw" and a few other options.
+# self.expect("frame variable two_as_member", error=True,
+# substrs=["no member named 'one' in 

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Okay, looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 477314.
clayborg added a comment.

Remove SBType::IsTypeForcefullyCompleted() for now. Change 
SBType::IsTypeComplete() to return false for forcefully completed types. I did 
this only in the public API, not internally since we have the 
CompilerType::IsForcefullyCompleted() available and internal code can deal with 
this special case if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

Files:
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/API/SBType.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
  lldb/unittests/Symbol/TestTypeSystemClang.cpp

Index: lldb/unittests/Symbol/TestTypeSystemClang.cpp
===
--- lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -394,7 +394,7 @@
 
   RecordDecl *empty_base_decl = TypeSystemClang::GetAsRecordDecl(empty_base);
   EXPECT_NE(nullptr, empty_base_decl);
-  EXPECT_FALSE(TypeSystemClang::RecordHasFields(empty_base_decl));
+  EXPECT_FALSE(m_ast->RecordHasFields(empty_base_decl));
 
   // Test that a record with direct fields returns true
   CompilerType non_empty_base = m_ast->CreateRecordType(
@@ -408,7 +408,7 @@
   TypeSystemClang::GetAsRecordDecl(non_empty_base);
   EXPECT_NE(nullptr, non_empty_base_decl);
   EXPECT_NE(nullptr, non_empty_base_field_decl);
-  EXPECT_TRUE(TypeSystemClang::RecordHasFields(non_empty_base_decl));
+  EXPECT_TRUE(m_ast->RecordHasFields(non_empty_base_decl));
 
   std::vector> bases;
 
@@ -429,10 +429,9 @@
   m_ast->GetAsCXXRecordDecl(empty_derived.GetOpaqueQualType());
   RecordDecl *empty_derived_non_empty_base_decl =
   TypeSystemClang::GetAsRecordDecl(empty_derived);
-  EXPECT_EQ(1u, TypeSystemClang::GetNumBaseClasses(
+  EXPECT_EQ(1u, m_ast->GetNumBaseClasses(
 empty_derived_non_empty_base_cxx_decl, false));
-  EXPECT_TRUE(
-  TypeSystemClang::RecordHasFields(empty_derived_non_empty_base_decl));
+  EXPECT_TRUE(m_ast->RecordHasFields(empty_derived_non_empty_base_decl));
 
   // Test that a record with no direct fields, but fields in a virtual base
   // returns true
@@ -452,10 +451,10 @@
   m_ast->GetAsCXXRecordDecl(empty_derived2.GetOpaqueQualType());
   RecordDecl *empty_derived_non_empty_vbase_decl =
   TypeSystemClang::GetAsRecordDecl(empty_derived2);
-  EXPECT_EQ(1u, TypeSystemClang::GetNumBaseClasses(
+  EXPECT_EQ(1u, m_ast->GetNumBaseClasses(
 empty_derived_non_empty_vbase_cxx_decl, false));
   EXPECT_TRUE(
-  TypeSystemClang::RecordHasFields(empty_derived_non_empty_vbase_decl));
+  m_ast->RecordHasFields(empty_derived_non_empty_vbase_decl));
 }
 
 TEST_F(TestTypeSystemClang, TemplateArguments) {
@@ -969,4 +968,3 @@
   ModuleSP module = t.GetExeModule();
   EXPECT_EQ(module.get(), nullptr);
 }
-
Index: lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
===
--- lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
+++ lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
@@ -16,10 +16,12 @@
 type_ = exe.FindFirstType(name)
 self.trace("type_: %s"%type_)
 self.assertTrue(type_)
+self.assertTrue(type_.IsTypeComplete())
 base = type_.GetDirectBaseClassAtIndex(0).GetType()
 self.trace("base:%s"%base)
 self.assertTrue(base)
 self.assertEquals(base.GetNumberOfFields(), 0)
+self.assertFalse(base.IsTypeComplete())
 
 def _check_debug_info_is_limited(self, target):
 # Without other shared libraries we should only see the member declared
@@ -28,6 +30,100 @@
 self._check_type(target, "InheritsFromOne")
 self._check_type(target, "InheritsFromTwo")
 
+def _check_incomplete_frame_variable_output(self):
+# Check that the display of the "frame variable" output identifies the
+# incomplete types. Currently the expression parser will find the real
+# definition for a type when running an expression for any forcefully
+# completed types, but "frame variable" won't. I hope to fix this with
+# a follow up patch, but if we don't find the actual definition we
+# should clearly show this to the user by showing which types were
+# incomplete. So this will test verifies the expected output for such
+# types. We also need to verify the standard "frame variable" output
+# which will inline all of the members on one line, versus the full
+# output from "frame variable --raw" and a few other 

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138259#3943739 , @labath wrote:

> In D138259#3941859 , @clayborg 
> wrote:
>
>> In D138259#3941465 , @labath wrote:
>>
>>> In D138259#3941431 , @clayborg 
>>> wrote:
>>>
 "a type should be complete but isn't and you are losing information that 
 should have been available for you to debug".
>>>
>>> I agree, but there are still two (or more) ways to communicate that 
>>> information.
>>>
>>> 1. "this type is complete" + "actually, I'm just missing the debug info and 
>>> pretending it's complete"
>>> 2. "this type is incomplete" + "it is incomplete because I am missing its 
>>> debug info"
>>>
>>> My question is which method would be more useful to the user.
>>
>> Gotcha. We could change "bool SBType::IsTypeComplete()" to return false, and 
>> then add a new:
>>
>>   bool SBType::ShouldBeComplete();
>>
>> That would return true if IsTypeComplete() returned false because it was 
>> forcefully completed.
>
> That is the expected behavior, but we wouldn't need to implement it that way. 
> We could still implement it by calling the "is forcefully completed" metadata 
> function, and so it wouldn't force the completion of the type. So a `true` 
> return value would mean that the type has been forcefully completed, while a 
> return value of `false` would mean that the type is complete (not just 
> pretend-complete) **or** the completion of the type hasn't been attempted yet.

The current SBType::IsTypeComplete() will complete the type in order to figure 
out if it is actually complete. So although we can shortcut this for forcefully 
completed types, it wouldn't work for other types.

> Note that I'm not saying that this is how it needs to be implemented. 
> However, I think its worth thinking about this, given that we're adding a new 
> public interface and all. I think it comes down to the question of which 
> situation would be more/less confusing to the (SB) user
>
> - querying a member/base class of an object and finding that it's incomplete 
> (even though that is not possible in regular c++)
> - querying a member/base class of an object and finding that it's empty (even 
> though the truth is that we just don't know what it contains.)

Why don't I remove this API change from this patch to allow the patch to get 
into open source and we can revisit if we need to since we don't have any 
clients for this right now. But I do think it is a good idea to change the 
SBTType::IsTypeComplete() to return false for forcefully completed types, so I 
will make that change and test it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D138259#3941859 , @clayborg wrote:

> In D138259#3941465 , @labath wrote:
>
>> In D138259#3941431 , @clayborg 
>> wrote:
>>
>>> "a type should be complete but isn't and you are losing information that 
>>> should have been available for you to debug".
>>
>> I agree, but there are still two (or more) ways to communicate that 
>> information.
>>
>> 1. "this type is complete" + "actually, I'm just missing the debug info and 
>> pretending it's complete"
>> 2. "this type is incomplete" + "it is incomplete because I am missing its 
>> debug info"
>>
>> My question is which method would be more useful to the user.
>
> Gotcha. We could change "bool SBType::IsTypeComplete()" to return false, and 
> then add a new:
>
>   bool SBType::ShouldBeComplete();
>
> That would return true if IsTypeComplete() returned false because it was 
> forcefully completed.

That is the expected behavior, but we wouldn't need to implement it that way. 
We could still implement it by calling the "is forcefully completed" metadata 
function, and so it wouldn't force the completion of the type. So a `true` 
return value would mean that the type has been forcefully completed, while a 
return value of `false` would mean that the type is complete (not just 
pretend-complete) **or** the completion of the type hasn't been attempted yet.

Note that I'm not saying that this is how it needs to be implemented. However, 
I think its worth thinking about this, given that we're adding a new public 
interface and all. I think it comes down to the question of which situation 
would be more/less confusing to the (SB) user

- querying a member/base class of an object and finding that it's incomplete 
(even though that is not possible in regular c++)
- querying a member/base class of an object and finding that it's empty (even 
though the truth is that we just don't know what it contains.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138259#3942246 , @yinghuitan 
wrote:

> Also, do you have any plan to record this happening in statistics dump so 
> that our telemetry can report it?

I do, that will be a separate patch I will make once this patch is in.




Comment at: lldb/source/Core/ValueObject.cpp:600
+  // no member variables or member functions will be available.
+  if (GetCompilerType().IsForcefullyCompleted()) {
+  destination = "";

yinghuitan wrote:
> Is this shown in lldb-vscode? If so, can I suggest we add a VSCode test for 
> it if not too much work? 
It will show up as the summary, no need to duplicate the functionality in 
another location


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added a comment.

Also, do you have any plan to record this happening in statistics dump so that 
our telemetry can report it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added inline comments.



Comment at: lldb/source/Core/ValueObject.cpp:600
+  // no member variables or member functions will be available.
+  if (GetCompilerType().IsForcefullyCompleted()) {
+  destination = "";

Is this shown in lldb-vscode? If so, can I suggest we add a VSCode test for it 
if not too much work? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138259#3941465 , @labath wrote:

> In D138259#3941431 , @clayborg 
> wrote:
>
>> "a type should be complete but isn't and you are losing information that 
>> should have been available for you to debug".
>
> I agree, but there are still two (or more) ways to communicate that 
> information.
>
> 1. "this type is complete" + "actually, I'm just missing the debug info and 
> pretending it's complete"
> 2. "this type is incomplete" + "it is incomplete because I am missing its 
> debug info"
>
> My question is which method would be more useful to the user.

Gotcha. We could change "bool SBType::IsTypeComplete()" to return false, and 
then add a new:

  bool SBType::ShouldBeComplete();

That would return true if IsTypeComplete() returned false because it was 
forcefully completed.

The main issue with doing it this way is if you ask if type if it is complete 
by calling "bool SBType::IsTypeComplete()", you will force the type to complete 
itself to be able to answer the question. Right now if you have a GUI debugger 
and you just show the top level variables, we never need to complete any of the 
types unless the user turns them open in the GUI. The main reason for this API 
in SBType is for GUI debuggers to be able to indicate there is a problem to the 
user, but we don't want it to cause the debugger to realize types when it 
doesn't need to. The current IsTypeForcefullyCompleted() won't need to complete 
the type in order to figure out the result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D138259#3941431 , @clayborg wrote:

> "a type should be complete but isn't and you are losing information that 
> should have been available for you to debug".

I agree, but there are still two (or more) ways to communicate that information.

1. "this type is complete" + "actually, I'm just missing the debug info and 
pretending it's complete"
2. "this type is incomplete" + "it is incomplete because I am missing its debug 
info"

My question is which method would be more useful to the user.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138259#3940689 , @labath wrote:

> I have a somewhat high level question. Do we actually want 
> (SB)Type::IsTypeComplete to return true for these kinds of types?
>
> For clang's benefit, we have to pretend that the AST objects are complete 
> (and empty), but we're not similarly restricted in our own representations of 
> those types, so we could theoretically just answer `false` here (and possibly 
> have some additional method to indicate that the type is strange in some way).

If we have a forward declaration to a type, it is ok for that type to be 
incomplete. So for a variable like "struct Foo; Foo *foo = ...", it is ok for 
"Foo" to not be complete here. But if we have "Foo foo = ...;" then it isn't ok 
for Foo to not be complete. We need to tell the difference between "a type is 
incomplete and it is ok" and "a type should be complete but isn't and you are 
losing information that should have been available for you to debug".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have a somewhat high level question. Do we actually want 
(SB)Type::IsTypeComplete to return true for these kinds of types?

For clang's benefit, we have to pretend that the AST objects are complete (and 
empty), but we're not similarly restricted in our own representations of those 
types, so we could theoretically just answer `false` here (and possibly have 
some additional method to indicate that the type is strange in some way).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138259#3938841 , @aprantl wrote:

> Generally, I think this can be useful information. I don't have any better 
> suggestion, but I'd like to ask the room if we think that `` 
> is a good message for the end users. (`Forward-declared type`, `type missing 
> from debug info`, ...?)

"forward-declared type" might sound like it is normal for this type to be 
forward declared. "type missing from debug info" is better as it conveys 
exactly what is going on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 476754.
clayborg added a comment.

Changed the API comment describing the SBType::IsTypeForcefullyCompleted().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

Files:
  lldb/bindings/interface/SBType.i
  lldb/include/lldb/API/SBType.h
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/API/SBType.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
  lldb/unittests/Symbol/TestTypeSystemClang.cpp

Index: lldb/unittests/Symbol/TestTypeSystemClang.cpp
===
--- lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -394,7 +394,7 @@
 
   RecordDecl *empty_base_decl = TypeSystemClang::GetAsRecordDecl(empty_base);
   EXPECT_NE(nullptr, empty_base_decl);
-  EXPECT_FALSE(TypeSystemClang::RecordHasFields(empty_base_decl));
+  EXPECT_FALSE(m_ast->RecordHasFields(empty_base_decl));
 
   // Test that a record with direct fields returns true
   CompilerType non_empty_base = m_ast->CreateRecordType(
@@ -408,7 +408,7 @@
   TypeSystemClang::GetAsRecordDecl(non_empty_base);
   EXPECT_NE(nullptr, non_empty_base_decl);
   EXPECT_NE(nullptr, non_empty_base_field_decl);
-  EXPECT_TRUE(TypeSystemClang::RecordHasFields(non_empty_base_decl));
+  EXPECT_TRUE(m_ast->RecordHasFields(non_empty_base_decl));
 
   std::vector> bases;
 
@@ -429,10 +429,9 @@
   m_ast->GetAsCXXRecordDecl(empty_derived.GetOpaqueQualType());
   RecordDecl *empty_derived_non_empty_base_decl =
   TypeSystemClang::GetAsRecordDecl(empty_derived);
-  EXPECT_EQ(1u, TypeSystemClang::GetNumBaseClasses(
+  EXPECT_EQ(1u, m_ast->GetNumBaseClasses(
 empty_derived_non_empty_base_cxx_decl, false));
-  EXPECT_TRUE(
-  TypeSystemClang::RecordHasFields(empty_derived_non_empty_base_decl));
+  EXPECT_TRUE(m_ast->RecordHasFields(empty_derived_non_empty_base_decl));
 
   // Test that a record with no direct fields, but fields in a virtual base
   // returns true
@@ -452,10 +451,10 @@
   m_ast->GetAsCXXRecordDecl(empty_derived2.GetOpaqueQualType());
   RecordDecl *empty_derived_non_empty_vbase_decl =
   TypeSystemClang::GetAsRecordDecl(empty_derived2);
-  EXPECT_EQ(1u, TypeSystemClang::GetNumBaseClasses(
+  EXPECT_EQ(1u, m_ast->GetNumBaseClasses(
 empty_derived_non_empty_vbase_cxx_decl, false));
   EXPECT_TRUE(
-  TypeSystemClang::RecordHasFields(empty_derived_non_empty_vbase_decl));
+  m_ast->RecordHasFields(empty_derived_non_empty_vbase_decl));
 }
 
 TEST_F(TestTypeSystemClang, TemplateArguments) {
@@ -969,4 +968,3 @@
   ModuleSP module = t.GetExeModule();
   EXPECT_EQ(module.get(), nullptr);
 }
-
Index: lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
===
--- lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
+++ lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
@@ -16,10 +16,14 @@
 type_ = exe.FindFirstType(name)
 self.trace("type_: %s"%type_)
 self.assertTrue(type_)
+self.assertTrue(type_.IsTypeComplete())
+self.assertFalse(type_.IsTypeForcefullyCompleted())
 base = type_.GetDirectBaseClassAtIndex(0).GetType()
 self.trace("base:%s"%base)
 self.assertTrue(base)
 self.assertEquals(base.GetNumberOfFields(), 0)
+self.assertTrue(base.IsTypeComplete())
+self.assertTrue(base.IsTypeForcefullyCompleted())
 
 def _check_debug_info_is_limited(self, target):
 # Without other shared libraries we should only see the member declared
@@ -28,6 +32,100 @@
 self._check_type(target, "InheritsFromOne")
 self._check_type(target, "InheritsFromTwo")
 
+def _check_incomplete_frame_variable_output(self):
+# Check that the display of the "frame variable" output identifies the
+# incomplete types. Currently the expression parser will find the real
+# definition for a type when running an expression for any forcefully
+# completed types, but "frame variable" won't. I hope to fix this with
+# a follow up patch, but if we don't find the actual definition we
+# should clearly show this to the user by showing which types were
+# incomplete. So this will test verifies the expected output for such
+# types. We also need to verify the standard "frame variable" output
+# which will inline all of the members on one line, versus the full
+# output from "frame variable --raw" and a few other options.
+# self.expect("frame variable 

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/API/SBType.h:212
+  /// Returns true for types that were incomplete in the debug information but
+  /// should have been a complete. When the debugger constructs types, we must
+  /// have enough information to reconstruct a type in a language specific AST

aprantl wrote:
> a complete? (missing word)
yep, will remove the "a" here..



Comment at: lldb/include/lldb/API/SBType.h:223
+  /// of the debug information in a debug session, possibly even in another
+  /// executable or shared library's debug information. If we require a full
+  /// definition for a type but we can't find ony, we must forcefully complete

aprantl wrote:
> Could you replace all instances of "we" with something more concrete? It 
> sounds like this paragraph really describes behaviors of TypeSystemClang?
It is describing what -flimit-debug-info does, so yes, this is very clang 
specific. But right now we are showing these omitted types to the user as if 
they are fine, and they are not, so the user and or UI can and should convey 
this somehow. Not a great user experience when someone enables 
-flimit-debug-info, not on purpose as this is the default on non darwin 
targets, and they get a subpar debugging experience. We run into this all the 
time with linux and Android and the users think the debugger doesn't know how 
to show variables, so we need to let them know what is going on somehow. 

That being said, I can probably make this paragraph a lot simpler. Open to 
suggestions.



Comment at: lldb/include/lldb/API/SBType.h:237
+  /// need to be forcefully completed
+  bool IsTypeForcefullyCompleted();
+

aprantl wrote:
> Why not `IsIncomplete()`?
We already have IsTypeComplete() above, which indicates that something is a 
forward declaration. The IsTypeComplete() will return true for these forcefully 
completed types since they appear to be complete. I am open to suggestions on 
better naming, I was just forwarding our internal function names, but for the 
API a better name might make more sense. I can't think of any off hand.



Comment at: lldb/source/Core/ValueObject.cpp:601
+  if (GetCompilerType().IsForcefullyCompleted()) {
+  destination = "";
+  return true;

aprantl wrote:
> I don't ha
I am open to suggestions on what people want this string to be. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Generally, I think this can be useful information. I don't have any better 
suggestion, but I'd like to ask the room if we think that `` 
is a good message for the end users. (`Forward-declared type`, `type missing 
from debug info`, ...?)




Comment at: lldb/source/Core/ValueObject.cpp:601
+  if (GetCompilerType().IsForcefullyCompleted()) {
+  destination = "";
+  return true;

I don't ha


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/API/SBType.h:212
+  /// Returns true for types that were incomplete in the debug information but
+  /// should have been a complete. When the debugger constructs types, we must
+  /// have enough information to reconstruct a type in a language specific AST

a complete? (missing word)



Comment at: lldb/include/lldb/API/SBType.h:223
+  /// of the debug information in a debug session, possibly even in another
+  /// executable or shared library's debug information. If we require a full
+  /// definition for a type but we can't find ony, we must forcefully complete

Could you replace all instances of "we" with something more concrete? It sounds 
like this paragraph really describes behaviors of TypeSystemClang?



Comment at: lldb/include/lldb/API/SBType.h:237
+  /// need to be forcefully completed
+  bool IsTypeForcefullyCompleted();
+

Why not `IsIncomplete()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The next step for this is to modify the 
ValueObject::MaybeCalculateCompleteType() to see if we get a type that has been 
forcefully completed and if it has, replace it with the real type. But that 
will require that Module::FindFirstType() works reliably and it currently 
doesn't (fix for this issue is in this patch: https://reviews.llvm.org/D137900).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

We can see this in action with the before "var" output of:

  (InheritsFromOne) ::inherits_from_one = {
(int) member = 47
  }

And after, we see the "One" class definition and we can see that it is 
incomplete:

  (InheritsFromOne) ::inherits_from_one = {
(One) One =  {}
(int) member = 47
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Forgot to mention that this patch will force any empty base classes that were 
forcefully completed to show up in the variable display so the user can know 
that the type was forcefully completed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, JDevlieghere, jingham, yinghuitan, aprantl.
Herald added a subscriber: mstorsjo.
Herald added a project: All.
clayborg requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

-flimit-debug-info and other compiler options might end up removing debug info 
that is needed for debugging. LLDB marks these types as being forcefully 
completed in the metadata in the TypeSystem. These types should have been 
complete in the debug info but were not because the compiler omitted them to 
save space. When we can't find a suitable replacement for the type, we should 
let the user know that these types are incomplete to indicate there was an 
issue instead of just showing nothing for a type.

The solution is to display presented in this patch is to display "" as the summary for any incomplete types. If there is a summary string or 
function that is provided for a type, but the type is currently forcefully 
completed, the installed summary will be ignored and we will display 
"". This patch also exposes the ability to ask a SBType if it 
was forcefully completed with:

  bool SBType::IsTypeForcefullyCompleted();

This will allow the user interface for a debugger to also detect this issue and 
possibly mark the variable display up on some way to indicate to the user the 
type is incomplete.

To show how this is diplayed, we can look at the existing output first for the 
example source file from the file: 
lldb/test/API/functionalities/limit-debug-info/main.cpp

(lldb) frame variable inherits_from_one inherits_from_two one_as_member 
two_as_member array_of_one array_of_two shadowed_one
(InheritsFromOne) ::inherits_from_one = (member = 47)
(InheritsFromTwo) ::inherits_from_two = (member = 47)
(OneAsMember) ::one_as_member = (one = member::One @ 0x00018028, member 
= 47)
(TwoAsMember) ::two_as_member = (two = member::Two @ 0x00018040, member 
= 47)
(array::One [3]) ::array_of_one = ([0] = array::One @ 0x00018068, [1] = 
array::One @ 0x00018069, [2] = array::One @ 0x0001806a)
(array::Two [3]) ::array_of_two = ([0] = array::Two @ 0x00018098, [1] = 
array::Two @ 0x00018099, [2] = array::Two @ 0x0001809a)
(ShadowedOne) ::shadowed_one = (member = 47)
(lldb) frame variable --show-types inherits_from_one inherits_from_two 
one_as_member two_as_member array_of_one array_of_two shadowed_one
(InheritsFromOne) ::inherits_from_one = {

  (int) member = 47

}
(InheritsFromTwo) ::inherits_from_two = {

  (int) member = 47

}
(OneAsMember) ::one_as_member = {

  (member::One) one = {}
  (int) member = 47

}
(TwoAsMember) ::two_as_member = {

  (member::Two) two = {}
  (int) member = 47

}
(array::One [3]) ::array_of_one = {

  (array::One) [0] = {}
  (array::One) [1] = {}
  (array::One) [2] = {}

}
(array::Two [3]) ::array_of_two = {

  (array::Two) [0] = {}
  (array::Two) [1] = {}
  (array::Two) [2] = {}

}
(ShadowedOne) ::shadowed_one = {

  (int) member = 47

}

With this patch in place we can now see any classes that were forcefully 
completed to let us know that we are missing information:

(lldb) frame variable inherits_from_one inherits_from_two one_as_member 
two_as_member array_of_one array_of_two shadowed_one
(InheritsFromOne) ::inherits_from_one = (One = , member = 47)
(InheritsFromTwo) ::inherits_from_two = (Two = , member = 47)
(OneAsMember) ::one_as_member = (one = , member = 47)
(TwoAsMember) ::two_as_member = (two = , member = 47)
(array::One[3]) ::array_of_one = ([0] = , [1] = , [2] = )
(array::Two[3]) ::array_of_two = ([0] = , [1] = , [2] = )
(ShadowedOne) ::shadowed_one = (func_shadow::One = , member = 
47)
(lldb) frame variable --show-types inherits_from_one inherits_from_two 
one_as_member two_as_member array_of_one array_of_two shadowed_one
(InheritsFromOne) ::inherits_from_one = {

  (One) One =  {}
  (int) member = 47

}
(InheritsFromTwo) ::inherits_from_two = {

  (Two) Two =  {}
  (int) member = 47

}
(OneAsMember) ::one_as_member = {

  (member::One) one =  {}
  (int) member = 47

}
(TwoAsMember) ::two_as_member = {

  (member::Two) two =  {}
  (int) member = 47

}
(array::One[3]) ::array_of_one = {

  (array::One) [0] =  {}
  (array::One) [1] =  {}
  (array::One) [2] =  {}

}
(array::Two[3]) ::array_of_two = {

  (array::Two) [0] =  {}
  (array::Two) [1] =  {}
  (array::Two) [2] =  {}

}
(ShadowedOne) ::shadowed_one = {

  (func_shadow::One) func_shadow::One =  {}
  (int) member = 47

}


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138259

Files:
  lldb/bindings/interface/SBType.i
  lldb/include/lldb/API/SBType.h
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/API/SBType.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp