Data formatters: Look through array element typedefs

[jarin]

Summary

Motivation: When formatting an array of typedefed chars, we would like to display the array as a string.

The string formatter currently does not trigger because the formatter lookup does not resolve typedefs for array elements (this behavior is inconsistent with pointers, for those we do look through pointee typedefs). This patch tries to make the array formatter lookup somewhat consistent with the pointer formatter lookup.

[teemperor]

I have some comments but otherwise this patch looks good to me. Thanks!

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/TestArrayTypedef.py

3

I rather have no comment than a generic one (it's kinda obvious what is tested here from the name).

14

I think we can mark this as NO_DEBUG_INFO_TESTCASE = True to only run this once.

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/main.cpp

2

We usually don't add license headers to test files.

lldb/source/API/SBType.cpp

218

I get that this is to preserve the previous SB API behavior but I think it's better if we keep this method a simple wrapper without extra functionality. That we return the canonical type seems like a bug for me, so it's IMHO fine to change this behavior.

lldb/source/DataFormatters/FormatManager.cpp

263

Maybe add a comment that we strip here typedefs of array element types.

267

Maybe add a comment that this is the original array type with the element type typedef desugared.

273

I know this is copied from a above but I think if this is more readable:

// this is not exactly the usual meaning of stripping typedefs.
const bool stripped_typedef = true;
GetPossibleMatches(
....
  stripped_typedef);

[teemperor]

Just FYI, I meant that lldb/source/API/SBType.cpp shouldn't be changed with this patch. I think it's fine that this patch will change the behavior (as the old behavior seems broken).

Also do you need someone to commit this for you?

[jarin]

I have addressed the comments, thanks for the quick review.

lldb/source/API/SBType.cpp

218

Unfortunately, returning the non-canonicalized type breaks bunch of test (see below). In any case, such change should be done in a separate patch.

lldb-api :: functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/list/TestDataFormatterStdList.py
lldb-api :: functionalities/data-formatter/refpointer-recursion/TestDataFormatterRefPtrRecursion.py
lldb-api :: functionalities/data-formatter/varscript_formatting/TestDataFormatterVarScriptFormatting.py

[jarin]

In D72133#1802727, @teemperor wrote:

Also do you need someone to commit this for you?

Yes, please (once we agree what to do with SBType.cpp).

[teemperor]

Interesting, I actually can't reproduce those failures on any of my machines. Can you post the error output you get?

But I agree that we can figure this out in a separate patch, so I'll give this another test run and then land it. Thanks for the patch!

[jarin]

Actually, there is something unusually flaky in my current checkout, so you might be right that not canonicalizing is harmless. It still makes more sense to land that separately.

[clayborg]

lldb/source/API/SBType.cpp

215–218

Why does getting the canonical type of an array type change the array element type? I would think it wouldn't matter but it obviously must?

lldb/source/DataFormatters/FormatManager.cpp

245–261

Wouldn't we want to add one for each typedef that has a formatter? Lets say we had:

typedef char MCHAR;
typedef MCHAR MMCHAR;

int main() {
  MMCHAR str[5] = "abcd";
  return 0;
}

if we had a special formatter for MCHAR that would maybe display MCHAR somehow special, would we be ok with always getting the standard C string formatter in this case? Seems like we should add one for each typedef that has a formatter and possibly also the base type?

lldb/source/Symbol/ClangASTContext.cpp

3946

If I ask an array type for its element type, I wouldn't expect all typedefs to be removed. This seems wrong, or it can become an option to this function as an extra param:

CompilerType
ClangASTContext::GetArrayElementType(lldb::opaque_compiler_type_t type, uint64_t *stride, bool get_canonical_element_type);

[clayborg]

So as long as the following are true from this patch I am ok:

• if I ask for the array element type of "str" in the test that was added, it should return "MCHAR". We shouldn't be removing any typedefs from the type. The user can call SBType::GetCanonicalType() if they need to (or equivalent with internal APIs)
• If there are no formatters for "MCHAR[]" we can fall back to "char[]".
• If there are no formatters for "MMCHAR[]" from my example we fall back to the _first_ array formatter that supports the array of typedefs, or back to the the array of canonical types. Only the first array based formatter should be returned as the desired formatter

This patch seems to redefine what getting the element type of an array is which seems wrong to me. We just need to make the code that uses these APIs smarter.

[jarin]

lldb/source/API/SBType.cpp

215–218

Removal of typedef for element type is the existing behavior that I have been trying preserve. Raphael already suggested we should not preserve it.

For example, I get the following output for my program (both without and with my patch):

Type:    MCHAR [10]
El type: char

Here is the script

import lldb, os

debugger = lldb.SBDebugger.Create()
debugger.SetAsync(False)
target = debugger.CreateTargetWithFileAndTargetTriple("df.out", \
  "x86_64-pc-linux")
main_bp = target.BreakpointCreateByLocation("df.cc", 11)
process = target.LaunchSimple(None, None, os.getcwd())
frame = process.selected_thread.GetSelectedFrame()
var_a = process.selected_thread.GetSelectedFrame().GetValueForVariablePath("a")
print("Type:    " + var_a.GetType().GetName())
print("El type: " + var_a.GetType().GetArrayElementType().GetName())

If we remove the canonicalization here, we will indeed get the behavior that you want:

Type:    MCHAR [10]
El type: MCHAR

lldb/source/DataFormatters/FormatManager.cpp

245–261

This does not work as expected with my patch, let me investigate why.

lldb/source/Symbol/ClangASTContext.cpp

3946

The original code for ClangASTContext::GetArrayElementType did remove the typedefs. With my change ClangASTContext::GetArrayElementType does not remove typedefs anymore.

[teemperor]

In D72133#1806822, @clayborg wrote:

So as long as the following are true from this patch I am ok:

• if I ask for the array element type of "str" in the test that was added, it should return "MCHAR". We shouldn't be removing any typedefs from the type. The user can call SBType::GetCanonicalType() if they need to (or equivalent with internal APIs)
• If there are no formatters for "MCHAR[]" we can fall back to "char[]".
• If there are no formatters for "MMCHAR[]" from my example we fall back to the _first_ array formatter that supports the array of typedefs, or back to the the array of canonical types. Only the first array based formatter should be returned as the desired formatter

This patch seems to redefine what getting the element type of an array is which seems wrong to me. We just need to make the code that uses these APIs smarter.

This patch is moving the API closer to what you describe. The current behavior is that we strip the typedefs. The behavior we expect to get is that we don't strip the typedefs (which is what you describe). This patch moves the internal API closer to the expected behavior. The public SBAPI will be changed in a follow-up patch to be closer to the expected behavior. So no part of this patch is about adding any implicit stripping of typedefs which I think we all agree is unexpected and bad behavior. The only reason why we add the canonical type conversion to the SBAPI is to keep the bad SBAPI behavior for now (but this doesn't add any new typedef stripping, it just keeps the old one around for the SBAPI).

