private_typeinfo: limit is_dst_type_derived_from_static_type optimization

[rprichard]

Summary

If the destination type does not derive from the static type, we can skip
the search_above_dst call, but we still need to run the
!does_dst_type_point_to_our_static_type block of code. That block of code
will increment info->number_to_dst_ptr to 2, and because dest isn't derived
from static, the cast will ultimately fail.

Fixes PR33439

[grandinj]

src/private_typeinfo.cpp

911 ↗

(On Diff #110147)

static_type,s
->
static_type's

914 ↗

(On Diff #110147)

it if it
->
it, if it

1048 ↗

(On Diff #110147)

ditto

1051 ↗

(On Diff #110147)

ditto

[rprichard]

Incorporate cosmetic changes to comments.

[rprichard]

See https://reviews.llvm.org/D36449 for a set of regression tests.

[EricWF]

Could you please include the tests for this change in this review?

Feel free to make this review dependent on D36446 or vice versa.

[rprichard]

Consolidate the regression test with the bugfix.

[jasonliu]

Ping about this patch.

[EricWF]

This also LGTM. Please make sure to correct the new blocks are properly indented before committing.

[rprichard]

In D36447#1096998, @EricWF wrote:

This also LGTM. Please make sure to correct the new blocks are properly indented before committing.

Thanks for the review. What blocks need an indentation fix?

[EricWF]

Looking at the actual raw diff, it looks like it's indented correctly - but phab's being a little weird about how it's displaying it.
I couldn't actually apply the phab patch directly, so I had to recreate it. So I'm not talking about any problem in specific.

[rprichard]

In D36447#1097004, @EricWF wrote:

Looking at the actual raw diff, it looks like it's indented correctly - but phab's being a little weird about how it's displaying it.
I couldn't actually apply the phab patch directly, so I had to recreate it. So I'm not talking about any problem in specific.

Sounds good to me. I don't have commit access -- who should commit these changes?

[EricWF]

In D36447#1102254, @rprichard wrote:

In D36447#1097004, @EricWF wrote:

Looking at the actual raw diff, it looks like it's indented correctly - but phab's being a little weird about how it's displaying it.
I couldn't actually apply the phab patch directly, so I had to recreate it. So I'm not talking about any problem in specific.

Sounds good to me. I don't have commit access -- who should commit these changes?

Sorry I should have asked if you have commit access. I'll commit these on your behalf.

[EricWF]

Committed as r332767.

libcxxabi/trunk/src/private_typeinfo.cpp

else if (is_equal(this, info->dst_type, use_strcmp))
             if (path_below == public_path)
                 info->path_dynamic_ptr_to_dst_ptr = public_path;
         }
         else  // We have haven't been here before
         {
             // Record the access path that got us here
             //   If there is more than one dst_type this path doesn't matter.
             info->path_dynamic_ptr_to_dst_ptr = path_below;
+            bool does_dst_type_point_to_our_static_type = false;
             // Only search above here if dst_type derives from static_type, or
             //    if it is unknown if dst_type derives from static_type.
             if (info->is_dst_type_derived_from_static_type != no)
             {
-                // Set up flags to record results from all base classes
                 bool is_dst_type_derived_from_static_type = false;
-                bool does_dst_type_point_to_our_static_type = false;
                 // We've found a dst_type with a potentially public path to here.
                 // We have to assume the path is public because it may become
                 //   public later (if we get back to here with a public path).
                 // We can stop looking above if:
                 //    1.  We've found a public path to (static_ptr, static_type).
                 //    2.  We've found an ambiguous cast from (static_ptr, static_type) to a dst_type.
                 //        This is detected at the (static_ptr, static_type).
                 //    3.  We can prove that there is no public path to (static_ptr, static_type)
else if (is_equal(this, info->dst_type, use_strcmp))
                             // If we found a static_type that isn't the one we're looking
                             //    for, and if there are no repeated types above here,
                             //    then stop looking.
                             if (!(__flags & __non_diamond_repeat_mask))
                                 break;
                         }
                     }
                 }
-                if (!does_dst_type_point_to_our_static_type)
-                {
-                    // We found a dst_type that doesn't point to (static_ptr, static_type)
-                    // So record the address of this dst_ptr and increment the
-                    // count of the number of such dst_types found in the tree.
-                    info->dst_ptr_not_leading_to_static_ptr = current_ptr;
-                    info->number_to_dst_ptr += 1;
-                    // If there exists another dst with a private path to
-                    //    (static_ptr, static_type), then the cast from 
-                    //     (dynamic_ptr, dynamic_type) to dst_type is now ambiguous,
-                    //      so stop search.
-                    if (info->number_to_static_ptr == 1 &&
-                            info->path_dst_ptr_to_static_ptr == not_public_path)
-                        info->search_done = true;
-                }
-                // If we found no static_type,s then dst_type doesn't derive
-                //   from static_type, else it does.  Record this result so that
-                //   next time we hit a dst_type we will know not to search above
-                //   it if it doesn't derive from static_type.
-                if (is_dst_type_derived_from_static_type)
-                    info->is_dst_type_derived_from_static_type = yes;
-                else
-                    info->is_dst_type_derived_from_static_type = no;
-            }
+                // If we found no static_type's then dst_type doesn't derive
+                //   from static_type, else it does.  Record this result so that
+                //   next time we hit a dst_type we will know not to search above
+                //   it, if it doesn't derive from static_type.
+                if (is_dst_type_derived_from_static_type)
+                    info->is_dst_type_derived_from_static_type = yes;
+                else
+                    info->is_dst_type_derived_from_static_type = no;
+            }
+                //    (static_ptr, static_type), then the cast from
         }
     }
     else
     {
         // This is not a static_type and not a dst_type.
         const Iter e = __base_info + __base_count;
         Iter p = __base_info;
         p->search_below_dst(info, current_ptr, path_below, use_strcmp);
else if (is_equal(this, info->dst_type, use_strcmp))
             if (path_below == public_path)
                 info->path_dynamic_ptr_to_dst_ptr = public_path;
         }
         else  // We have haven't been here before
         {
             // Record the access path that got us here
             //   If there is more than one dst_type this path doesn't matter.
             info->path_dynamic_ptr_to_dst_ptr = path_below;
+            bool does_dst_type_point_to_our_static_type = false;
             // Only search above here if dst_type derives from static_type, or
             //    if it is unknown if dst_type derives from static_type.
             if (info->is_dst_type_derived_from_static_type != no)
             {
-                // Set up flags to record results from all base classes
                 bool is_dst_type_derived_from_static_type = false;
-                bool does_dst_type_point_to_our_static_type = false;
                 // Zero out found flags
                 info->found_our_static_ptr = false;
                 info->found_any_static_type = false;
                 __base_type->search_above_dst(info, current_ptr, current_ptr, public_path, use_strcmp);
                 if (info->found_any_static_type)
                 {
                     is_dst_type_derived_from_static_type = true;
                     if (info->found_our_static_ptr)
                         does_dst_type_point_to_our_static_type = true;
                 }
-                if (!does_dst_type_point_to_our_static_type)
-                {
-                    // We found a dst_type that doesn't point to (static_ptr, static_type)
-                    // So record the address of this dst_ptr and increment the
-                    // count of the number of such dst_types found in the tree.
-                    info->dst_ptr_not_leading_to_static_ptr = current_ptr;
-                    info->number_to_dst_ptr += 1;
-                    // If there exists another dst with a private path to
-                    //    (static_ptr, static_type), then the cast from 
-                    //     (dynamic_ptr, dynamic_type) to dst_type is now ambiguous.
-                    if (info->number_to_static_ptr == 1 &&
-                            info->path_dst_ptr_to_static_ptr == not_public_path)
-                        info->search_done = true;
-                }
-                // If we found no static_type,s then dst_type doesn't derive
-                //   from static_type, else it does.  Record this result so that
-                //   next time we hit a dst_type we will know not to search above
-                //   it if it doesn't derive from static_type.
-                if (is_dst_type_derived_from_static_type)
-                    info->is_dst_type_derived_from_static_type = yes;
-                else
-                    info->is_dst_type_derived_from_static_type = no;
-            }
+                // If we found no static_type's then dst_type doesn't derive
+                //   from static_type, else it does.  Record this result so that
+                //   next time we hit a dst_type we will know not to search above
+                //   it, if it doesn't derive from static_type.
+                if (is_dst_type_derived_from_static_type)
+                    info->is_dst_type_derived_from_static_type = yes;
+                else
+                    info->is_dst_type_derived_from_static_type = no;
+            }
+                //    (static_ptr, static_type), then the cast from
         }
     }
     else
     {
         // This is not a static_type and not a dst_type
         __base_type->search_below_dst(info, current_ptr, path_below, use_strcmp);
     }
 }

