[CodeGen] Use ABI alignment for C++ new expressions

[BertalanD]

Summary

In case of placement new, if we do not know the alignment of the
operand, we can't assume it has the preferred alignment. It might be
e.g. a pointer to a struct member which follows ABI alignment rules.

This makes UBSAN no longer report "constructor call on misaligned
address" when constructing a double into a struct field of type double
on i686. The psABI specifies an alignment of 4 bytes, but the preferred
alignment used by Clang is 8 bytes.

We now use ABI alignment for allocating new as well, as the preferred
alignment should be used for over-aligning e.g. local variables, which
isn't relevant for ABI code dealing with operator new. AFAICT there
wouldn't be problems either way though.

Fixes #54845

I don't have commit rights, so if this patch is accepted, I'd like someone to commit it for me.
Author info: "Daniel Bertalan <[e-mail]>"

[BertalanD]

Added a test case.

[BertalanD]

Removed accidental execute permission from the test.

[rjmccall]

It should probably be the ABI alignment in all cases; I think the preferred alignment is just supposed to be used to opportunistically over-align things like e.g. local variables, which doesn't seem relevant for the ABI code involving a call to operator new.

[BertalanD]

In D124736#3485128, @rjmccall wrote:

It should probably be the ABI alignment in all cases; I think the preferred alignment is just supposed to be used to opportunistically over-align things like e.g. local variables, which doesn't seem relevant for the ABI code involving a call to operator new.

Right, that makes sense. I'll update this patch tomorrow. Thanks for the review :)

[BertalanD]

Use ABI alignment for allocating operator new as well.

Sorry for the delay.

[rjmccall]

LGTM

[BertalanD]

Could you please commit this for me, @rjmccall?

I'm wondering if there's a chance this could be backported to the LLVM 14 branch. We've been running into the linked issue in production, it would be great if it were fixed in the next point release.

clang/lib/CodeGen/CGExprCXX.cpp

else if (ILE)
       minElements = ILE->getNumInits();
   }
 
   llvm::Value *numElements = nullptr;
   llvm::Value *allocSizeWithoutCookie = nullptr;
   llvm::Value *allocSize =
     EmitCXXNewAllocSize(*this, E, minElements, numElements,
                         allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign = getContext().getTypeAlignInChars(allocType);
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
   Address allocation = Address::invalid();
   CallArgList allocatorArgs;
   if (allocator->isReservedGlobalPlacementOperator()) {
     assert(E->getNumPlacementArgs() == 1);
     const Expr *arg = *E->placement_arguments().begin();

clang/test/CodeGenCXX/pr54845.cpp

+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.000000e+00, {{.*}}, align 4
+  new (d) double(0);
+}