So from what I can tell this patch is improving the behavior by making it closer to what Greg describes (and what is correct), so it should land IMHO. The remaining existing typedef stripping can be fixed later (unless Greg feels strongly about fixing all typedef stripping it in this patch which is fine by me).

lldb/source/API/SBType.cpp

215–218

Why does getting the canonical type of an array type change the array element type? I would think it wouldn't matter but it obviously must?

It does matter because that's how canonical types work, see the Clang documentation in ASTContext.h:

/// The non-canonical version of a type may have many "decorated" versions of
/// types.  Decorators can include typedefs, 'typeof' operators, etc. The
/// returned type is guaranteed to be free of any of these, allowing two
/// canonical types to be compared for exact equality with a simple pointer
/// comparison.

lldb/source/DataFormatters/FormatManager.cpp

245–261

I tested this case when I reviewed this patch and IIRC this was caused by yet another call in GetArrayType (not sure though). But I don't think that's really blocking this patch as the old behaviour had the same flaw from what I can remember.

[jarin]

In D72133#1806822, @clayborg wrote:

So as long as the following are true from this patch I am ok:

• if I ask for the array element type of "str" in the test that was added, it should return "MCHAR". We shouldn't be removing any typedefs from the type. The user can call SBType::GetCanonicalType() if they need to (or equivalent with internal APIs)

