Fix Reference case for TypeSystemClang::GetChildCompilerTypeAtIndex(...) to avoid possible invalid cast

[shafik]

Summary

D103532 modified this case to preserve type sugar but we can end up with cases where the cast is not valid. I modified the code to use GetLValueReferenceType(type)/GetRValueReferenceType(type) respectively.

In the case being tested in the test case we end with the following type:

TypedefType 0x7f8a710202f0 'std::__compressed_pair_elem<struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep, 0, false>::const_reference' sugar
|-Typedef 0x7f8a71020280 'const_reference'
`-LValueReferenceType 0x7f8a71020250 'const struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep &'
...

which can't be cast to ReferenceType.

[aprantl]

That looks obviously correct, thanks!

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

6506

maybe use = (parent_type_class == clang::Type::LValueReference) ? GetLValueReferenceType(type).GetPointeeType() : GetRValueReferenceType(type).GetPointeeType() ?

[aprantl]

lldb/test/API/lang/cpp/null_references/TestNullReferences.py

14

Why is the continue necessary and what is it actually that's triggering the crash?

[teemperor]

Thanks for the patch!

So IIUC correctly this fixes a crash when calling Dereference on an SBValue that is of type SomeTypedef with typedef int& SomeTypedef? If yes, then I think the test here could just be another type+assert in TestCPPDereferencingReferences.py. The test here is a bit expensive and seems to depend on a specific libc++ version (-> it will also always pass on Linux), even though this is just a TypeSystemClang patch.

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

6505

I really like the idea of reusing the TypeSystemClang functions here (we should do this everywhere I think). FWIW, I think this can all just be CompilerType pointee_clang_type = GetNonReferenceType(type); From the code below GetPointeeType would also work. We already call the variable here pointee type so I don't think calling references pointers here will hurt too much, so I think both is fine.

6508

I think the logic here is reversed? type is a reference type (potentially behind a typedef). GetLValueReferenceType on type returns the reference type *to* that type. Not the underlying reference. The fallback for this is just that it returns the current type IIRC if you already have a reference, that's why this works. So, GetLValueReferenceType is just a no-op and GetPointeeType is doing the actual dereferencing. I think just the GetPointeeType is needed.

[shafik]

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

6505

I tried GetNonReferenceType(type).GetPointeeType() but I get back an empty CompilerType it looks like it is doing almost exactly the same thing as the previous code:

(*this)->getAs<ReferenceType>()

[shafik]

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

6508

Maybe I am confused but I thought given:

TypedefType 0x7fb11c841460 'std::__compressed_pair_elem<struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep, 0, false>::const_reference' sugar
|-Typedef 0x7fb11c8413f0 'const_reference'
`-LValueReferenceType 0x7fb11c8413c0 'const struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep &'
  `-QualType 0x7fb11cac3361 'const struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep' const
    `-RecordType 0x7fb11cac3360 'struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep'
      `-CXXRecord 0x7fb11cac32b8 '__rep'

that llvm::cast<clang::ReferenceType>(GetQualType(type).getTypePtr()) was intended to obtain:

LValueReferenceType 0x7fb11c829630 'const struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep &'
`-QualType 0x7fb11cac3361 'const struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep' const
  `-RecordType 0x7fb11cac3360 'struct std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >::__rep'
    `-CXXRecord 0x7fb11cac32b8 '__rep'

which is what GetLValueReferenceType(type) does.

[shafik]

In D108717#2965815, @teemperor wrote:

Thanks for the patch!

So IIUC correctly this fixes a crash when calling Dereference on an SBValue that is of type SomeTypedef with typedef int& SomeTypedef? If yes, then I think the test here could just be another type+assert in TestCPPDereferencingReferences.py. The test here is a bit expensive and seems to depend on a specific libc++ version (-> it will also always pass on Linux), even though this is just a TypeSystemClang patch.

Let me see if I can mock up a test using that and if so that would definitely be nicer.

[teemperor]

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

6505

I mean the whole thing could be just GetNonReferenceType. GetNonReferenceType gives you the underlying type, so the GetPointeeType after would just fail (-> empty return type) when it gets whatever the underlying type is:

