Add additional mangling for struct members of non trivial structs

[danzimm]

Summary

In https://bugs.llvm.org/show_bug.cgi?id=41206 we observe bad codegen when embedding a non-trivial C struct within a C struct. This is due to the fact that name mangling for non-trivial structs marks the two structs as identical. This diff contains a potential fix for this issue. Please comment if there're any untested scenarios I should include.

Test Plan: I've included a unit test & updated other tests

[lebedev.ri]

There is no test here.
Tests should probably go into clang/test/CodeGen.

[danzimm]

@lebedev.ri right, sorry about that- I prematurely diff'd (got a few terminals crossed and thought I was finished with the test already). I will be amending with a test and a few other test fixes shortly. Sorry about the miscommunication :/

[danzimm]

I forgot to add the test originally, this update contains a test and updates to old tests to make them pass

[danzimm]

@smeenai please feel free add any reviewers that I might've missed

[ahatanak]

LGTM

[smeenai]

Do you need someone to commit this for you?

clang/test/CodeGenObjC/nontrivial-c-struct-within-struct-name.m

1 ↗

(On Diff #192436)

I think you can get rid of -fblocks, -fobjc-runtime=ios-11.0, -DUSESTRUCT, and -I %S/Inputs

[smeenai]

Also, it might be worth adding a third level of struct to the test, to show that it handles arbitrary nesting correctly (which it does).

[danzimm]

Add a third level to ensure nontrivial structs within structs within structs works (this suggests that N embeddings works too). Also change the invocation of the test

[danzimm]

@smeenai good idea on the third level!

Yep, I'll need somebody to commit this for me, thanks!

[smeenai]

Looks good. I'll commit this.

cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp

 //
 // <function-name> ::= <prefix> <alignment-info> "_" <struct-field-info>
 // <prefix> ::= "__destructor_" | "__default_constructor_" |
 //              "__copy_constructor_" | "__move_constructor_" |
 //              "__copy_assignment_" | "__move_assignment_"
 // <alignment-info> ::= <dst-alignment> ["_" <src-alignment>]
 // <struct-field-info> ::= <field-info>+
 // <field-info> ::= <struct-or-scalar-field-info> | <array-field-info>
-// <struct-or-scalar-field-info> ::= <struct-field-info> | <strong-field-info> |
-//                                   <trivial-field-info>
+// <struct-or-scalar-field-info> ::= "_S" <struct-field-info> |
+//                                   <strong-field-info> | <trivial-field-info>
 // <array-field-info> ::= "_AB" <array-offset> "s" <element-size> "n"
 //                        <num-elements> <innermost-element-info> "_AE"
 // <innermost-element-info> ::= <struct-or-scalar-field-info>
 // <strong-field-info> ::= "_s" ["b"] ["v"] <field-offset>
 // <trivial-field-info> ::= "_t" ["v"] <field-offset> "_" <field-size>
 
 template <class Derived> struct GenFuncNameBase {
   std::string getVolatileOffsetStr(bool IsVolatile, CharUnits Offset) {
void visitARCWeak(QualType FT, const FieldDecl *FD,
     appendStr("_w");
     CharUnits FieldOffset = CurStructOffset + asDerived().getFieldOffset(FD);
     appendStr(getVolatileOffsetStr(FT.isVolatileQualified(), FieldOffset));
   }
 
   void visitStruct(QualType QT, const FieldDecl *FD,
                    CharUnits CurStructOffset) {
     CharUnits FieldOffset = CurStructOffset + asDerived().getFieldOffset(FD);
+    appendStr("_S");
     asDerived().visitStructFields(QT, FieldOffset);
   }
 
   template <class FieldKind>
   void visitArray(FieldKind FK, const ArrayType *AT, bool IsVolatile,
                   const FieldDecl *FD, CharUnits CurStructOffset) {
     // String for non-volatile trivial fields is emitted when
     // flushTrivialFields is called.

cfe/trunk/test/CodeGenObjC/nontrivial-c-struct-within-struct-name.m

+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+
+@class I;
+
+typedef struct {
+  I *name;
+} Foo;
+
+typedef struct {
+  Foo foo;
+} Bar;
+
+typedef struct {
+  Bar bar;
+} Baz;
+
+I *getI();
+
+void f() {
+  Foo foo = {getI()};
+  Bar bar = {foo};
+  Baz baz = {bar};
+}
+
+// CHECK: define linkonce_odr hidden void @__destructor_8_S_S_s0(i8** %[[DST:.*]])
+// CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
+// CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
+// CHECK: call void @__destructor_8_S_s0(i8** %[[V0]])
+// CHECK: ret void
+
+// CHECK: define linkonce_odr hidden void @__destructor_8_S_s0(i8** %[[DST:.*]])
+// CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
+// CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
+// CHECK: call void @__destructor_8_s0(i8** %[[V0]])
+// CHECK: ret void
+
+// CHECK: define linkonce_odr hidden void @__destructor_8_s0(i8** %dst)
+// CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
+// CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
+// CHECK: call void @llvm.objc.storeStrong(i8** %[[V0]], i8* null)
+// CHECK: ret void

cfe/trunk/test/CodeGenObjC/strong-in-c-struct.m

 void calleeStrongSmall(StrongSmall);
 void func(Strong *);
 
 // CHECK: %[[STRUCT_BITFIELD1:.*]] = type { i8, i8, i8*, i32, i8*, [3 x i32], i8*, double, i8, i8 }
 
 // CHECK: define void @test_constructor_destructor_StrongOuter()
 // CHECK: %[[T:.*]] = alloca %[[STRUCT_STRONGOUTER:.*]], align 8
 // CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T]] to i8**
-// CHECK: call void @__default_constructor_8_s16_s24(i8** %[[V0]])
+// CHECK: call void @__default_constructor_8_S_s16_s24(i8** %[[V0]])
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T]] to i8**
-// CHECK: call void @__destructor_8_s16_s24(i8** %[[V1]])
+// CHECK: call void @__destructor_8_S_s16_s24(i8** %[[V1]])
 // CHECK: ret void
 
-// CHECK: define linkonce_odr hidden void @__default_constructor_8_s16_s24(i8** %[[DST:.*]])
+// CHECK: define linkonce_odr hidden void @__default_constructor_8_S_s16_s24(i8** %[[DST:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: call void @__default_constructor_8_s16(i8** %[[V0]])
 // CHECK: %[[V1:.*]] = bitcast i8** %[[V0]] to i8*
 // CHECK: %[[V2:.*]] = getelementptr inbounds i8, i8* %[[V1]], i64 24
 // CHECK: %[[V3:.*]] = bitcast i8* %[[V2]] to i8**
 // CHECK: %[[V4:.*]] = bitcast i8** %[[V3]] to i8*
 // CHECK: call void @llvm.memset.p0i8.i64(i8* align 8 %[[V4]], i8 0, i64 8, i1 false)
 // CHECK: ret void
 
 // CHECK: define linkonce_odr hidden void @__default_constructor_8_s16(i8** %[[DST:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V1:.*]] = bitcast i8** %[[V0]] to i8*
 // CHECK: %[[V2:.*]] = getelementptr inbounds i8, i8* %[[V1]], i64 16
 // CHECK: %[[V3:.*]] = bitcast i8* %[[V2]] to i8**
 // CHECK: %[[V4:.*]] = bitcast i8** %[[V3]] to i8*
 // CHECK: call void @llvm.memset.p0i8.i64(i8* align 8 %[[V4]], i8 0, i64 8, i1 false)
 // CHECK: ret void
 
-// CHECK: define linkonce_odr hidden void @__destructor_8_s16_s24(i8** %[[DST:.*]])
+// CHECK: define linkonce_odr hidden void @__destructor_8_S_s16_s24(i8** %[[DST:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: call void @__destructor_8_s16(i8** %[[V0]])
 // CHECK: %[[V1:.*]] = bitcast i8** %[[V0]] to i8*
 // CHECK: %[[V2:.*]] = getelementptr inbounds i8, i8* %[[V1]], i64 24
 // CHECK: %[[V3:.*]] = bitcast i8* %[[V2]] to i8**
 // CHECK: call void @llvm.objc.storeStrong(i8** %[[V3]], i8* null)

 
 // CHECK: define void @test_copy_constructor_StrongOuter(%[[STRUCT_STRONGOUTER:.*]]* %[[S:.*]])
 // CHECK: %[[S_ADDR:.*]] = alloca %[[STRUCT_STRONGOUTER]]*, align 8
 // CHECK: %[[T:.*]] = alloca %[[STRUCT_STRONGOUTER]], align 8
 // CHECK: store %[[STRUCT_STRONGOUTER]]* %[[S]], %[[STRUCT_STRONGOUTER]]** %[[S_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load %[[STRUCT_STRONGOUTER]]*, %[[STRUCT_STRONGOUTER]]** %[[S_ADDR]], align 8
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T]] to i8**
 // CHECK: %[[V2:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[V0]] to i8**