As discussed before, this works exactly the same way without my patch.

• If there are no formatters for "MCHAR[]" we can fall back to "char[]".

This works. If there is formatter for MCHAR[], we use that formatter.

Note that if there is formatter for the element type (MCHAR), the command interpreter will not use that one because of the logic in ValueObjectPrinter::ShouldPrintChildren (which only expands children if the summary formatter says it has children or if there is no summary formatter). This might be a CLI usability regression.

• If there are no formatters for "MMCHAR[]" from my example we fall back to the _first_ array formatter that supports the array of typedefs, or back to the the array of canonical types. Only the first array based formatter should be returned as the desired formatter

This also works. Here is an example:

(lldb) b df.cc:7
Breakpoint 1: where = df.out`main + 61 at df.cc:7:3, address = 0x000000000040114d
(lldb) r
...
(lldb) p m
(MCHAR [10]) $0 = "m1"
(lldb) p mm
(MMCHAR [10]) $1 = "m2"
(lldb) type summary add --summary-string "-- mchar --" -x "MCHAR \[[0-9]+]"
(lldb) p m
(MCHAR [10]) $2 = -- mchar --
(lldb) p mm
(MMCHAR [10]) $3 = -- mchar --
(lldb) type summary add --summary-string "-- mmchar --" -x "MMCHAR \[[0-9]+]"
(lldb) p m
(MCHAR [10]) $6 = -- mchar --
(lldb) p mm
(MMCHAR [10]) $7 = -- mmchar --

Here is the program:

typedef char MCHAR;
typedef MCHAR MMCHAR;

int main() {
  MMCHAR mm[10] = "m2";
  MCHAR m[10] = "m1";
  return 0;  // break here
}

[clayborg]

Ok thanks for clarifying all my points! Looks good. And it would be nice to fix any locations that were canonicalizing types when they shouldn't be in a future patch.

[jarin]

Raphael, could you possibly land the patch for me? Thanks!

[teemperor]

In D72133#1813724, @jarin wrote:

Raphael, could you possibly land the patch for me? Thanks!

It's in my queue, but the green dragon bot (and my own CI) are currently dead due to a libc++ change that breaks the data formatters. I'll get that running and then I'll land this.

[teemperor]

Actually the failures are gmodules only so I'll land this now.

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/TestArrayTypedef.py

+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class ArrayTypedefTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_array_typedef(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here",
+            lldb.SBFileSpec("main.cpp", False))
+        self.expect("expr str", substrs=['"abcd"'])

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/main.cpp

+typedef char MCHAR;
+
+int main() {
+  MCHAR str[5] = "abcd";
+  return 0;  // break here
+}
+

lldb/source/API/SBType.cpp

return LLDB_RECORD_RESULT(
       SBType(TypeImplSP(new TypeImpl(m_opaque_sp->GetDereferencedType()))));
 }
 
 SBType SBType::GetArrayElementType() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBType, SBType, GetArrayElementType);
 
   if (!IsValid())
     return LLDB_RECORD_RESULT(SBType());
-  return LLDB_RECORD_RESULT(SBType(TypeImplSP(
-      new TypeImpl(m_opaque_sp->GetCompilerType(true).GetArrayElementType()))));
+  CompilerType canonical_type =
+      m_opaque_sp->GetCompilerType(true).GetCanonicalType();
+  return LLDB_RECORD_RESULT(
+      SBType(TypeImplSP(new TypeImpl(canonical_type.GetArrayElementType()))));
 }
 
 SBType SBType::GetArrayType(uint64_t size) {
   LLDB_RECORD_METHOD(lldb::SBType, SBType, GetArrayType, (uint64_t), size);
 
   if (!IsValid())
     return LLDB_RECORD_RESULT(SBType());
   return LLDB_RECORD_RESULT(SBType(TypeImplSP(

lldb/source/DataFormatters/FormatManager.cpp

if (compiler_type.IsPointerType()) {
     GetPossibleMatches(
         valobj, non_ptr_type,
         reason |
             lldb_private::eFormatterChoiceCriterionStrippedPointerReference,
         use_dynamic, entries, true, did_strip_ref, did_strip_typedef);
     if (non_ptr_type.IsTypedefType()) {
       CompilerType deffed_pointed_type =
           non_ptr_type.GetTypedefedType().GetPointerType();
+      const bool stripped_typedef = true;
       GetPossibleMatches(
           valobj, deffed_pointed_type,
           reason | lldb_private::eFormatterChoiceCriterionNavigatedTypedefs,
           use_dynamic, entries, did_strip_ptr, did_strip_ref,
-          true); // this is not exactly the usual meaning of stripping typedefs
+          stripped_typedef); // this is not exactly the usual meaning of
+                             // stripping typedefs
+    }
+  }
+
+  // For arrays with typedef-ed elements, we add a candidate with the typedef
+  // stripped.
+  uint64_t array_size;
+  if (compiler_type.IsArrayType(nullptr, &array_size, nullptr)) {
+    CompilerType element_type = compiler_type.GetArrayElementType();
+    if (element_type.IsTypedefType()) {
+      // Get the stripped element type and compute the stripped array type
+      // from it.
+      CompilerType deffed_array_type =
+          element_type.GetTypedefedType().GetArrayType(array_size);
+      const bool stripped_typedef = true;
+      GetPossibleMatches(
+          valobj, deffed_array_type,
+          reason | lldb_private::eFormatterChoiceCriterionNavigatedTypedefs,
+          use_dynamic, entries, did_strip_ptr, did_strip_ref,
+          stripped_typedef); // this is not exactly the usual meaning of
+                             // stripping typedefs
     }
   }
 
   for (lldb::LanguageType language_type :
        GetCandidateLanguages(valobj.GetObjectRuntimeLanguage())) {
     if (Language *language = Language::FindPlugin(language_type)) {
       for (ConstString candidate :
            language->GetPossibleFormattersMatches(valobj, use_dynamic)) {
         entries.push_back(
             {candidate,
              reason | lldb_private::eFormatterChoiceCriterionLanguagePlugin,
              did_strip_ptr, did_strip_ref, did_strip_typedef});
       }
     }
   }
 
   // try to strip typedef chains
   if (compiler_type.IsTypedefType()) {
     CompilerType deffed_type = compiler_type.GetTypedefedType();
     GetPossibleMatches(
         valobj, deffed_type,
         reason | lldb_private::eFormatterChoiceCriterionNavigatedTypedefs,

lldb/source/Symbol/ClangASTContext.cpp

 }
 
 // Creating related types
 
 CompilerType
 ClangASTContext::GetArrayElementType(lldb::opaque_compiler_type_t type,
                                      uint64_t *stride) {
   if (type) {
-    clang::QualType qual_type(GetCanonicalQualType(type));
+    clang::QualType qual_type(GetQualType(type));
 
     const clang::Type *array_eletype =
         qual_type.getTypePtr()->getArrayElementTypeNoTypeQual();
 
     if (!array_eletype)
       return CompilerType();
 
-    CompilerType element_type =
-        GetType(array_eletype->getCanonicalTypeUnqualified());
+    CompilerType element_type = GetType(clang::QualType(array_eletype, 0));
 
     // TODO: the real stride will be >= this value.. find the real one!
     if (stride)
       if (Optional<uint64_t> size = element_type.GetByteSize(nullptr))
         *stride = *size;
 
     return element_type;
   }