[clang] Make init for empty no_unique_address fields a no-op write

[mstorsjo]

Summary

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.

The existing clang tests in CodeGenCXX/tail-padding.cpp contain
cases of fields that have no_unique_address on non-empty struct
members; the check for isEmptyRecord() is needed to retain the
previous behaviour in that test.

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.

[efriedma]

I see what you're getting at here... but I don't think this works quite right. If the empty class has a non-trivial constructor, we have to pass the correct "this" address to that constructor. Usually a constructor for an empty class won't do anything with that address, but it could theoretically do something with it.

In order to preserve the address in the cases we need it, we need a different invariant: the handling for each aggregate expression in EmitAggExpr needs to ensure it doesn't store anything to an empty class. Which is unfortunately more fragile than I'd like, but I can't think of a better approach. This check needs to happen further down the call stack. Maybe put it in CodeGenFunction::EmitCall.

[cor3ntin]

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

2

This should probably get another run line for powerpc64le

[mstorsjo]

In D157332#4568341, @efriedma wrote:

I see what you're getting at here... but I don't think this works quite right. If the empty class has a non-trivial constructor, we have to pass the correct "this" address to that constructor. Usually a constructor for an empty class won't do anything with that address, but it could theoretically do something with it.

Hmm, I think I see. In this specific case, there's no constructor for the empty class invoked at all, we call the function f() which returns an struct S (which is empty). But if we'd remove the initialization of y(f()) and add a constructor to S(), I see that we generate code that is indeed wrong in that aspect.

In order to preserve the address in the cases we need it, we need a different invariant: the handling for each aggregate expression in EmitAggExpr needs to ensure it doesn't store anything to an empty class. Which is unfortunately more fragile than I'd like, but I can't think of a better approach. This check needs to happen further down the call stack. Maybe put it in CodeGenFunction::EmitCall.

Ok, I'll see if I can look further into that... (I don't have a huge amount of time for it at the moment, so if someone else with an interest in the area, e.g. @crtrott or @amyk want to dig into it, feel free!)

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

2

Sure, I could do that. (Initially I had both, but thought people would consider it unnecessary duplication.)

[crtrott]

Question: does this bug potentially affect code generation for AMD/NVIDIA/Intel GPUs?

[crtrott]

To answer my own question: I was able to reproduce the bug when compiling for NVIDIA GPUs, I was not able to reproduce it for AMD GPUs yet, and I didn't try for Intel GPUs.

[mstorsjo]

In D157332#4570290, @crtrott wrote:

Question: does this bug potentially affect code generation for AMD/NVIDIA/Intel GPUs?

I believe the easiest way to test that is to try compiling struct S {}; S ret() { return S(); } into IR and checking the signature - if it returns void, the target should be immune to this bug, otherwise the bug probably is present.

[mstorsjo]

Plumb the info from EmitInitializerForField through AggValueSlot and ReturnValueSlot to EmitCall.

The fields/variables could use better names.

[efriedma]

You can't, in general, check whether a type is stored in a no_unique_address field. Consider the following; the bad store is in the constructor S2, but the relevant no_unique_address attribute doesn't even show up until the definition of S3.

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() {}

So we have to suppress all stores of empty types, whether or not we see a no_unique_address attribute.

Given that, I don't think the isDummy field is necessary; the important part is just whether the type is empty, and you can derive that directly from the type itself.

[mstorsjo]

Updated to just check isEmptyRecord in EmitCall.

The second part of the test probably is kinda redundant/pointless at this point, and I guess the test comment needs updating too; can do that later when the implementation is fine.

[efriedma]

This approach looks fine.

clang/lib/CodeGen/CGCall.cpp

5781

The isEmptyRecord call could use a comment briefly explaining that empty records can overlap with other data.

The existing comment about the offset probably belongs inside the if statement.

[mstorsjo]

Move the existing comment into the new if statement, add a comment to the new if. Add a comment to the second part of the testcase, which probably is good to keep anyway.

[mstorsjo]

Thanks a lot for the guidance!

[efriedma]

LGTM

clang/lib/CodeGen/CGCall.cpp

 //===--- CGCall.cpp - Encapsulate calling convention details --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // These classes wrap the information about a call or function
 // definition used to handle ABI compliancy.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/Attr.h"
case ABIArgInfo::Direct: {
       Address DestPtr = ReturnValue.getValue();
       bool DestIsVolatile = ReturnValue.isVolatile();
 
       if (!DestPtr.isValid()) {
         DestPtr = CreateMemTemp(RetTy, "coerce");
         DestIsVolatile = false;
       }
 
       // If the value is offset in memory, apply the offset now.
+      if (!isEmptyRecord(getContext(), RetTy, true)) {
         Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
         CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+      }
 
       return convertTempToRValue(DestPtr, RetTy, SourceLocation());
     }
 
     case ABIArgInfo::Expand:
     case ABIArgInfo::IndirectAliased:
       llvm_unreachable("Invalid ABI kind for return argument");
     }

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

+// 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
+
+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)