-// CHECK: call void @__copy_constructor_8_8_t0w16_s16_s24_t32w8(i8** %[[V1]], i8** %[[V2]])
+// CHECK: call void @__copy_constructor_8_8_S_t0w16_s16_s24_t32w8(i8** %[[V1]], i8** %[[V2]])
 // CHECK: %[[V3:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T]] to i8**
-// CHECK: call void @__destructor_8_s16_s24(i8** %[[V3]])
+// CHECK: call void @__destructor_8_S_s16_s24(i8** %[[V3]])
 // CHECK: ret void
 
-// CHECK: define linkonce_odr hidden void @__copy_constructor_8_8_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
+// CHECK: define linkonce_odr hidden void @__copy_constructor_8_8_S_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: %[[SRC_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: store i8** %[[SRC]], i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V1:.*]] = load i8**, i8*** %[[SRC_ADDR]], align 8
 // CHECK: call void @__copy_constructor_8_8_t0w16_s16(i8** %[[V0]], i8** %[[V1]])
 // CHECK: %[[V2:.*]] = bitcast i8** %[[V0]] to i8*

 // CHECK: %[[V11:.*]] = call i8* @llvm.objc.retain(i8* %[[V10]])
 // CHECK: store i8* %[[V11]], i8** %[[V6]], align 8
 // CHECK: ret void
 
 void test_copy_constructor_StrongOuter(StrongOuter *s) {
   StrongOuter t = *s;
 }
 
