This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Teach CallEvent about C++17 aligned new.
ClosedPublic

Authored by NoQ on Oct 5 2018, 6:03 PM.

Details

Summary

In C++17, when class C has large alignment value, a special case of overload resolution rule kicks in for expression new C that causes the aligned version of operator new() to be called. The aligned new has two arguments: size and alignment. However, the new-expression has only one argument: the construct-expression for C(). This causes a false positive in core.CallAndMessage's check for matching number of arguments and number of parameters.

Update CXXAllocatorCall, which is a CallEvent sub-class for operator new calls within new-expressions, so that the number of arguments always matched the number of parameters.

Dunno, maybe we should instead abandon the idea of reserving a whole argument/parameter index for each of those implicit arguments that aren't even represented by an expression in the AST.

Side note: Ugh, we never supported passing size into operator new() calls, even though it's known in compile time. And now also alignment. They are currently symbolic (unconstrained) within allocator calls.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Oct 5 2018, 6:03 PM
Szelethus added inline comments.Oct 6 2018, 4:51 AM
include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
929 ↗(On Diff #168556)

Can you include doxygen comments too, or make these doxygen comments?

xazax.hun accepted this revision.Oct 8 2018, 2:22 AM

LGTM!

I agree that it would make sense to either not have arguments that are not represented in the AST or create expressions for those implicit arguments.

This revision is now accepted and ready to land.Oct 8 2018, 2:22 AM
This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Oct 15 2018, 11:03 AM

Whoops, almost forgot to doxygen-ize comments. Landed in rC344540.

I'm not sure if this implementation is correct.

I'm expecting this checker code not to crash:

const auto *alloc = dyn_cast<CXXAllocatorCall>(&Call);
if (!alloc)
  return;

const int NumImpArgs = alloc->getNumImplicitArgs();
errs() << "alloc->getNumImplicitArgs(): " << NumImpArgs << '\n'; // prints 1
for (int i = 0; i < NumImpArgs; ++i)
  errs() << "> " << alloc->getPlacementArgExpr(i) << '\n'; // crash: assertion violated

const int NumArgs = alloc->getNumArgs();
errs() << "alloc->getNumArgs(): " << NumArgs << '\n';
for (int i = NumImpArgs; i < NumArgs; ++i)
  errs() << "> " << alloc->getArgExpr(i) << '\n';

Analyzed code:

void foo() {
  int *p = new int;
}

Assertion:

clang: ../../clang/include/clang/AST/ExprCXX.h:2272: clang::Expr* clang::CXXNewExpr::getPlacementArg(unsigned int): Assertion `(I < getNumPlacementArgs()) && "Index out of range!"' failed.

I'm planning to improve the MallocChecker using CallEvents directly, instead of using the underlaying CallExpr or CXXNewExpr objects in MallocChecker::checkCXXNewOrCXXDelete.
Am I misusing something? @NoQ

NoQ added a comment.Nov 5 2020, 7:50 PM

Everything looks good to me here. The new-expression new int; has 1 implicit argument (allocation size passed to the implementation of operator new, the value is probably 4) and 0 placement arguments (the ones that are explicitly written down after new and before int). See also https://en.cppreference.com/w/cpp/language/new#Placement_new.

In D52957#2377914, @NoQ wrote:

Everything looks good to me here. The new-expression new int; has 1 implicit argument (allocation size passed to the implementation of operator new, the value is probably 4) and 0 placement arguments (the ones that are explicitly written down after new and before int). See also https://en.cppreference.com/w/cpp/language/new#Placement_new.

I'm still confused.

alloc->getNumImplicitArgs(): 1
alloc->getNumArgs(): 1
alloc->getArgExpr(0): nullptr

I would expect alloc->getArgExpr(0) to return the implicit parameter, aka. the size of the placement new.
I can not see any other getXX function which seems applicable. My best bet was the getPlacementArgExpr thus I raised this issue here.

How can I access the placement new's allocation size of an CXXAllocatorCall if not via getArgExpr()?

NoQ added a comment.Nov 6 2020, 9:00 AM

You cannot have an argument expression because there's no argument expression anywhere in the AST. There's an argument, but it's not computed as a value of any syntactic expression. If there was no argument, getArgExpr(0) would have crashed; but it returns a nullptr which indicates that there's no expression to return.

The argument value can be computed by taking the size of the type (and aligning to the requested alignment, i guess(?)) and multiplying it by array size (for which there is an expression) in case of array new. It'd be great to write down these computations once in the CallEvent class and then re-use them.

I guess the actual shocking truth here is that we've never performed these computations when inlining the allocators; the size argument that's bound to the size parameter in the Store while the allocator body is inlined ended up being a fresh symbol, which is not correct.

In D52957#2379330, @NoQ wrote:

You cannot have an argument expression because there's no argument expression anywhere in the AST. There's an argument, but it's not computed as a value of any syntactic expression. If there was no argument, getArgExpr(0) would have crashed; but it returns a nullptr which indicates that there's no expression to return.

Aa, now I see. Thanks.

The argument value can be computed by taking the size of the type (and aligning to the requested alignment, i guess(?)) and multiplying it by array size (for which there is an expression) in case of array new. It'd be great to write down these computations once in the CallEvent class and then re-use them.

Should I provide them as member functions to the CXXAllocatorCall class?
Something like size_t getAlignment() and size_t getAllocationSize()?

I guess the actual shocking truth here is that we've never performed these computations when inlining the allocators; the size argument that's bound to the size parameter in the Store while the allocator body is inlined ended up being a fresh symbol, which is not correct.

I might miss something to understand this. Could you elaborate on that if you think is related?

NoQ added a comment.Nov 9 2020, 2:04 PM
In D52957#2379330, @NoQ wrote:

The argument value can be computed by taking the size of the type (and aligning to the requested alignment, i guess(?)) and multiplying it by array size (for which there is an expression) in case of array new. It'd be great to write down these computations once in the CallEvent class and then re-use them.

Should I provide them as member functions to the CXXAllocatorCall class?
Something like size_t getAlignment() and size_t getAllocationSize()?

Yes. Note that allocation size is not necessarily concrete, so you'll have to return an SVal there. Alignment, i guess, is always concrete (?) you'll probably still want to return an SVal because a lot of users will want an SVal anyway.

I guess the actual shocking truth here is that we've never performed these computations when inlining the allocators; the size argument that's bound to the size parameter in the Store while the allocator body is inlined ended up being a fresh symbol, which is not correct.

I might miss something to understand this. Could you elaborate on that if you think is related?

For CXXAllocatorCall's implicit arguments getArgSVal() always fails and returns an UnknownVal as it stumbles upon lack of expression and doesn't know how to work around it. In particular, it fails in addParameterValuesToBindings() which leaves the respective Store bindings empty.