[llvm-branch-commits] [clang] 4d502f1 - [clang] Skip stores in init for fields that are empty structs

[Tobias Hieta via llvm-branch-commits]

Author: Martin Storsjö
Date: 2023-08-16T08:14:54+02:00
New Revision: 4d502f14aca55b8378e75373639cbee305c8c2ad

URL: https://github.com/llvm/llvm-project/commit/4d502f14aca55b8378e75373639cbee305c8c2ad
DIFF: https://github.com/llvm/llvm-project/commit/4d502f14aca55b8378e75373639cbee305c8c2ad.diff

LOG: [clang] Skip stores in init for fields that are empty structs

An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes https://github.com/llvm/llvm-project/issues/64253, and
possibly https://github.com/llvm/llvm-project/issues/64077
and https://github.com/llvm/llvm-project/issues/64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332

(cherry picked from commit d60c3d08e78dfbb4b180776b83e910d810e1f36a)

Added: 
    clang/test/CodeGenCXX/ctor-empty-nounique.cpp

Modified: 
    clang/lib/CodeGen/CGCall.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 51f43b0797fd0f..fcc1620f7a0436 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5759,9 +5760,14 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
         DestIsVolatile = false;
       }
 
-      // If the value is offset in memory, apply the offset now.
-      Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-      CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+      // An empty record can overlap other data (if declared with
+      // no_unique_address); omit the store for such types - as there is no
+      // actual data to store.
+      if (!isEmptyRecord(getContext(), RetTy, true)) {
+        // If the value is offset in memory, apply the offset now.
+        Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+        CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+      }
 
       return convertTempToRValue(DestPtr, RetTy, SourceLocation());
     }

diff  --git a/clang/test/CodeGenCXX/ctor-empty-nounique.cpp b/clang/test/CodeGenCXX/ctor-empty-nounique.cpp
new file mode 100644
index 00000000000000..f01cad1dacf266
--- /dev/null
+++ b/clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)