[clang][NFC] Use a more accurate size type in the new operation, prevent issues when PointerWidth and the SizeType width are not same.

[shafik]

Adding reviewers for more visibility.

[cor3ntin]

Can you add a test case and a more detailed description of the issue ? Thanks!

[CaprYang]

In D152160#4396007, @cor3ntin wrote:

Can you add a test case and a more detailed description of the issue ? Thanks!

here: https://github.com/llvm/llvm-project/issues/63116
Sorry. I don't know how to building a backend where PointerWidth and SizeTy width are not the same. so I can't add the test... can you tell me how to do it?

[cor3ntin]

@aaron.ballman you have an opinion on that?

[erichkeane]

Presumably this is with a custom downstream 'target' here? I don't see how we'd do that either. However, I think I've seen basically this same patch up for review about a month and a half ago that I think we did a few round of reviews on? Or am I misremembering this?

This needs to be submitted for review with 'context' as well (-U999999 when generating the patch).

I think this ends up being a 'fine' change, I don't have a problem with it, and it seems more 'accurate' to make it the size type. So 1 nit, but else I think this is going to be fine.

clang/lib/Sema/SemaExprCXX.cpp

2646

Since you're capturing the size type above in SizeTy, just use it here too.

[aaron.ballman]

In D152160#4446936, @cor3ntin wrote:

@aaron.ballman you have an opinion on that?

I'm not usually in favor of changes without test coverage, but this is effectively an NFC change as far as Clang is concerned so I'm okay with the changes. That said, the patch title seems wrong (there's no crash being fixed by this within Clang, otherwise we could test the changes), and a summary should be added justifying what's changing and why. Erich's nit should be addressed as well.

[CaprYang]

In D152160#4448583, @erichkeane wrote:

Presumably this is with a custom downstream 'target' here? I don't see how we'd do that either. However, I think I've seen basically this same patch up for review about a month and a half ago that I think we did a few round of reviews on? Or am I misremembering this?

This needs to be submitted for review with 'context' as well (-U999999 when generating the patch).

I think this ends up being a 'fine' change, I don't have a problem with it, and it seems more 'accurate' to make it the size type. So 1 nit, but else I think this is going to be fine.

It has been updated with diff -U999999, thanks

clang/lib/Sema/SemaExprCXX.cpp

2646

Yes, corrected it, thanks

[CaprYang]

In D152160#4498259, @aaron.ballman wrote:

In D152160#4446936, @cor3ntin wrote:

@aaron.ballman you have an opinion on that?

I'm not usually in favor of changes without test coverage, but this is effectively an NFC change as far as Clang is concerned so I'm okay with the changes. That said, the patch title seems wrong (there's no crash being fixed by this within Clang, otherwise we could test the changes), and a summary should be added justifying what's changing and why. Erich's nit should be addressed as well.

You're right, I have changed the title and marked NFC. Do you think this is okay?

[aaron.ballman]

LGTM! Do you need someone to commit this on your behalf? If so, what name and email address would you like used for patch attribution?

[CaprYang]

In D152160#4507108, @aaron.ballman wrote:

LGTM! Do you need someone to commit this on your behalf? If so, what name and email address would you like used for patch attribution?

Yes, please commit this for me, thx!
author: CaprYang <[e-mail]>

clang/lib/Sema/SemaExprCXX.cpp

bool Sema::FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
                                    QualType AllocType, bool IsArray,
                                    bool &PassAlignment, MultiExprArg PlaceArgs,
                                    FunctionDecl *&OperatorNew,
                                    FunctionDecl *&OperatorDelete,
                                    bool Diagnose) {
   // --- Choosing an allocation function ---
   // C++ 5.3.4p8 - 14 & 18
   // 1) If looking in AFS_Global scope for allocation functions, only look in
   //    the global scope. Else, if AFS_Class, only look in the scope of the
   //    allocated class. If AFS_Both, look in both.
   // 2) If an array size is given, look for operator new[], else look for
   //   operator new.
   // 3) The first argument is always size_t. Append the arguments from the
   //   placement form.
 
   SmallVector<Expr*, 8> AllocArgs;
   AllocArgs.reserve((PassAlignment ? 2 : 1) + PlaceArgs.size());
 
   // We don't care about the actual value of these arguments.
   // FIXME: Should the Sema create the expression and embed it in the syntax
   // tree? Or should the consumer just recalculate the value?
   // FIXME: Using a dummy value will interact poorly with attribute enable_if.
-  IntegerLiteral Size(
-      Context,
-      llvm::APInt::getZero(
-          Context.getTargetInfo().getPointerWidth(LangAS::Default)),
-      Context.getSizeType(), SourceLocation());
+  QualType SizeTy = Context.getSizeType();
+  unsigned SizeTyWidth = Context.getTypeSize(SizeTy);
+  IntegerLiteral Size(Context, llvm::APInt::getZero(SizeTyWidth), SizeTy,
+                      SourceLocation());
   AllocArgs.push_back(&Size);
 
   QualType AlignValT = Context.VoidTy;
   if (PassAlignment) {
     DeclareGlobalNewDelete();
     AlignValT = Context.getTypeDeclType(getStdAlignValT());
   }
   CXXScalarValueInitExpr Align(AlignValT, nullptr, SourceLocation());