[cfe-dev] Representing the allocation in CXXNewExpr

[Jordan Rose]

On Mar 30, 2012, at 13:08, Sebastian Redl wrote:

> With the number of bits needed by flags down this much, is it now feasible to sink this into the shared bitfield of Stmt?

Good point, didn't think of that. Changed.


>> --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h
>> +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h
> 
> This looks unrelated.

Without it, the analyzer crashes when trying to evaluate a CXXOperatorCallExpr that uses an implicitly-declared new. It's not /directly/ related but it's part of "not breaking tests". (Before this we didn't even look at the allocation.)


>> +    // FIXME: Can the array size ever throw?
>> +    if (const Expr *ArraySize = cast<CXXNewExpr>(this)->getArraySize())
>> +      CT = MergeCanThrow(CT, ArraySize->CanThrow(C));
> 
> Of course it can throw.
> new int[callSomeFunction()]

At some point I read about C++ not having VLAs and crossed wires in my head; when I realized my mistake I never removed the comment. Thanks.


>> +  } else if (Kind == OO_New || Kind == OO_Array_New) {
>> +    // FIXME: Should the array brackets/array size be included for new[]?
>> +    if (getRParenLoc().isValid())
>> +      // There are explicit placement args.
>> +      return SourceRange(getArg(0)->getSourceRange().getBegin(), getRParenLoc());
>> +    else
>> +      // There are no explicit placement args.
>> +      return getArg(0)->getSourceRange();
> 
> I think it would be better, but perhaps inconsistent with non-array new, since I don't think we include the type range there, do we?

Right...we /could/ include the type range there as well, but that seems wrong. "new Foo" is not exactly a single unit in "new Foo(1,2,3)", especially if the allocator being used is global. It's a tradeoff.


> I think it would make more sense to leave the printing of the new and the placement arguments to the CXXOperatorCallExpr. But I'm not very sure about this opinion.
> 
> It appears that there are quite a few places that get more complicated with this, e.g. the need to represent the allocation size expression in the AST. We need to find a way to improve this.

This is essentially the cause of ALL the complications. I considered a new expression type CXXAllocSizeExpr, or a new kind of UnaryExprOrTypeTraitExpr, but even then there's still cases where we treat the size argument differently:

- Printing the new-expression. CXXOperatorCallExpr would be taught to skip the size argument when it's an OO_New or OO_Array_New.
- Itanium name-mangling, which currently skips the size argument. The Itanium spec does seem to imply that this is correct (the size is skipped for the expression but not for the allocator function's name), but there's no clause dealing specifically with new and new[].
- TreeTransform, which currently rebuilds the new expression /and/ the allocation call from scratch (because the two aren't distinct in Sema). This probably wouldn't be /too/ hard to change; I just haven't done it yet.
- CodeGen: Delete cleanups if the allocator fails. We're already keeping all the placement args around, though...no reason not to just extract the size argument from there too.
- CodeGen: We need to know the array+cookie size for the call to the allocator and the array size alone for the call to the initializer.

That last one is the killer. Teaching CXXOperatorCallExpr to just skip its first argument for printing and mangling wouldn't be so bad, but I'm not so happy about computing the array size again for the initializer call. And if we extract it from the arguments list, we just have to subtract the cookie size off again.

I do want to point out a couple good things I /hadn't/ put in the first version that this would allow -- namely, killing an old FIXME about using CheckFunctionCall, which we can actually do now. (Though right now CheckFunctionCall skips C++ operators, even though there could easily be nonnull/format attributes attached for the placement args.) If we can solve the CodeGen problem, then we also get to re-use all the real argument emission code for that too. (Of course, we could do that anyway by adding a flag to EmitCallArgs.)

(Slightly) updated patch attached...
Jordy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: CXXNewExpr-allocation.patch
Type: application/octet-stream
Size: 32575 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120330/6ab446f4/attachment.obj>