[lldb][TypeSystemClang] Desugar an elaborated type before checking if it's a typedef or getting a typedefed type

[aleksandr.urakov]

Summary

Sometimes a result variable of some expression can be presented as an elaborated type. In this case the methods IsTypedefType() and GetTypedefedType() of SBType didn't work. This patch fixes that.

I didn't find the test for these API methods, so I added a basic test for this too.

[labath]

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

547

The name of this function is fairly misleading as it only desugars elaborated types (but not e.g. auto, decltypes, typeof expressions, etc).

Do you want to desugar those too? If yes, then you could call RemoveWrappingTypes while passing "typedef" as the thing-to-avoid.

[aleksandr.urakov]

Hello, Pavel! Thanks for the review. Yes, this method looks like a better fit, I just didn't notice it before.

[teemperor]

LGTM now beside some minor request for the test. Thanks for the patch!

lldb/test/API/lang/cpp/typedef/TestCppTypedef.py

34

I know we do this (sadly) really often in LLDB, but these static error messages are just not useful. If my expression fails and the error message is "expression failed" then that doesn't help me with debugging the issue (especially when it's the only thing some remote bot sends back to me after a commit).

You could do self.assertTrue(expr_result.IsValid(), "Expression failed with:" + expr_result.GetError().GetCString()) instead and then people see the actual compiler output in the error log. Same for the other expr evaluation below.

[labath]

lldb/test/API/lang/cpp/typedef/TestCppTypedef.py

34

One of these days, I'm going to write assertSuccess/assertFailure functions which know how to test&print SBError objects. But I'm fairly busy these days, so if someone wants to beat me to it, be my guest.

[aleksandr.urakov]

Thanks! Fixed. The only thing is that GetCString() can return nullptr, which leads to None in Python, so I made a simple wrapper for that.

I'm not very deep into the testing infrastructure, so I'm not sure that I am a right person to implement assertSuccess etc. Sorry about that.

[teemperor]

In D78697#2001682, @aleksandr.urakov wrote:

Thanks! Fixed. The only thing is that GetCString() can return nullptr, which leads to None in Python, so I made a simple wrapper for that.

I think GetCString should always return something when the expression fails unless something really goes wrong. I think the wrapper is a good idea for a more generic error handling code but in this case it just makes the test verbose and doesn't really add any new information (with and without the wrapper the program aborts with the same backtrace from what I can see. I would even say the concatenation with None error is a bit clearer in this situation, as we have a failure without an error string). Otherwise this is good to go.

I'm not very deep into the testing infrastructure, so I'm not sure that I am a right person to implement assertSuccess etc. Sorry about that.

Sure, I don't think that was meant to be a suggested enhancement for this specific line of patches.

[aleksandr.urakov]

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

[teemperor]

In D78697#2001751, @aleksandr.urakov wrote:

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

Oh true, totally missed that. I think we usually solved that by putting the assert in some 'if', but that's not much better than the wrapper. And this hopefully anyway goes away with the discussed utility method. So let's just leave it at that for now and no longer delay this patch.

LGTM, thanks for the quick turnaround!

[labath]

In D78697#2001751, @aleksandr.urakov wrote:

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

You should be able to use str(sb_error)

In D78697#2001735, @teemperor wrote:

I'm not very deep into the testing infrastructure, so I'm not sure that I am a right person to implement assertSuccess etc. Sorry about that.

Sure, I don't think that was meant to be a suggested enhancement for this specific line of patches.

yep.

[aleksandr.urakov]

Exactly! Removed unneeded wrapper.

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

static void ParseLangArgs(LangOptions &Opts, InputKind IK, const char *triple) {
   // FIXME: This is affected by other options (-fno-inline).
   Opts.NoInlineDefine = !Opt;
 
   // This is needed to allocate the extra space for the owning module
   // on each decl.
   Opts.ModulesLocalVisibility = 1;
 }
 
 TypeSystemClang::TypeSystemClang(llvm::StringRef name,
                                  llvm::Triple target_triple) {
   m_display_name = name.str();
   if (!target_triple.str().empty())
     SetTargetTriple(target_triple.str());
   // The caller didn't pass an ASTContext so create a new one for this
   // TypeSystemClang.
   CreateASTContext();
 }
