Fix unsound C++ union copy-constructor (#3892)

### What * Closes #3864 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3892) (if applicable) * [X] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/3892) - [Docs preview](https://rerun.io/preview/bd53891896e118c00fa8d48bf51f5f12eadae453/docs)  - [Examples preview](https://rerun.io/preview/bd53891896e118c00fa8d48bf51f5f12eadae453/examples)  - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
rerun-io · Oct 17, 2023 · 7f6e0af · 7f6e0af
1 parent 30dc8be
commit 7f6e0af
Show file tree

Hide file tree

Showing 87 changed files with 782 additions and 803 deletions.
diff --git a/.clang-format b/.clang-format
@@ -15,6 +15,7 @@ BinPackArguments: false
 BreakStringLiterals: false
 ColumnLimit: 100
 ContinuationIndentWidth: 4
+DerivePointerAlignment: false
 EmptyLineBeforeAccessModifier: LogicalBlock
 IndentWidth: 4
 IndentWrappedFunctionNames: true

diff --git a/crates/re_types_builder/src/codegen/cpp/mod.rs b/crates/re_types_builder/src/codegen/cpp/mod.rs
@@ -813,8 +813,9 @@ impl QuotedObject {
                 let comment = quote_comment("Nothing to destroy");
                 quote! {
                     case detail::#tag_typename::NONE: {
-                        break; #comment
-                    }
+                        #NEWLINE_TOKEN
+                        #comment
+                    } break;
                 }
             })
             .chain(obj.fields.iter().map(|obj_field| {
@@ -825,8 +826,9 @@ impl QuotedObject {
                     let comment = quote_comment("has a trivial destructor");
                     quote! {
                         case detail::#tag_typename::#tag_ident: {
-                            break; #comment
-                        }
+                            #NEWLINE_TOKEN
+                            #comment
+                        } break;
                     }
                 } else if let Type::Array { elem_type, length } = &obj_field.typ {
                     // We need special casing for destroying arrays in C++:
@@ -838,8 +840,7 @@ impl QuotedObject {
                             for (size_t i = #length; i > 0; i -= 1) {
                                 _data.#field_ident[i-1].~TypeAlias();
                             }
-                            break;
-                        }
+                        } break;
                     }
                 } else {
                     let typedef_declaration =
@@ -849,8 +850,7 @@ impl QuotedObject {
                         case detail::#tag_typename::#tag_ident: {
                             typedef #typedef_declaration;
                             _data.#field_ident.~TypeAlias();
-                            break;
-                        }
+                        } break;
                     }
                 }
             }))
