Variable auto-init: don't initialize aggregate padding of all aggregates

[jfb]

Summary

C guarantees that brace-init with fewer initializers than members in the
aggregate will initialize the rest of the aggregate as-if it were static
initialization. In turn static initialization guarantees that padding is
initialized to zero bits.

rdar://problem/50188861

[rjmccall]

IIRC, C says *members* are initialized as if they were in static storage, which might mean that their interior padding should be zeroed, but I don't think says anything about the padding in the enclosing aggregate. But I think zero-initializing padding is probably the right thing to do in general under this flag.

[erik.pilkington]

LGTM, I think this makes sense.

[jfb]

In D61280#1485073, @rjmccall wrote:

IIRC, C says *members* are initialized as if they were in static storage, which might mean that their interior padding should be zeroed, but I don't think says anything about the padding in the enclosing aggregate. But I think zero-initializing padding is probably the right thing to do in general under this flag.

Quoth the Standard:

C17 6.7.9 Initialization ❡21
If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.

C17 6.7.9 Initialization ❡10
If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static or thread storage duration is not initialized explicitly, then:

• if it has pointer type, it is initialized to a null pointer;
• if it has arithmetic type, it is initialized to (positive or unsigned) zero;
• if it is an aggregate, every member is initialized (recursively) according to these rules, and and padding is initialized to zero bits;
• if it is a union, the first named member is initialized (recursively) according to these rules, and any padding is initialized to zero bits;

🙂

[rjmccall]

I don't think the implication is supposed to be that padding is zero-initialized or not depending on where in the aggregate it appears, but it doesn't really matter, I don't think we're arguing about the goal of this patch.

[jfb]

In D61280#1485115, @rjmccall wrote:

I don't think the implication is supposed to be that padding is zero-initialized or not depending on where in the aggregate it appears, but it doesn't really matter, I don't think we're arguing about the goal of this patch.

The way I read it, C guarantees that an aggregate's padding is zero-initialized when you use brace-enclosed list *unless* you've mentioned all the elements / members of the aggregate. Fail to mention one → padding must be zero. Mention all of them → padding isn't guaranteed.

But then *reading* padding is another story. What triggered this change is C code that used memcmp on a struct with padding, and was expecting padding to be set. Not great code, but AFAICT technically correct.

[jfb]

This seems uncontroversial, I'm happy to make follow-up change if you have suggestions.

[jfb]

Follow-up test fix in http://llvm.org/r359636

lib/CodeGen/CGDecl.cpp

if (isTrivialInitializer(Init)) {
     return;
   }
 
   llvm::Constant *constant = nullptr;
   if (emission.IsConstantAggregate || D.isConstexpr() ||
       D.isUsableInConstantExpressions(getContext())) {
     assert(!capturedByInit && "constant init contains a capturing block?");
     constant = ConstantEmitter(*this).tryEmitAbstractForInitializer(D);
-    if (constant && trivialAutoVarInit !=
-                        LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+    if (constant && !constant->isZeroValue() &&
+        (trivialAutoVarInit !=
+         LangOptions::TrivialAutoVarInitKind::Uninitialized)) {
       IsPattern isPattern =
           (trivialAutoVarInit == LangOptions::TrivialAutoVarInitKind::Pattern)
               ? IsPattern::Yes
               : IsPattern::No;
-      constant = constWithPadding(CGM, isPattern,
+      // C guarantees that brace-init with fewer initializers than members in
+      // the aggregate will initialize the rest of the aggregate as-if it were
+      // static initialization. In turn static initialization guarantees that
+      // padding is initialized to zero bits. We could instead pattern-init if D
+      // has any ImplicitValueInitExpr, but that seems to be unintuitive
+      // behavior.
+      constant = constWithPadding(CGM, IsPattern::No,
                                   replaceUndef(CGM, isPattern, constant));
     }
   }
 
   if (!constant) {
     initializeWhatIsTechnicallyUninitialized(Loc);
     LValue lv = MakeAddrLValue(Loc, type);
     lv.setNonGC(true);

test/CodeGen/padding-init.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s
+
+// C guarantees that brace-init with fewer initializers than members in the
+// aggregate will initialize the rest of the aggregate as-if it were static
+// initialization. In turn static initialization guarantees that padding is
+// initialized to zero bits.
+
+// CHECK: @__const.partial_init.s = private unnamed_addr constant { i8, [7 x i8], i64 } { i8 42, [7 x i8] zeroinitializer, i64 0 }, align 8
+
+// Technically, we could initialize this padding to non-zero because all of the
+// struct's members have initializers.
+
+// CHECK: @__const.init_all.s = private unnamed_addr constant { i8, [7 x i8], i64 } { i8 42, [7 x i8] zeroinitializer, i64 -2401053089374216531 }, align 8
+
+struct S {
+  char c;
+  long long l;
+};
+
+void use(struct S*);
+
+// CHECK-LABEL: @empty_braces(
+// CHECK:       %s = alloca
+// CHECK-NEXT:  %[[B:[0-9+]]] = bitcast %struct.S* %s to i8*
+// CHECK-NEXT:  call void @llvm.memset{{.*}}(i8* align 8 %[[B]], i8 0,
+// CHECK-NEXT:  call void @use(%struct.S* %s)
+void empty_braces() {
+  struct S s = {};
+  return use(&s);
+}
+
+// CHECK-LABEL: @partial_init(
+// CHECK:       %s = alloca
+// CHECK-NEXT:  %[[B:[0-9+]]] = bitcast %struct.S* %s to i8*
+// CHECK-NEXT:  call void @llvm.memcpy{{.*}}(i8* align 8 %[[B]], {{.*}}@__const.partial_init.s
+// CHECK-NEXT:  call void @use(%struct.S* %s)
+void partial_init() {
+  struct S s = { .c = 42 };
+  return use(&s);
+}
+
+// CHECK-LABEL: @init_all(
+// CHECK:       %s = alloca
+// CHECK-NEXT:  %[[B:[0-9+]]] = bitcast %struct.S* %s to i8*
+// CHECK-NEXT:  call void @llvm.memcpy{{.*}}(i8* align 8 %[[B]], {{.*}}@__const.init_all.s
+// CHECK-NEXT:  call void @use(%struct.S* %s)
+void init_all() {
+  struct S s = { .c = 42, .l = 0xdeadbeefc0fedead };
+  return use(&s);
+}