[NFC] Minor fix to tryEmitAbstract type in EmitCXXNewAllocSize

[andykaylor]

In EmitCXXNewAllocSize, when handling a constant array size, we were calling tryEmitAbstract with the type of the object being allocated. This worked out because the type was always a pointer and tryEmitAbstract only ends up using the size of the type to extend or truncate the constant, and in this case the destination type should be size_t, which is the same size as the pointer. So, this change fixes the type, but it makes no functional difference with the current constant emitter implementation.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Andy Kaylor (andykaylor)

Changes

In EmitCXXNewAllocSize, when handling a constant array size, we were calling tryEmitAbstract with the type of the object being allocated. This worked out because the type was always a pointer and tryEmitAbstract only ends up using the size of the type to extend or truncate the constant, and in this case the destination type should be size_t, which is the same size as the pointer. So, this change fixes the type, but it makes no functional difference with the current constant emitter implementation.

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CGExprCXX.cpp (+2-2)

diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index 648b9b9ed98063..83b28a16ab76b6 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -732,8 +732,8 @@ static llvm::Value *EmitCXXNewAllocSize(CodeGenFunction &CGF,
   // Emit the array size expression.
   // We multiply the size of all dimensions for NumElements.
   // e.g for 'int[2][3]', ElemType is 'int' and NumElements is 6.
-  numElements =
-    ConstantEmitter(CGF).tryEmitAbstract(*e->getArraySize(), e->getType());
+  numElements = ConstantEmitter(CGF).tryEmitAbstract(
+      *e->getArraySize(), CGF.getContext().getSizeType());
   if (!numElements)
     numElements = CGF.EmitScalarExpr(*e->getArraySize());
   assert(isa<llvm::IntegerType>(numElements->getType()));

[andykaylor]

I ran into this while porting this code to the CIR generator because the constant emitter there was asserting that the destination type was an integer type. My motivation for posting this change here is to make sure my understanding of the expected destination type is correct.

[erichkeane]

Do you have a lit test that shows where this would be wrong? Typically in cases like this we count on the AST to have given us the correct type.

Also, it isn't clear to my WHY we expect it to be size-type?

Also, size_t isn't guaranteed to be the same size as the pointer, so this could actually be a breaking change on those platforms. By standard, we don't actually have a type that makes that guarantee, though intptr_t and uintptr_t might be the closest.

Still don't really see a situation where this is troublesome though.

[andykaylor]

If I understand what tryEmitAbstract is doing correctly, the type here is the type to which we intend the constant to be converted when translating an expression of the form 'new T[C]'. In this case C is a literal, but we need to assign a type to it. The tryEmitAbstract code is creating a constant unsigned integer and truncating or extending it to the width of the type passed in as the second argument.

I think this rule from 5.3.4 of the C++11 standard applies:

Every constant-expression in a noptr-new-declarator shall be a converted constant expression (5.19) of type
std::size_t and shall evaluate to a strictly positive value.

If there is a target that clang supports for which the size of size_t is not equal to the size of ptr, that would show a change in behavior with this change. Is there such a target?

[erichkeane]

I THINK there are some 32 bit targets with 32 bit pointers and 64 bit size_t, but I'm not positive.

That said, see here:
https://godbolt.org/z/8s1dj4cac

The AST already does the cast to the correct platform-specific size. More particularly:

|   `-CXXNewExpr <line:4:5, col:14> 'int *' array Function 0x1b516b00 'operator new[]' 'void *(unsigned long)'
|     `-ImplicitCastExpr <col:13> 'unsigned long' <IntegralCast>
|       `-ImplicitCastExpr <col:13> 'short' <LValueToRValue>
|         `-DeclRefExpr <col:13> 'short' lvalue ParmVar 0x1b5162c0 'I' 'short'

See the cast to unsigned long, or size_t, which I've now confirmed is correct, as it is defined to be size_t in the standard for operator new[]. THUS, the overload resolution will always get this type 'correct'. If you've found an exception to that, it should probably be fixed in the AST.

I would expect the e->getType() to be more correct here, because it is the result of the AST being formed correctly (which, if operator new[] is ever extended either by standard or extension to allow user overrides of non-size_t array sizes here, size_t would be incorrect).

[andykaylor]

In the code before my change, the e in e->getType() is the new expression, not the constant. I'll see if I can use e->getArraySize()->getType() instead.

[erichkeane]

OH! I missed that! yes, that seems incorrect. It should be likely the type of the size expression, not the new expression.

[erichkeane]

Patch itself looks fine, still need a test that shows a case where this is done 'wrong', as well as a release note.

Wouldn't mind @rjmccall or @efriedma-quic taking a look though.

[andykaylor]

Patch itself looks fine, still need a test that shows a case where this is done 'wrong', as well as a release note.

Without a target where sizeof(size_t) != sizeof(int*) there won't be any observable difference, and I can't find an example where that condition is met. For amdgcn targets. sizeof(private int*) or sizeof(local int*) is not equal to sizeof(size_t) but I can't use 'new' for OpenCL, so I still can't show a difference.

So, I don't think this can have a test, and I don't think it needs a release note. It's just a cleanup, and as I said above, my motivation was primarily to make sure I was doing the right thing in the equivalent CIR code.

[erichkeane]

Patch itself looks fine, still need a test that shows a case where this is done 'wrong', as well as a release note.

Without a target where sizeof(size_t) != sizeof(int*) there won't be any observable difference, and I can't find an example where that condition is met. For amdgcn targets. sizeof(private int*) or sizeof(local int*) is not equal to sizeof(size_t) but I can't use 'new' for OpenCL, so I still can't show a difference.

So, I don't think this can have a test, and I don't think it needs a release note. It's just a cleanup, and as I said above, my motivation was primarily to make sure I was doing the right thing in the equivalent CIR code.

Shouldn't there be a pointer-vs-intty difference in the LLVM-IR here?

[andykaylor]

Shouldn't there be a pointer-vs-intty difference in the LLVM-IR here?

No, because in this case tryEmitAbstract() is only using the size of the type that's passed in. After a few visits, it eventually calls this function:

  llvm::Constant *ProduceIntToIntCast(const Expr *E, QualType DestType) {
    QualType FromType = E->getType();
    // See also HandleIntToIntCast in ExprConstant.cpp
    if (FromType->isIntegerType())
      if (llvm::Constant *C = Visit(E, FromType))
        if (auto *CI = dyn_cast<llvm::ConstantInt>(C)) {
          unsigned SrcWidth = CGM.getContext().getIntWidth(FromType);
          unsigned DstWidth = CGM.getContext().getIntWidth(DestType);
          if (DstWidth == SrcWidth)
            return CI;
          llvm::APInt A = FromType->isSignedIntegerType()
                              ? CI->getValue().sextOrTrunc(DstWidth)
                              : CI->getValue().zextOrTrunc(DstWidth);
          return llvm::ConstantInt::get(CGM.getLLVMContext(), A);
        }
    return nullptr;
  }

The DestType parameter is only used to resize the constant if necessary (which in this case it won't be). The equivalent function in CIR was asserting that DestType was an integer type, which is what led me to this change.

[erichkeane]

Shouldn't there be a pointer-vs-intty difference in the LLVM-IR here?

No, because in this case tryEmitAbstract() is only using the size of the type that's passed in. After a few visits, it eventually calls this function:

  llvm::Constant *ProduceIntToIntCast(const Expr *E, QualType DestType) {
    QualType FromType = E->getType();
    // See also HandleIntToIntCast in ExprConstant.cpp
    if (FromType->isIntegerType())
      if (llvm::Constant *C = Visit(E, FromType))
        if (auto *CI = dyn_cast<llvm::ConstantInt>(C)) {
          unsigned SrcWidth = CGM.getContext().getIntWidth(FromType);
          unsigned DstWidth = CGM.getContext().getIntWidth(DestType);
          if (DstWidth == SrcWidth)
            return CI;
          llvm::APInt A = FromType->isSignedIntegerType()
                              ? CI->getValue().sextOrTrunc(DstWidth)
                              : CI->getValue().zextOrTrunc(DstWidth);
          return llvm::ConstantInt::get(CGM.getLLVMContext(), A);
        }
    return nullptr;
  }

The DestType parameter is only used to resize the constant if necessary (which in this case it won't be). The equivalent function in CIR was asserting that DestType was an integer type, which is what led me to this change.

Oooh, hrmph. I find myself thinking that ProduceIntToIntCast shouldn't be necessary in most cases, since the size_t is pointer type is pretty uniform. Though I think I saw above you found some platforms we support doing this, so an assert wouldn't be a good replacement.

I can see now why this isn't really testable, this is really quite unfortunate here.