if (!type)
     return false;
 
   return (GetTypeInfo(type, nullptr) & eTypeIsScalar) != 0;
 }
 
 bool TypeSystemClang::IsTypedefType(lldb::opaque_compiler_type_t type) {
   if (!type)
     return false;
-  return GetQualType(type)->getTypeClass() == clang::Type::Typedef;
+  return RemoveWrappingTypes(GetQualType(type), {clang::Type::Typedef})
+             ->getTypeClass() == clang::Type::Typedef;
 }
 
 bool TypeSystemClang::IsVoidType(lldb::opaque_compiler_type_t type) {
   if (!type)
     return false;
   return GetCanonicalQualType(type)->isVoidType();
 }
 
if (type) {
     return GetType(clang_ast.getTypedefType(decl));
   }
   return CompilerType();
 }
 
 CompilerType
 TypeSystemClang::GetTypedefedType(lldb::opaque_compiler_type_t type) {
   if (type) {
-    const clang::TypedefType *typedef_type =
-        llvm::dyn_cast<clang::TypedefType>(GetQualType(type));
+    const clang::TypedefType *typedef_type = llvm::dyn_cast<clang::TypedefType>(
+        RemoveWrappingTypes(GetQualType(type), {clang::Type::Typedef}));
     if (typedef_type)
       return GetType(typedef_type->getDecl()->getUnderlyingType());
   }
   return CompilerType();
 }
 
 // Create related types using the current type's AST
 

lldb/test/API/lang/cpp/typedef/Makefile

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

lldb/test/API/lang/cpp/typedef/TestCppTypedef.py

+"""
+Test that we can retrieve typedefed types correctly
+"""
+
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import decorators
+
+class TestCppTypedef(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test_typedef(self):
+        """
+            Test that we retrieve typedefed types correctly
+        """
+
+        # Build and run until the breakpoint
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.cpp")
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, "Set a breakpoint here", self.main_source_file)
+
+        # Get the current frame
+        frame = thread.GetSelectedFrame()
+
+        # First of all, check that we can get a typedefed type correctly in a simple case
+
+        expr_result = frame.EvaluateExpression("(SF)s")
+        self.assertTrue(expr_result.IsValid(), "Expression failed with: " + str(expr_result.GetError()))
+
+        typedef_type = expr_result.GetType();
+        self.assertTrue(typedef_type.IsValid(), "Can't get `SF` type of evaluated expression")
+        self.assertTrue(typedef_type.IsTypedefType(), "Type `SF` should be a typedef")
+
+        typedefed_type = typedef_type.GetTypedefedType()
+        self.assertTrue(typedefed_type.IsValid(), "Can't get `SF` typedefed type")
+        self.assertEqual(typedefed_type.GetName(), "S<float>", "Got invalid `SF` typedefed type")
+
+        # Check that we can get a typedefed type correctly in the case
+        # when an elaborated type is created during the parsing
+
+        expr_result = frame.EvaluateExpression("(SF::V)s.value")
+        self.assertTrue(expr_result.IsValid(), "Expression failed with: " + str(expr_result.GetError()))
+
+        typedef_type = expr_result.GetType();
+        self.assertTrue(typedef_type.IsValid(), "Can't get `SF::V` type of evaluated expression")
+        self.assertTrue(typedef_type.IsTypedefType(), "Type `SF::V` should be a typedef")
+
+        typedefed_type = typedef_type.GetTypedefedType()
+        self.assertTrue(typedefed_type.IsValid(), "Can't get `SF::V` typedefed type")
+        self.assertEqual(typedefed_type.GetName(), "float", "Got invalid `SF::V` typedefed type")

lldb/test/API/lang/cpp/typedef/main.cpp

+template<typename T>
+struct S {
+  typedef T V;
+
+  V value;
+};
+
+typedef S<float> SF;
+
+int main (int argc, char const *argv[]) {
+  SF s{ .5 };
+  return 0; // Set a breakpoint here
+}