[clang codegen] Fix alignment of "Address" for incomplete array pointer.

[efriedma]

Summary

The code was assuming all incomplete types don't have meaningful alignment, but that clearly isn't true.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45710

[rsmith]

clang/lib/CodeGen/CodeGenFunction.cpp

176 ↗

(On Diff #260792)

I don't know if it matters in practice, but this is still wrong. An incomplete type can have a known alignment, for a case like struct alignas(32) S;. Perhaps we should remove this test entirely and call getTypeAlignIfKnown instead of getTypeAlign[InChars] below.

[efriedma]

clang/lib/CodeGen/CodeGenFunction.cpp

176 ↗

(On Diff #260792)

I can't think of any way to observe the alignment computed by getNaturalTypeAlignment for an incomplete class. We usually only use the alignment computed by getNaturalTypeAlignment() to set the alignment of memory operations, and you can't do any memory operations with an incomplete class.

But the result might be easier to read, in any case.

[rjmccall]

clang/lib/CodeGen/CodeGenFunction.cpp

176 ↗

(On Diff #260792)

If there's a getTypeAlignIfKnown(), it would be better to use it.

[efriedma]

"Address" the review comments. Not really happy with this, but not sure what else to do.

[efriedma]

Hang on, I submitted this too early. Need to look a bit more.

[efriedma]

This should work correctly, now, I think?

[rjmccall]

clang/lib/CodeGen/CodeGenModule.cpp

 
 CharUnits CodeGenModule::getNaturalTypeAlignment(QualType T,
                                                  LValueBaseInfo *BaseInfo,
                                                  TBAAAccessInfo *TBAAInfo,
                                                  bool forPointeeType) {
   if (TBAAInfo)
     *TBAAInfo = getTBAAAccessInfo(T);
 
+  // FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown. But
+  // that doesn't return the information we need to compute BaseInfo.
+
   // Honor alignment typedef attributes even on incomplete types.
   // We also honor them straight for C++ class types, even as pointees;
   // there's an expressivity gap here.
   if (auto TT = T->getAs<TypedefType>()) {
     if (auto Align = TT->getDecl()->getMaxAlignment()) {
       if (BaseInfo)
         *BaseInfo = LValueBaseInfo(AlignmentSource::AttributedType);
       return getContext().toCharUnitsFromBits(Align);
     }
   }
 
+  bool AlignForArray = T->isArrayType();
+
+  // Analyze the base element type, so we don't get confused by incomplete
+  // array types.
+  T = getContext().getBaseElementType(T);
+
+  if (T->isIncompleteType()) {
+    // We could try to replicate the logic from
+    // ASTContext::getTypeAlignIfKnown, but nothing uses the alignment if the
+    // type is incomplete, so it's impossible to test. We could try to reuse
+    // getTypeAlignIfKnown, but that doesn't return the information we need
+    // to set BaseInfo.  So just ignore the possibility that the alignment is
+    // greater than one.
+    if (BaseInfo)
+      *BaseInfo = LValueBaseInfo(AlignmentSource::Type);
+    return CharUnits::One();
+  }
+
   if (BaseInfo)
     *BaseInfo = LValueBaseInfo(AlignmentSource::Type);
 
   CharUnits Alignment;
-  if (T->isIncompleteType()) {
-    Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best.
-  } else {
-    // For C++ class pointees, we don't know whether we're pointing at a
-    // base or a complete object, so we generally need to use the
-    // non-virtual alignment.
-    const CXXRecordDecl *RD;
-    if (forPointeeType && (RD = T->getAsCXXRecordDecl())) {
-      Alignment = getClassPointerAlignment(RD);
-    } else {
-      Alignment = getContext().getTypeAlignInChars(T);
-      if (T.getQualifiers().hasUnaligned())
-        Alignment = CharUnits::One();
-    }
+  if (forPointeeType && !AlignForArray && (RD = T->getAsCXXRecordDecl())) {
 
-    // Cap to the global maximum type alignment unless the alignment
-    // was somehow explicit on the type.
-    if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) {
-      if (Alignment.getQuantity() > MaxAlign &&
-          !getContext().isAlignmentRequired(T))
-        Alignment = CharUnits::fromQuantity(MaxAlign);
-    }
-  }
   return Alignment;
 }
 
 bool CodeGenModule::stopAutoInit() {
   unsigned StopAfter = getContext().getLangOpts().TrivialAutoVarInitStopAfter;
   if (StopAfter) {
     // This number is positive only when -ftrivial-auto-var-init-stop-after=* is
     // used

clang/test/CodeGenCXX/alignment.cpp

void g() {
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B:%.*]]*
     // CHECK: [[ARRAY_P:%.*]] = getelementptr inbounds [[B]], [[B]]* [[B_P]], i32 0, i32 2
     // CHECK: [[T0:%.*]] = bitcast [[ARRAY]]* [[RESULT]] to i8*
     // CHECK: [[T1:%.*]] = bitcast [[ARRAY]]* [[ARRAY_P]] to i8*
     // CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 64 [[T0]], i8* align 8 [[T1]], i64 16, i1 false)
     D d;
     AlignedArray result = d.bArray;
   }
+
+  // CHECK-LABEL: @_ZN5test11hEPA_NS_1BE
+  void h(B (*b)[]) {
+    // CHECK: [[RESULT:%.*]] = alloca [[ARRAY]], align 64
+    // CHECK: [[B_P:%.*]] = load [0 x [[B]]]*, [0 x [[B]]]**
+    // CHECK: [[ELEMENT_P:%.*]] = getelementptr inbounds [0 x [[B]]], [0 x [[B]]]* [[B_P]], i64 0
+    // CHECK: [[ARRAY_P:%.*]] = getelementptr inbounds [[B]], [[B]]* [[ELEMENT_P]], i32 0, i32 2
+    // CHECK: [[T0:%.*]] = bitcast [[ARRAY]]* [[RESULT]] to i8*
+    // CHECK: [[T1:%.*]] = bitcast [[ARRAY]]* [[ARRAY_P]] to i8*
+    // CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 64 [[T0]], i8* align 16 [[T1]], i64 16, i1 false)
+    AlignedArray result = (*b)->bArray;
+  }
 }
+
+// CHECK-LABEL: @_Z22incomplete_array_derefPA_i
+// CHECK: load i32, i32* {{%.*}}, align 4
+int incomplete_array_deref(int (*p)[]) { return (*p)[2]; }