Page MenuHomePhabricator

[lldb-mi] display summary for simple types + refactor (use lldb formatting for all cases)
ClosedPublic

Authored by evgeny777 on Oct 16 2015, 2:39 AM.

Details

Summary

Current revision do not use lldb type summaries for simple types with no children (like function pointers). So this patch makes MI use lldb type summaries for evaluation of all types of objects, so MI own formatters are no longer needed.

Diff Detail

Event Timeline

evgeny777 updated this revision to Diff 37489.Oct 16 2015, 2:39 AM
evgeny777 retitled this revision from to [lldb-mi] display summary for simple types + refactor (use lldb formatting for all cases).
evgeny777 updated this object.
evgeny777 added subscribers: lldb-commits, KLapshin.
tools/lldb-mi/MICmnLLDBDebugger.cpp
37

I would definitely not stop the revision for this but I wonder if it would make sense to try and discover whether "char" is signed or unsigned from the type itself?

841

Should you also cover "signed char" and "unsigned char" here?

evgeny777 added inline comments.Oct 16 2015, 10:33 AM
tools/lldb-mi/MICmnLLDBDebugger.cpp
841

Hmm. I thought that if regex is false, exact match will be done, won't it? If yes than simple char type should be signed, right?

evgeny777 added inline comments.Oct 16 2015, 11:04 AM
tools/lldb-mi/MICmnLLDBDebugger.cpp
37

To my understanding this is not needed (see comment below)

841

unsigned - not. signed -yes. One question: if I register summary for "char" - it will not be called for "unsigned char" and "signed char", right?
If so I will need adding "signed char" and no checks for signed/unsigned inside summary provider are required, correct?

tools/lldb-mi/MICmnLLDBDebugger.cpp
841

I think the signedness of char depends on the implementation. Which means that "char" will cover one of them, but not the other. Which is why I was suggesting adding "char", "signed char" and "unsigned char". Just to cover all bases.

841

Correct. It will only be called for an exactly matching type name. We strip "useless" qualifiers like "volatile" and "restrict" before doing format matching. But, for obvious reasons, not signed and unsigned.

Well, if you add separate formatters for signed char vs. unsigned char, then they will know of course
The one for plain "char" might still need to figure it out though. Because given just "char", the signedness depends on the underlying compiler. I know internally we have support for this. If we don't in the SB API, we may need to add support. But that seems like it can be done in a subsequent revision.

abidh edited edge metadata.Oct 19 2015, 3:29 AM

Please also indicate if a revision is dependent on some other revisions that has not yet been committed as this one is on D13657.

tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
126–127

As the condition is changed, can you explain a little bit in which scenario, you want to pass true/false here for first parameter.

131–132

Same as above.

evgeny777 added inline comments.Oct 19 2015, 4:05 AM
tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
126–127

This is "get summary whenever possible, unless explicitly told otherwise".
For example this change of condition allows getting summary for function pointers (currently only hexadecimal pointer value is printed).

So false is only passed when character type is evaluated and m_bHandleCharType is false. I do not fully understand why this boolean flag exists - may be performance reasons?

131–132

In case one day someone create summary provider for some pointer type which is not char*, wchar_t* and related, it will be handled by MI correctly. The only exception is if we have char pointer and explicitly told not to handle char type

evgeny777 updated this revision to Diff 37733.Oct 19 2015, 4:08 AM
evgeny777 edited edge metadata.

Review updated to reflect Enrico comments regarding signed/unsigned char.
Now all character types are evaluated as ASCII code (signed or unsigned) + value.

Additional method introduced to SBValue to check if type is signed or unsigned integer

include/lldb/API/SBValue.h
372

Greg pointed me to SBType::GetBasicType() - this will return to you one of

eBasicTypeSignedChar,
eBasicTypeUnsignedChar,

such that you don't need to add new API to get this information

Also, even if this were necessary, it would go on SBType, definitely not SBValue

evgeny777 added inline comments.Oct 19 2015, 12:07 PM
include/lldb/API/SBValue.h
372

This method exists in ValueObject, so I thought it makes sense having it in SBValue also. But ok, let's use GetBasicType

evgeny777 updated this revision to Diff 37888.Oct 20 2015, 9:15 AM

Update according to suggestions from granata.enrico

abidh accepted this revision.Oct 20 2015, 10:23 AM
abidh edited edge metadata.

Apart from some inline comments, looks ok to me.

tools/lldb-mi/MICmnLLDBDebugger.cpp
831

Please change it to 'true' and 'false'.

This revision is now accepted and ready to land.Oct 20 2015, 10:23 AM
This revision was automatically updated to reflect the committed changes.