This is an archive of the discontinued LLVM Phabricator instance.

Add the ability to see when a type in incomplete.
ClosedPublic

Authored by clayborg on Nov 17 2022, 9:52 PM.

Details

Summary

-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 "<incomplete type>" 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 "<incomplete type>". 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 @ 0x0000000100008028, member = 47)
(TwoAsMember) ::two_as_member = (two = member::Two @ 0x0000000100008040, member = 47)
(array::One [3]) ::array_of_one = ([0] = array::One @ 0x0000000100008068, [1] = array::One @ 0x0000000100008069, [2] = array::One @ 0x000000010000806a)
(array::Two [3]) ::array_of_two = ([0] = array::Two @ 0x0000000100008098, [1] = array::Two @ 0x0000000100008099, [2] = array::Two @ 0x000000010000809a)
(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 = <incomplete type>, member = 47)
(InheritsFromTwo) ::inherits_from_two = (Two = <incomplete type>, member = 47)
(OneAsMember) ::one_as_member = (one = <incomplete type>, member = 47)
(TwoAsMember) ::two_as_member = (two = <incomplete type>, member = 47)
(array::One[3]) ::array_of_one = ([0] = <incomplete type>, [1] = <incomplete type>, [2] = <incomplete type>)
(array::Two[3]) ::array_of_two = ([0] = <incomplete type>, [1] = <incomplete type>, [2] = <incomplete type>)
(ShadowedOne) ::shadowed_one = (func_shadow::One = <incomplete type>, 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 = <incomplete type> {}
  (int) member = 47
}
(InheritsFromTwo) ::inherits_from_two = {
  (Two) Two = <incomplete type> {}
  (int) member = 47
}
(OneAsMember) ::one_as_member = {
  (member::One) one = <incomplete type> {}
  (int) member = 47
}
(TwoAsMember) ::two_as_member = {
  (member::Two) two = <incomplete type> {}
  (int) member = 47
}
(array::One[3]) ::array_of_one = {
  (array::One) [0] = <incomplete type> {}
  (array::One) [1] = <incomplete type> {}
  (array::One) [2] = <incomplete type> {}
}
(array::Two[3]) ::array_of_two = {
  (array::Two) [0] = <incomplete type> {}
  (array::Two) [1] = <incomplete type> {}
  (array::Two) [2] = <incomplete type> {}
}
(ShadowedOne) ::shadowed_one = {
  (func_shadow::One) func_shadow::One = <incomplete type> {}
  (int) member = 47
}

Diff Detail

Event Timeline

clayborg created this revision.Nov 17 2022, 9:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 9:52 PM
Herald added a subscriber: mstorsjo. · View Herald Transcript
clayborg requested review of this revision.Nov 17 2022, 9:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 9:52 PM
clayborg edited the summary of this revision. (Show Details)Nov 17 2022, 9:52 PM

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.

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 = <incomplete type> {}
  (int) member = 47
}

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

aprantl added inline comments.Nov 18 2022, 7:51 PM
lldb/include/lldb/API/SBType.h
212

a complete? (missing word)

223

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

237

Why not IsIncomplete()?

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 <incomplete type> is a good message for the end users. (Forward-declared type, type missing from debug info, ...?)

lldb/source/Core/ValueObject.cpp
601

I don't ha

clayborg added inline comments.Nov 20 2022, 9:24 AM
lldb/include/lldb/API/SBType.h
212

yep, will remove the "a" here..

223

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.

237

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.

lldb/source/Core/ValueObject.cpp
601

I am open to suggestions on what people want this string to be.

clayborg updated this revision to Diff 476754.Nov 20 2022, 9:26 AM

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

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 <incomplete type> 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.

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

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

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

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

yinghuitan added inline comments.Nov 21 2022, 2:29 PM
lldb/source/Core/ValueObject.cpp
600

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

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

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.

lldb/source/Core/ValueObject.cpp
600

It will show up as the summary, no need to duplicate the functionality in another location

"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.)

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

clayborg updated this revision to Diff 477314.Nov 22 2022, 2:59 PM

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.

labath accepted this revision.Nov 23 2022, 5:59 AM

Okay, looks good to me.

This revision is now accepted and ready to land.Nov 23 2022, 5:59 AM
This revision was automatically updated to reflect the committed changes.

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<lldb_private::TypeSystem>’}
   99 |     return m_type_system->IsForcefullyCompleted(m_type);
      |                         ^~

Any clues about what’s missing?

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<lldb_private::TypeSystem>’}
   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