[CodeGen] Remove the check that allowed non-POD compound literals to be directly evaluated into the destination even when it might alias the source

[ahatanak]

Evaluate all aggregate compound literals into a temporary and then copy it to the destination if aliasing is possible.

This fixes a latent issue exposed by #154490, where evaluating the RHS directly into the destination could ignore potential aliasing.

rdar://164094548

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Akira Hatanaka (ahatanak)

Changes

Evaluate all aggregate compound literals into a temporary and then copy it to the destination if aliasing is possible.

This fixes a latent issue exposed by #154490, where evaluating the RHS directly into the destination could ignore potential aliasing.

rdar://164094548

---

Full diff: https://github.com/llvm/llvm-project/pull/167344.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGExprAgg.cpp (+3-4)
• (added) clang/test/CodeGenObjC/nontrivial-c-struct.m (+59)

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index eee397f1f3d19..4e61a6f61948f 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -755,10 +755,9 @@ void AggExprEmitter::VisitOpaqueValueExpr(OpaqueValueExpr *e) {
 
 void
 AggExprEmitter::VisitCompoundLiteralExpr(CompoundLiteralExpr *E) {
-  if (Dest.isPotentiallyAliased() &&
-      E->getType().isPODType(CGF.getContext())) {
-    // For a POD type, just emit a load of the lvalue + a copy, because our
-    // compound literal might alias the destination.
+  if (Dest.isPotentiallyAliased()) {
+    // Just emit a load of the lvalue + a copy, because our compound literal
+    // might alias the destination.
     EmitAggLoadOfLValue(E);
     return;
   }
diff --git a/clang/test/CodeGenObjC/nontrivial-c-struct.m b/clang/test/CodeGenObjC/nontrivial-c-struct.m
new file mode 100644
index 0000000000000..fa4fa223bc2d9
--- /dev/null
+++ b/clang/test/CodeGenObjC/nontrivial-c-struct.m
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple arm64e-apple-ios18 -fptrauth-calls -fptrauth-intrinsics -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_S0:.*]] = type { i32, i32, ptr }
+// CHECK: %[[STRUCT_S1:.*]] = type { ptr, ptr }
+
+// This struct isn't POD because it has an address-discriminated ptrauth
+// field.
+typedef struct {
+  int f0, f1;
+  int * __ptrauth(1,1,50) f2;
+} S0;
+
+// This struct isn't POD because it has an address-discriminated ptrauth
+// field and an ARC ObjC pointer field.
+typedef struct {
+  id f0;
+  int * __ptrauth(1,1,50) f1;
+} S1;
+
+// CHECK: define void @compound_literal_assignment0(ptr noundef %[[P:.*]])
+// CHECK: %[[P_ADDR:.*]] = alloca ptr, align 8
+// CHECK-NEXT: %[[_COMPOUNDLITERAL:.*]] = alloca %[[STRUCT_S0]], align 8
+// CHECK-NEXT: store ptr %[[P]], ptr %[[P_ADDR]], align 8
+// CHECK-NEXT: %[[V0:.*]] = load ptr, ptr %[[P_ADDR]], align 8
+// CHECK-NEXT: %[[F0:.*]] = getelementptr inbounds nuw %[[STRUCT_S0]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 0
+// CHECK-NEXT: %[[V1:.*]] = load ptr, ptr %[[P_ADDR]], align 8
+// CHECK-NEXT: %[[F1:.*]] = getelementptr inbounds nuw %[[STRUCT_S0]], ptr %[[V1]], i32 0, i32 1
+// CHECK-NEXT: %[[V2:.*]] = load i32, ptr %[[F1]], align 4
+// CHECK-NEXT: store i32 %[[V2]], ptr %[[F0]], align 8
+// CHECK-NEXT: %[[F11:.*]] = getelementptr inbounds nuw %[[STRUCT_S0]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 1
+// CHECK-NEXT: %[[V3:.*]] = load ptr, ptr %[[P_ADDR]], align 8
+// CHECK-NEXT: %[[F02:.*]] = getelementptr inbounds nuw %[[STRUCT_S0]], ptr %[[V3]], i32 0, i32 0
+// CHECK-NEXT: %[[V4:.*]] = load i32, ptr %[[F02]], align 8
+// CHECK-NEXT: store i32 %[[V4]], ptr %[[F11]], align 4
+// CHECK-NEXT: %[[F2:.*]] = getelementptr inbounds nuw %[[STRUCT_S0]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 2
+// CHECK-NEXT: store ptr null, ptr %[[F2]], align 8
+// CHECK-NEXT: call void @__copy_assignment_8_8_t0w8_pa1_50_8(ptr %[[V0]], ptr %[[_COMPOUNDLITERAL]])
+// CHECK-NEXT: ret void
+
+void compound_literal_assignment0(S0 *p) {
+  *p = (S0){.f0 = p->f1, .f1 = p->f0};
+}
+
+// CHECK: define void @compound_literal_assignment1(ptr noundef %[[P:.*]])
+// CHECK: %[[P_ADDR:.*]] = alloca ptr, align 8
+// CHECK-NEXT: %[[_COMPOUNDLITERAL:.*]] = alloca %[[STRUCT_S1]], align 8
+// CHECK-NEXT: store ptr %[[P]], ptr %[[P_ADDR]], align 8
+// CHECK-NEXT: %[[V0:.*]] = load ptr, ptr %[[P_ADDR]], align 8
+// CHECK-NEXT: %[[F0:.*]] = getelementptr inbounds nuw %[[STRUCT_S1]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 0
+// CHECK-NEXT: store ptr null, ptr %[[F0]], align 8
+// CHECK-NEXT: %[[F1:.*]] = getelementptr inbounds nuw %[[STRUCT_S1]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 1
+// CHECK-NEXT: store ptr null, ptr %[[F1]], align 8
+// CHECK-NEXT: call void @__copy_assignment_8_8_s0_pa1_50_8(ptr %[[V0]], ptr %[[_COMPOUNDLITERAL]])
+// CHECK-NEXT: call void @__destructor_8_s0(ptr %[[_COMPOUNDLITERAL]])
+// CHECK-NEXT: ret void
+
+void compound_literal_assignment1(S1 *p) {
+  *p = (S1){};
+}

[ahatanak]

There's a bug in QualType::isCXX98PODType where it returns false for C structs that have ObjC pointer fields when ARC is enabled, which I'm planning to fix later.

[efriedma-quic]

LGTM

[ojhunt]

LGTM as well, the id issue is probably going to be a misery to fix as we likely have ABI that depends on structs like this being considered POD for calling convention purposes :(

[ojhunt]

I'm a little concerned about potential codegen regressions though, might be worth watching for regressions showing up in perf bots

[efriedma-quic]

I doubt we'll see regressions on public bots. This should have no effect for C code because normally all C types are POD (ignoring ARC/ptrauth/etc.), and should have basically no effect for C++ code because compound literals are very rare in C++.

[ojhunt]

I doubt we'll see regressions on public bots. This should have no effect for C code because normally all C types are POD (ignoring ARC/ptrauth/etc.), and should have basically no effect for C++ code because compound literals are very rare in C++.

Sorry - I was meaning in the specific context of changing arc behavior, from the existing code we can see that even under arc, a struct contain a field that arc would do arc things with would be labeled pod, even though strictly speaking it should not be. With explicit ptrauth qualifiers we accept that C structs do weird things, but if we were to change the behavior of "is pod" to report a struct containing arc'd fields as not being pod (which is strictly correct), it seems entirely plausible we'd get some kinds of abi breakage.

e.g. nothing related to this specific PR, it was a comment to Akira on what we need to consider if we do try to address the questionable labeling of types with arc'd fields.