-/// CHECK: define linkonce_odr hidden void @__copy_assignment_8_8_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
+/// CHECK: define linkonce_odr hidden void @__copy_assignment_8_8_S_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: %[[SRC_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: store i8** %[[SRC]], i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V1:.*]] = load i8**, i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V2:.*]] = bitcast i8** %[[V0]] to i8*
 // CHECK: %[[V3:.*]] = getelementptr inbounds i8, i8* %[[V2]], i64 24
 // CHECK: %[[V4:.*]] = bitcast i8* %[[V3]] to i8**
 // CHECK: %[[V5:.*]] = bitcast i8** %[[V1]] to i8*
 // CHECK: %[[V6:.*]] = getelementptr inbounds i8, i8* %[[V5]], i64 24
 // CHECK: %[[V7:.*]] = bitcast i8* %[[V6]] to i8**
 // CHECK: %[[V8:.*]] = load i8*, i8** %[[V7]], align 8
 // CHECK: call void @llvm.objc.storeStrong(i8** %[[V4]], i8* %[[V8]])
 
 void test_copy_assignment_StrongOuter(StrongOuter *d, StrongOuter *s) {
   *d = *s;
 }
 
 // CHECK: define void @test_move_constructor_StrongOuter()
 // CHECK: %[[T1:.*]] = getelementptr inbounds %[[STRUCT_BLOCK_BYREF_T:.*]], %[[STRUCT_BLOCK_BYREF_T]]* %{{.*}}, i32 0, i32 7
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T1]] to i8**
-// CHECK: call void @__default_constructor_8_s16_s24(i8** %[[V1]])
+// CHECK: call void @__default_constructor_8_S_s16_s24(i8** %[[V1]])
 // CHECK: %[[T2:.*]] = getelementptr inbounds %[[STRUCT_BLOCK_BYREF_T]], %[[STRUCT_BLOCK_BYREF_T]]* %{{.*}}, i32 0, i32 7
 // CHECK: %[[V9:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T2]] to i8**
-// CHECK: call void @__destructor_8_s16_s24(i8** %[[V9]])
+// CHECK: call void @__destructor_8_S_s16_s24(i8** %[[V9]])
 
 // CHECK: define internal void @__Block_byref_object_copy_(i8*, i8*)
