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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
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;
+
----------------
shafik wrote:
> teemperor wrote:
> > 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.
> 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>()`
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
```


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

```
lang=c++
  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();  
```

```
lang=python
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.


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

https://reviews.llvm.org/D108717