Page MenuHomePhabricator

[Sema] Perform call checking when building CXXNewExpr
ClosedPublic

Authored by lebedev.ri on Jan 20 2020, 1:57 AM.

Details

Summary

There was even a TODO for this.
The main motivation is to make use of call-site based
__attribute__((alloc_align(param_idx))) validation (D72996).

Diff Detail

Event Timeline

lebedev.ri planned changes to this revision.Jan 20 2020, 2:38 AM

Ah right, hmm, posted too soon.
I actually need to synthesize the size/align arguments.

Not only do we need to 'insert' size implicit argument,
but we may need to insert alignment implicit argument.
And while for size we can only insert nullptr,
for alignment we can actually insert the correct value.

Rebased, NFC.

rsmith added a comment.Feb 3 2020, 6:39 PM

It would be really useful to provide these extra parameters to FindAllocationFunctions too. For example, we could support direct dispatch to a size-class-specific allocation function:

void *operator new(std::size_t n) __attribute__((enable_if(n == 16, "optimized allocator for 16-byte objects")));
clang/lib/AST/ExprConstant.cpp
14464

This is wrong and may result in user-visible non-conformance; an expression of type std::align_val_t should never return true from isIntegerConstantExpr, because align_val_t is a scoped enumeration.

Can we instead change the caller (presumably the checking for the alloc_align attribute) to call a more general evaluation function rather than requiring an ICE?

clang/lib/Sema/SemaExprCXX.cpp
2084–2086

If this is not an array new, or it's an array new with a constant array bound, we can pass in a correct size. For the remaining (runtime array bound) cases, please conjure up an OpaqueValueExpr as the corresponding argument here rather than working around the nullptr argument in EvaluateWithSubstitution.

2095–2102

Please put these on the stack rather than ASTContext-allocating them. Both of these classes already support stack allocation.

lebedev.ri marked 3 inline comments as done.

@rsmith Thank you for taking a look!
Addressed all review comments.

Looks good if you change the error to a warning.

clang/lib/Sema/SemaChecking.cpp
4482–4486

Oh, I wasn't aware we gave an error for this case. We can't issue an error for passing a bad alignment to operator new -- it's valid for code to pass a bad alignment, but results in undefined behavior. The same is true for aligned_alloc and posix_memalign, so we should presumably just downgrade this from an error to a warning in general rather than (say) treating operator new as a special case.

For example, this is valid and may be reasonable in some cases:

template<int N> void *make() {
  if ((N & (N-1)) == 0)
    return operator new(N, std::align_val_t(N));
  else
    return operator new(N);
}
void *p = make<7>();
lebedev.ri added inline comments.Feb 4 2020, 12:59 PM
clang/lib/Sema/SemaChecking.cpp
4482–4486

Hmm.
I'll post that as a followup patch here.

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.
clang/lib/Sema/SemaChecking.cpp
4482–4486

Posted D73996.
I'm not sure which cases should remain as error?

lebedev.ri marked an inline comment as done.Feb 9 2020, 11:07 PM

Bump @rsmith
thanks

Rebased ontop of patch demoting call-site-based align attr checking from error into a warning, NFC.

Looks good if you change the error to a warning.

I'm going to treat that as an implicit accept since the error got changed into a warning.
Will commit this soon.

Rebased, NFC.

rsmith accepted this revision.Feb 25 2020, 2:07 PM

Looks good if you change the error to a warning.

I'm going to treat that as an implicit accept since the error got changed into a warning.

Yes, that was my intent. =) Here's an explicit "accept" for you anyway.

This revision is now accepted and ready to land.Feb 25 2020, 2:07 PM

Looks good if you change the error to a warning.

I'm going to treat that as an implicit accept since the error got changed into a warning.

Yes, that was my intent. =) Here's an explicit "accept" for you anyway.

Great. Thank you for review!
Thank you for confirming.
Will land tomorrow.

This revision was automatically updated to reflect the committed changes.