-// CHECK: call void @__move_constructor_8_8_t0w16_s16_s24_t32w8(
+// CHECK: call void @__move_constructor_8_8_S_t0w16_s16_s24_t32w8(
 
-// CHECK: define linkonce_odr hidden void @__move_constructor_8_8_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
+// CHECK: define linkonce_odr hidden void @__move_constructor_8_8_S_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: %[[SRC_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: store i8** %[[SRC]], i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V1:.*]] = load i8**, i8*** %[[SRC_ADDR]], align 8
 // CHECK: call void @__move_constructor_8_8_t0w16_s16(i8** %[[V0]], i8** %[[V1]])
 // CHECK: %[[V2:.*]] = bitcast i8** %[[V0]] to i8*
 // CHECK: %[[V3:.*]] = getelementptr inbounds i8, i8* %[[V2]], i64 24
 // CHECK: %[[V4:.*]] = bitcast i8* %[[V3]] to i8**
 // CHECK: %[[V5:.*]] = bitcast i8** %[[V1]] to i8*
 // CHECK: %[[V6:.*]] = getelementptr inbounds i8, i8* %[[V5]], i64 24
 // CHECK: %[[V7:.*]] = bitcast i8* %[[V6]] to i8**
 // CHECK: %[[V8:.*]] = load i8*, i8** %[[V7]], align 8
 // CHECK: store i8* null, i8** %[[V7]], align 8
 // CHECK: store i8* %[[V8]], i8** %[[V4]], align 8
 
 // CHECK: define internal void @__Block_byref_object_dispose_(i8*)
-// CHECK: call void @__destructor_8_s16_s24(
+// CHECK: call void @__destructor_8_S_s16_s24(
 
 void test_move_constructor_StrongOuter(void) {
   __block StrongOuter t;
   BlockTy b = ^{ (void)t; };
 }
 
-// CHECK: define linkonce_odr hidden void @__move_assignment_8_8_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
+// CHECK: define linkonce_odr hidden void @__move_assignment_8_8_S_t0w16_s16_s24_t32w8(i8** %[[DST:.*]], i8** %[[SRC:.*]])
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: %[[SRC_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: store i8** %[[SRC]], i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V1:.*]] = load i8**, i8*** %[[SRC_ADDR]], align 8
 // CHECK: call void @__move_assignment_8_8_t0w16_s16(i8** %[[V0]], i8** %[[V1]])
 // CHECK: %[[V2:.*]] = bitcast i8** %[[V0]] to i8*

 }
 
 // CHECK: define linkonce_odr hidden void @__default_constructor_8_AB8s8n4_s8_AE(
 // CHECK: call void @llvm.memset.p0i8.i64(i8* align 8 %{{.*}}, i8 0, i64 32, i1 false)
 void test_constructor_destructor_IDArray(void) {
   IDArray t;
 }
 
-// CHECK: define linkonce_odr hidden void @__default_constructor_8_AB8s24n4_s24_AE(
+// CHECK: define linkonce_odr hidden void @__default_constructor_8_AB8s24n4_S_s24_AE(
 void test_constructor_destructor_StructArray(void) {
   StructArray t;
 }
 
 // Test that StructArray's field 'd' is copied before entering the loop.
 
-// CHECK: define linkonce_odr hidden void @__copy_constructor_8_8_t0w8_AB8s24n4_t8w16_s24_AE(i8** %[[DST:.*]], i8** %[[SRC:.*]])
+// CHECK: define linkonce_odr hidden void @__copy_constructor_8_8_t0w8_AB8s24n4_S_t8w16_s24_AE(i8** %[[DST:.*]], i8** %[[SRC:.*]])
 // CHECK: entry:
 // CHECK: %[[DST_ADDR:.*]] = alloca i8**, align 8
 // CHECK: %[[SRC_ADDR:.*]] = alloca i8**, align 8
 // CHECK: store i8** %[[DST]], i8*** %[[DST_ADDR]], align 8
 // CHECK: store i8** %[[SRC]], i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V0:.*]] = load i8**, i8*** %[[DST_ADDR]], align 8
 // CHECK: %[[V1:.*]] = load i8**, i8*** %[[SRC_ADDR]], align 8
 // CHECK: %[[V2:.*]] = bitcast i8** %[[V0]] to i64*