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

static_type,s
->
static_type's

914

it if it
->
it, if it

1048

ditto

1051

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.

src/private_typeinfo.cpp

             // 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).

                         }
                     }
                 }
-                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
+                // 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.
+                //   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 (!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;
+            }
         }
     }
     else

             // 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;

                     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
+                // 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.
+                //   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 (!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;
+            }
         }
     }
     else

test/dynamic_cast.pass.cpp

 
 }  // 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();
 }