@@ -867,8 +867,8 @@ impl QuotedObject {
 
         let copy_constructor = {
             // Note that `switch` on an enum without handling all cases causes `-Wswitch-enum` warning!
-            let mut copy_match_arms = Vec::new();
-            let mut default_match_arms = Vec::new();
+            let mut placement_new_arms = Vec::new();
+            let mut trivial_memcpy_cases = Vec::new();
             for obj_field in &obj.fields {
                 let tag_ident = format_ident!("{}", obj_field.name);
                 let case = quote!(case detail::#tag_typename::#tag_ident:);
@@ -877,14 +877,19 @@ impl QuotedObject {
                 // but is typically the reason why we need to do this in the first place - if we'd always memcpy we'd get double-free errors.
                 // (As with swap, we generously assume that objects are rellocatable)
                 if obj_field.typ.has_default_destructor(objects) {
-                    default_match_arms.push(case);
+                    trivial_memcpy_cases.push(case);
                 } else {
+                    // the `this->_data` union is not yet initialized, so we must use placement new:
+                    let typedef_declaration =
+                        quote_variable(&mut hpp_includes, obj_field, &format_ident!("TypeAlias"));
+                    hpp_includes.insert_system("new"); // placement-new
+
                     let field_ident = format_ident!("{}", obj_field.snake_case_name());
-                    copy_match_arms.push(quote! {
+                    placement_new_arms.push(quote! {
                         #case {
-                            _data.#field_ident = other._data.#field_ident;
-                            break;
-                        }
+                            typedef #typedef_declaration;
+                            new (&_data.#field_ident) TypeAlias(other._data.#field_ident);
+                        } break;
                     });
                 }
             }
@@ -895,21 +900,51 @@ impl QuotedObject {
                 std::memcpy(thisbytes, otherbytes, sizeof(detail::#data_typename));
             };
 
-            if copy_match_arms.is_empty() {
-                quote!(#pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
-                    #trivial_memcpy
-                })
-            } else {
-                quote!(#pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
-                    switch (other._tag) {
-                        #(#copy_match_arms)*
+            let comment = quote_doc_comment("Copy constructor");
 
-                        case detail::#tag_typename::NONE:
-                        #(#default_match_arms)*
+            if placement_new_arms.is_empty() {
+                quote! {
+                    #NEWLINE_TOKEN
+                    #NEWLINE_TOKEN
+                    #comment
+                    #pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
                         #trivial_memcpy
-                            break;
                     }
-                })
+                }
+            } else if trivial_memcpy_cases.is_empty() {
+                quote! {
+                    #NEWLINE_TOKEN
+                    #NEWLINE_TOKEN
+                    #comment
+                    #pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
+                        switch (other._tag) {
+                            #(#placement_new_arms)*
+
+                            case detail::#tag_typename::NONE: {
+                                // there is nothing to copy
+                            } break;
+                        }
+                    }
+                }
+            } else {
+                quote! {
+                    #NEWLINE_TOKEN
+                    #NEWLINE_TOKEN
+                    #comment
+                    #pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
+                        switch (other._tag) {
+                            #(#placement_new_arms)*
+
+                            #(#trivial_memcpy_cases)* {
+                                #trivial_memcpy
+                            } break;
+
+                            case detail::#tag_typename::NONE: {
+                                // there is nothing to copy
+                            } break;
+                        }
+                    }
+                }
             }
         };
 
@@ -1503,8 +1538,7 @@ fn quote_fill_arrow_array_builder(
                         case detail::#tag_name::#variant_name: {
                             auto #variant_builder = static_cast<arrow::#arrow_builder_type*>(variant_builder_untyped);
                             #variant_append
-                            break;
-                        }
+                        } break;
                     }
                 });
 
@@ -1523,8 +1557,7 @@ fn quote_fill_arrow_array_builder(
                         switch (union_instance._tag) {
                             case detail::#tag_name::NONE: {
                                 ARROW_RETURN_NOT_OK(variant_builder_untyped->AppendNull());
-                                break;
-                            }
+                            } break;
                             #(#tag_cases)*
                         }
                     }

diff --git a/docs/code-examples/roundtrips.py b/docs/code-examples/roundtrips.py
@@ -31,7 +31,6 @@
     "image_advanced": ["cpp", "rust"], # Missing examples
     "image_simple": ["cpp"], # TODO(#2919): Not yet implemented in C++
     "log_line": ["cpp", "rust", "py"], # Not a complete example -- just a single log line
-    "pinhole_simple": ["cpp"], # TODO(#2919): Seg-faults in C++
     "quick_start_connect": ["cpp"], # TODO(#3870): Not yet implemented in C++
     "scalar_multiple_plots": ["cpp"], # TODO(#2919): Not yet implemented in C++
     "segmentation_image_simple": ["cpp"], # TODO(#2919): Not yet implemented in C++

diff --git a/rerun_cpp/src/rerun/blueprint/auto_space_views.cpp b/rerun_cpp/src/rerun/blueprint/auto_space_views.cpp
diff --git a/rerun_cpp/src/rerun/blueprint/panel_view.cpp b/rerun_cpp/src/rerun/blueprint/panel_view.cpp
diff --git a/rerun_cpp/src/rerun/components/annotation_context.cpp b/rerun_cpp/src/rerun/components/annotation_context.cpp
diff --git a/rerun_cpp/src/rerun/components/blob.cpp b/rerun_cpp/src/rerun/components/blob.cpp