Respond to review feedback

wesleywiser · wesleywiser · commit 3127419e2b2c · 2021-06-02T10:23:12.000-04:00
diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs
@@ -1485,8 +1485,8 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
             _ => bug!(),
         };
 
-        // This will always find the metadata in the type map.
         let fallback = use_enum_fallback(cx);
+        // This will always find the metadata in the type map.
         let self_metadata = type_metadata(cx, self.enum_type, self.span);
 
         match self.layout.variants {
@@ -1541,11 +1541,11 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
                     //   struct {variant 0 name} {
                     //     tag$ variant$;
                     //     <variant 0 fields>
-                    //   } Variant0;
+                    //   } variant0;
                     //   <other variant structs>
                     // }
                     // ```
-                    // The natvis in `intrinsic.nativs` then matches on `this.Variant0.variant$` to
+                    // The natvis in `intrinsic.nativs` then matches on `this.variant0.variant$` to
                     // determine which variant is active and then displays it.
                     Some(DirectTag {
                         tag_field: Field::from(tag_field),
@@ -1582,7 +1582,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
 
                         MemberDescription {
                             name: if fallback {
-                                format!("Variant{}", i.as_u32())
+                                format!("variant{}", i.as_u32())
                             } else {
                                 variant_info.variant_name()
                             },
@@ -1623,43 +1623,27 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
                     }
                 };
 
-                // For MSVC, we will generate a union of two structs, one for the dataful variant and one that just points to
-                // the discriminant field. We also create an enum that contains tag values for the non-dataful variants and
-                // make the discriminant field that type. We then use natvis to render the enum type correctly in Windbg/VS.
+                // For MSVC, we will generate a union of two fields, one for the dataful variant
+                // and one that just points to the discriminant. We also create an enum that
+                // contains tag values for the non-dataful variants and make the discriminant field
+                // that type. We then use natvis to render the enum type correctly in Windbg/VS.
                 // This will generate debuginfo roughly equivalent to the following C:
                 // ```c
-                // union enum$<{name}, {min niche}, {max niche}, {dataful variant name} {
-                //   struct dataful_variant {
+                // union enum$<{name}, {min niche}, {max niche}, {dataful variant name}> {
+                //   struct <dataful variant name> {
                 //     <fields in dataful variant>
-                //   },
-                //   struct discriminant$ {
-                //     enum tag$ {
-                //       <non-dataful variants>
-                //     } discriminant;
-                //   }
+                //   } dataful_variant;
+                //   enum Discriminant$ {
+                //     <non-dataful variants>
+                //   } discriminant;
                 // }
                 // ```
                 // The natvis in `intrinsic.natvis` matches on the type name `enum$<*, *, *, *>`
-                // and evaluates `this.discriminant$.discriminant`. If the value is between
-                // the min niche and max niche, then the enum is in the dataful variant and
-                // `this.dataful_variant` is rendered. Otherwise, the enum is in one of the
-                // non-dataful variants. In that case, we just need to render the name of the
-                // `this.discriminant$.discriminant` enum.
+                // and evaluates `this.discriminant`. If the value is between the min niche and max
+                // niche, then the enum is in the dataful variant and `this.dataful_variant` is
+                // rendered. Otherwise, the enum is in one of the non-dataful variants. In that
+                // case, we just need to render the name of the `this.discriminant` enum.
                 if fallback {
-                    let unique_type_id = debug_context(cx)
-                        .type_map
-                        .borrow_mut()
-                        .get_unique_type_id_of_enum_variant(cx, self.enum_type, "discriminant$");
-
-                    let variant_metadata = create_struct_stub(
-                        cx,
-                        self.layout.ty,
-                        &"discriminant$",
-                        unique_type_id,
-                        Some(self_metadata),
-                        DIFlags::FlagArtificial,
-                    );
-
                     let dataful_variant_layout = self.layout.for_variant(cx, dataful_variant);
 
                     let mut discr_enum_ty = tag.value.to_ty(cx.tcx);
@@ -1694,8 +1678,8 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
                         llvm::LLVMRustDIBuilderCreateEnumerationType(
                             DIB(cx),
                             self_metadata,
-                            "tag$".as_ptr().cast(),
-                            "tag$".len(),
+                            "Discriminant$".as_ptr().cast(),
+                            "Discriminant$".len(),
                             unknown_file_metadata(cx),
                             UNKNOWN_LINE_NUMBER,
                             tag.value.size(cx).bits(),
@@ -1706,27 +1690,6 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
                         )
                     };
 
-                    let (size, align) =
-                        cx.size_and_align_of(dataful_variant_layout.field(cx, tag_field).ty);
-                    let members = vec![MemberDescription {
-                        name: "discriminant".to_string(),
-                        type_metadata: discr_enum,
-                        offset: dataful_variant_layout.fields.offset(tag_field),
-                        size,
-                        align,
-                        flags: DIFlags::FlagArtificial,
-                        discriminant: None,
-                        source_info: None,
-                    }];
-
-                    set_members_of_composite_type(
-                        cx,
-                        self.enum_type,
-                        variant_metadata,
-                        members,
-                        None,
-                    );
-
                     let variant_info = variant_info_for(dataful_variant);
                     let (variant_type_metadata, member_desc_factory) = describe_enum_variant(
                         cx,
@@ -1747,6 +1710,9 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
                         Some(&self.common_members),
                     );
 
+                    let (size, align) =
+                        cx.size_and_align_of(dataful_variant_layout.field(cx, tag_field).ty);
+
                     vec![
                         MemberDescription {
                             // Name the dataful variant so that we can identify it for natvis
@@ -1760,11 +1726,11 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
                             source_info: variant_info.source_info(cx),
                         },
                         MemberDescription {
-                            name: "discriminant$".into(),
-                            type_metadata: variant_metadata,
-                            offset: Size::ZERO,
-                            size: self.layout.size,
-                            align: self.layout.align.abi,
+                            name: "discriminant".into(),
+                            type_metadata: discr_enum,
+                            offset: dataful_variant_layout.fields.offset(tag_field),
+                            size,
+                            align,
                             flags: DIFlags::FlagZero,
                             discriminant: None,
                             source_info: None,
diff --git a/compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs b/compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs
@@ -238,6 +238,10 @@ pub fn push_debuginfo_type_name<'tcx>(
         }
     }
 
+    /// MSVC names enums differently than other platforms so that the debugging visualization
+    // format (natvis) is able to understand enums and render the active variant correctly in the
+    // debugger. For more information, look in `src/etc/natvis/intrinsic.natvis` and
+    // `EnumMemberDescriptionFactor::create_member_descriptions`.
     fn msvc_enum_fallback(
         tcx: TyCtxt<'tcx>,
         ty: Ty<'tcx>,
diff --git a/src/etc/natvis/intrinsic.natvis b/src/etc/natvis/intrinsic.natvis
@@ -150,7 +150,7 @@
     </Expand>
   </Type>
   <Type Name="enum$&lt;*&gt;">
-    <Intrinsic Name="tag" Expression="Variant0.variant$" />
+    <Intrinsic Name="tag" Expression="variant0.variant$" />
     <DisplayString Condition="tag() == 0">{tag(),en}</DisplayString>
     <DisplayString Condition="tag() == 1" Optional="true">{tag(),en}</DisplayString>
     <DisplayString Condition="tag() == 2" Optional="true">{tag(),en}</DisplayString>
@@ -169,31 +169,32 @@
     <DisplayString Condition="tag() == 15" Optional="true">{tag(),en}</DisplayString>
 
     <Expand>
-      <ExpandedItem Condition="tag() == 0">Variant0</ExpandedItem>
-      <ExpandedItem Condition="tag() == 1" Optional="true">Variant1</ExpandedItem>
-      <ExpandedItem Condition="tag() == 2" Optional="true">Variant2</ExpandedItem>
-      <ExpandedItem Condition="tag() == 3" Optional="true">Variant3</ExpandedItem>
-      <ExpandedItem Condition="tag() == 4" Optional="true">Variant4</ExpandedItem>
-      <ExpandedItem Condition="tag() == 5" Optional="true">Variant5</ExpandedItem>
-      <ExpandedItem Condition="tag() == 6" Optional="true">Variant6</ExpandedItem>
-      <ExpandedItem Condition="tag() == 7" Optional="true">Variant7</ExpandedItem>
-      <ExpandedItem Condition="tag() == 8" Optional="true">Variant8</ExpandedItem>
-      <ExpandedItem Condition="tag() == 9" Optional="true">Variant9</ExpandedItem>
-      <ExpandedItem Condition="tag() == 10" Optional="true">Variant10</ExpandedItem>
-      <ExpandedItem Condition="tag() == 11" Optional="true">Variant11</ExpandedItem>
-      <ExpandedItem Condition="tag() == 12" Optional="true">Variant12</ExpandedItem>
-      <ExpandedItem Condition="tag() == 13" Optional="true">Variant13</ExpandedItem>
-      <ExpandedItem Condition="tag() == 14" Optional="true">Variant14</ExpandedItem>
-      <ExpandedItem Condition="tag() == 15" Optional="true">Variant15</ExpandedItem>
+      <ExpandedItem Condition="tag() == 0">variant0</ExpandedItem>
+      <ExpandedItem Condition="tag() == 1" Optional="true">variant1</ExpandedItem>
+      <ExpandedItem Condition="tag() == 2" Optional="true">variant2</ExpandedItem>
+      <ExpandedItem Condition="tag() == 3" Optional="true">variant3</ExpandedItem>
+      <ExpandedItem Condition="tag() == 4" Optional="true">variant4</ExpandedItem>
+      <ExpandedItem Condition="tag() == 5" Optional="true">variant5</ExpandedItem>
+      <ExpandedItem Condition="tag() == 6" Optional="true">variant6</ExpandedItem>
+      <ExpandedItem Condition="tag() == 7" Optional="true">variant7</ExpandedItem>
+      <ExpandedItem Condition="tag() == 8" Optional="true">variant8</ExpandedItem>
+      <ExpandedItem Condition="tag() == 9" Optional="true">variant9</ExpandedItem>
+      <ExpandedItem Condition="tag() == 10" Optional="true">variant10</ExpandedItem>
+      <ExpandedItem Condition="tag() == 11" Optional="true">variant11</ExpandedItem>
+      <ExpandedItem Condition="tag() == 12" Optional="true">variant12</ExpandedItem>
+      <ExpandedItem Condition="tag() == 13" Optional="true">variant13</ExpandedItem>
+      <ExpandedItem Condition="tag() == 14" Optional="true">variant14</ExpandedItem>
+      <ExpandedItem Condition="tag() == 15" Optional="true">variant15</ExpandedItem>
     </Expand>
   </Type>
 
-  <!-- $T1 is the name of the enum, $T2 is the low value of the dataful variant tag, $T3 is the high value of the dataful variant tag, $T4 is the name of the dataful variant -->
+  <!-- $T1 is the name of the enum, $T2 is the low value of the dataful variant tag,
+       $T3 is the high value of the dataful variant tag, $T4 is the name of the dataful variant -->
   <Type Name="enum$&lt;*, *, *, *&gt;">
-    <Intrinsic Name="tag" Expression="discriminant$.discriminant" />
+    <Intrinsic Name="tag" Expression="discriminant" />
     <Intrinsic Name="is_dataful" Expression="tag() &gt;= $T2 &amp;&amp; tag() &lt;= $T3" />
     <DisplayString Condition="is_dataful()">{"$T4",sb}({dataful_variant})</DisplayString>
-    <DisplayString Condition="!is_dataful()">{discriminant$.discriminant,en}</DisplayString>
+    <DisplayString Condition="!is_dataful()">{discriminant,en}</DisplayString>
     <Expand>
       <ExpandedItem Condition="is_dataful()">dataful_variant</ExpandedItem>
       <Synthetic Condition="is_dataful()" Name="[variant]">
diff --git a/src/test/debuginfo/msvc-pretty-enums.rs b/src/test/debuginfo/msvc-pretty-enums.rs
@@ -11,50 +11,43 @@
 // cdb-check:a,!              [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::Some]
 // cdb-check:        [+0x000] __0              : Low (0x2) [Type: msvc_pretty_enums::CStyleEnum]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : 0x2 [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::tag$]
+// cdb-check:    [+0x000] discriminant     : 0x2 [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::Discriminant$]
 
 // cdb-command: dx -r2 b,!
 // cdb-check:b,!              [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::Some]
 // cdb-check:        [+0x000] __0              : 0x11 [Type: msvc_pretty_enums::CStyleEnum]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : None (0x11) [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::tag$]
+// cdb-check:    [+0x000] discriminant     : None (0x11) [Type: enum$<core::option::Option<enum$<msvc_pretty_enums::CStyleEnum>>, 2, 16, Some>::Discriminant$]
 
 // cdb-command: dx -r2 c,!
 // cdb-check:c,!              [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::Data]
 // cdb-check:        [+0x000] my_data          : 0x11 [Type: msvc_pretty_enums::CStyleEnum]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : Tag1 (0x11) [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::tag$]
+// cdb-check:    [+0x000] discriminant     : Tag1 (0x11) [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::Discriminant$]
 
 // cdb-command: dx -r2 d,!
 // cdb-check:d,!              [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::Data]
 // cdb-check:        [+0x000] my_data          : High (0x10) [Type: msvc_pretty_enums::CStyleEnum]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : 0x10 [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::tag$]
+// cdb-check:    [+0x000] discriminant     : 0x10 [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::Discriminant$]
 
 // cdb-command: dx -r2 e,!
 // cdb-check:e,!              [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::Data]
 // cdb-check:        [+0x000] my_data          : 0x13 [Type: msvc_pretty_enums::CStyleEnum]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : Tag2 (0x13) [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::tag$]
+// cdb-check:    [+0x000] discriminant     : Tag2 (0x13) [Type: enum$<msvc_pretty_enums::NicheLayoutEnum, 2, 16, Data>::Discriminant$]
 
 // cdb-command: dx -r2 f,!
 // cdb-check:f,!              [Type: enum$<core::option::Option<u32*>, 1, [...], Some>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::Some]
 // cdb-check:        [+0x000] __0              : 0x[...] : 0x1 [Type: unsigned int *]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : 0x[...] [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::tag$]
+// cdb-check:    [+0x000] discriminant     : 0x[...] [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::Discriminant$]
 
 // cdb-command: dx -r2 g,!
 // cdb-check:g,!              [Type: enum$<core::option::Option<u32*>, 1, [...], Some>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::Some]
 // cdb-check:        [+0x000] __0              : 0x0 [Type: unsigned int *]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : None (0x0) [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::tag$]
+// cdb-check:    [+0x000] discriminant     : None (0x0) [Type: enum$<core::option::Option<u32*>, 1, [...], Some>::Discriminant$]
 
 // cdb-command: dx h
 // cdb-check:h                : Some [Type: enum$<core::option::Option<u32>>]
@@ -72,8 +65,7 @@
 // cdb-check:k,!              [Type: enum$<core::option::Option<alloc::string::String>, 1, [...], Some>]
 // cdb-check:    [+0x000] dataful_variant  [Type: enum$<core::option::Option<alloc::string::String>, 1, [...], Some>::Some]
 // cdb-check:        [+0x000] __0              [Type: alloc::string::String]
-// cdb-check:    [+0x000] discriminant$    [Type: enum$<core::option::Option<alloc::string::String>, 1, [...], Some>::discriminant$]
-// cdb-check:        [+0x000] discriminant     : 0x[...] [Type: enum$<core::option::Option<alloc::string::String>, 1, [...], Some>::tag$]
+// cdb-check:    [+0x000] discriminant     : 0x[...] [Type: enum$<core::option::Option<alloc::string::String>, 1, [...], Some>::Discriminant$]
 
 pub enum CStyleEnum {
     Low = 2,

Original file line number	Diff line number	Diff line change
`@@ -238,6 +238,10 @@ pub fn push_debuginfo_type_name<'tcx>(`
`238`	`238`	`}`
`239`	`239`	`}`
`240`	`240`
	`241`	`+ /// MSVC names enums differently than other platforms so that the debugging visualization`
	`242`	`+ // format (natvis) is able to understand enums and render the active variant correctly in the`
	`243`	+ // debugger. For more information, look in `src/etc/natvis/intrinsic.natvis` and
	`244`	+ // `EnumMemberDescriptionFactor::create_member_descriptions`.
`241`	`245`	`fn msvc_enum_fallback(`
`242`	`246`	`tcx: TyCtxt<'tcx>,`
`243`	`247`	`ty: Ty<'tcx>,`