Skip to content

[NFC] Minor fix to tryEmitAbstract type in EmitCXXNewAllocSize #123433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 22, 2025
Merged

[NFC] Minor fix to tryEmitAbstract type in EmitCXXNewAllocSize #123433

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25edfca
[NFC] Minor fix to tryEmitAbstract type in EmitCXXNewAllocSize
andykaylor Jan 18, 2025
3bab57b
Get the size from e->getArraySize() rather than using size_t
andykaylor Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clang/lib/CodeGen/CGExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*e->getArraySize(), (*e->getArraySize())->getType());
if (!numElements)
numElements = CGF.EmitScalarExpr(*e->getArraySize());
assert(isa<llvm::IntegerType>(numElements->getType()));
Expand Down
Loading