libcxxabi/trunk/test/dynamic_cast.pass.cpp

void test()
     assert(dynamic_cast<Class6*>(c1) == 0);
     assert(dynamic_cast<Left*>(c1) == static_cast<Left*>(&m));
     assert(dynamic_cast<Right*>(c1) == static_cast<Right*>(&m));
     assert(dynamic_cast<Main*>(c1) == static_cast<Main*>(&m));
 }
 
 }  // t3
 
+namespace t4
+{
+
+// PR33439
+struct C2 { virtual ~C2() {} Pad1 _; };
+struct C3 { virtual ~C3() {} Pad2 _; };
+struct C4 : C3 { Pad3 _; };
+struct C8 : C2, virtual C4 { Pad4 _; };
+struct C9 : C4, C8 { Pad5 _; };
+
+C9 c9;
+C2 *c2 = &c9;
+
+void test()
+{
+    assert(dynamic_cast<C2*>(c2) == static_cast<C2*>(&c9));
+    assert(dynamic_cast<C3*>(c2) == 0);
+    assert(dynamic_cast<C4*>(c2) == 0);
+    assert(dynamic_cast<C8*>(c2) == static_cast<C8*>(&c9));
+    assert(dynamic_cast<C9*>(c2) == static_cast<C9*>(&c9));
+}
+
+}  // t4
+
+namespace t5
+{
+
+// PR33439
+struct Dummy { virtual ~Dummy() {} Pad1 _; };
+struct Src { virtual ~Src() {} Pad2 _; };
+struct Dest : Dummy { Pad3 _; };
+struct A1 : Dest { Pad4 _; };
+struct A2 : Dest { Pad5 _; };
+struct Root : Src, A1, A2 { Pad6 _; };
+
+Root root;
+Src *src = &root;
+
+void test()
+{
+    assert(dynamic_cast<Dummy*>(src) == 0);
+    assert(dynamic_cast<Src*>(src) == static_cast<Src*>(&root));
+    assert(dynamic_cast<Dest*>(src) == 0);
+    assert(dynamic_cast<A1*>(src) == static_cast<A1*>(&root));
+    assert(dynamic_cast<A2*>(src) == static_cast<A2*>(&root));
+}
+
+}  // t5
+
 int main()
 {
     t1::test();
     t2::test();
     t3::test();
+    t4::test();
+    t5::test();
 }