CompilerType int_ref = ...;
int_ref.GetName() // -> "int &"
int_ref.GetNonReferenceType().GetName() // -> "int"
int_ref.GetPointeeType().GetName() // -> "int"

CompilerType int = ...;
int.GetName() // -> "int"
int_ref..GetPointeeType().GetName() // -> "" (Invalid CompilerType as int isn't a pointer/reference

6508

That is the right intention of the original code, but GetLValueReferenceType is just the inverse of what should happen. It takes a non-reference type and returns the reference type for it (int -> int &). I added some dump calls to the code which might explain what's going on:

case clang::Type::LValueReference:                                            
case clang::Type::RValueReference:                                            
  if (idx_is_valid) {                                                         
    CompilerType pointee_clang_type;                                          
                                                                              
    if (parent_type_class == clang::Type::LValueReference) {                  
      llvm::errs() << "type:\n";                                              
      GetQualType(type).dump();                                               
      CompilerType tmp = GetLValueReferenceType(type);                        
      llvm::errs() << "tmp (after GetLValueReferenceType):\n";                                               
      tmp.dump();                                                             
      pointee_clang_type = tmp.GetPointeeType();                              
      llvm::errs() << "pointee\n";                                            
      pointee_clang_type.dump();

type:
LValueReferenceType 0x564ba82bde80 'TTT &'
`-TypedefType 0x564ba82bde30 'TTT' sugar
  |-Typedef 0x564ba82bddc0 'TTT'
  `-BuiltinType 0x564ba82bd5d0 'int'
tmp (after GetLValueReferenceType):
LValueReferenceType 0x564ba82bdeb0 'TTT &'
`-TypedefType 0x564ba82bde30 'TTT' sugar
  |-Typedef 0x564ba82bddc0 'TTT'
  `-BuiltinType 0x564ba82bd5d0 'int'
pointee
TypedefType 0x564ba82bde30 'TTT' sugar
|-Typedef 0x564ba82bddc0 'TTT'
`-BuiltinType 0x564ba82bd5d0 'int'

So you get a reference type, then LValueReferenceType does nothing[1] as it's already a reference, and GetPointeeType returns the underlying type.

[1] It seems the function actually desugars the type, but GetPointeeType can handle sugar so it effectively is a no-op.

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

if (transparent_pointers && pointee_clang_type.IsAggregateType()) {
       }
     }
     break;
   }
 
   case clang::Type::LValueReference:
   case clang::Type::RValueReference:
     if (idx_is_valid) {
-      const clang::ReferenceType *reference_type =
-          llvm::cast<clang::ReferenceType>(GetQualType(type).getTypePtr());
-      CompilerType pointee_clang_type =
-          GetType(reference_type->getPointeeType());
+      CompilerType pointee_clang_type;
+
+      if (parent_type_class == clang::Type::LValueReference)
+        pointee_clang_type = GetLValueReferenceType(type).GetPointeeType();
+      else
+        pointee_clang_type = GetRValueReferenceType(type).GetPointeeType();
+
       if (transparent_pointers && pointee_clang_type.IsAggregateType()) {
         child_is_deref_of_parent = false;
         bool tmp_child_is_deref_of_parent = false;
         return pointee_clang_type.GetChildCompilerTypeAtIndex(
             exe_ctx, idx, transparent_pointers, omit_empty_base_classes,
             ignore_array_bounds, child_name, child_byte_size, child_byte_offset,
             child_bitfield_bit_size, child_bitfield_bit_offset,
             child_is_base_class, tmp_child_is_deref_of_parent, valobj,

lldb/test/API/lang/cpp/null_references/Makefile

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

lldb/test/API/lang/cpp/null_references/TestNullReferences.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestNullReferences(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+        self.runCmd("continue")

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

+#include <string>
+
+int f(std::string &instr) {
+  return instr.size(); // break here
+}
+
+int main() {
+  std::string *bad_str = (std::string *)nullptr;
+  // This is undefined behavior. We are purposefully trying to hit
+  // GetCrashingDereference(...)
+  return f(*bad_str);
+}