[Lldb-commits] [PATCH] D108717: Fix Reference case for TypeSystemClang::GetChildCompilerTypeAtIndex(...) to avoid possible invalid cast

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:6505
     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;
+
----------------
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.


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:6508
+      if (parent_type_class == clang::Type::LValueReference)
+        pointee_clang_type = GetLValueReferenceType(type).GetPointeeType();
+      else
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108717/new/

https://reviews.llvm.org/D108717