[cfe-dev] Ownership of Stmts and Exprs, and destroying them

[Sebastian Redl]

Chris Lattner wrote:
> I don't know that anyone thinks it good that we leak memory :).  The 
> history here is that much of Sema was built before there *was* a clear 
> ownership model.  This means that there is a lot of "bad" code in it 
> that doesn't obey the newly defined ownership model.  Also, the parser 
> has a couple of places where it leaks as well.  If the parser does 
> error recovery and ends up obviating a parsed expr/statement/etc, it 
> should call the Actions::DeleteExpr/DeleteStmt methods to free them.  
> This happens in some cases, but not all of them.
OK. I thought there had to be such methods, but didn't go looking.
>
>> To destroy a Stmt or its derived classes, you don't use delete, but the
>> Destroy member. This may be overridden in subclasses, and the base
>> version first recursively calls Destroy for every child statement, and
>> then commits suicide (delete this). Calling delete yourself is *not*
>> sufficient.
>
> I think that statements (exprs are statements) are freed with delete 
> currently:
>
> void Sema::DeleteExpr(ExprTy *E) {
>   delete static_cast<Expr*>(E);
> }
> void Sema::DeleteStmt(StmtTy *S) {
>   delete static_cast<Stmt*>(S);
> }
>
> We're slowly migrating decls to use a Create/Destroy interface, in 
> order to let them be pool allocated or use some other allocation 
> mechanism.  It would be good to also move statements this way 
> eventually, but decls should be finished first.
Look at what happens if you just delete a CallExpr:

  ~CallExpr() {
    delete [] SubExprs;
  }

Doesn't delete the actual expressions, just the array holding them.
Expr has no explicit destructor.

  virtual ~Stmt() {}

So if you delete a CallExpr, its children are never freed and leak. The 
reason is found in Stmt::Destroy:

void Stmt::DestroyChildren(ASTContext& C) {
  for (child_iterator I = child_begin(), E = child_end(); I !=E; ++I)
    if (Stmt* Child = *I) Child->Destroy(C);
}

void Stmt::Destroy(ASTContext& C) {
  DestroyChildren(C);
  // FIXME: Eventually all Stmts should be allocated with the allocator
  //  in ASTContext, just like with Decls.
  // this->~Stmt();
  delete this;
}

In other words, I believe that Sema::DeleteExpr/Stmt, just like holding 
exprs with OwningPtrs, is insufficient. They need to Destroy, not delete.
> This would be great.  Probably the first place to start is to use 
> audit the parser for places where early exits cause parsed stmts to be 
> leaked.
OK.
> Once that is done, start going through sema.  The pattern we want to 
> use in Sema for statements is using OwningPtr to hold incoming 
> arguments.  For example, ActOnBinOp should look like:
>
>                                     ExprTy *LHS, ExprTy *RHS) {
>   BinaryOperator::Opcode Opc = ConvertTokenKindToBinaryOpcode(Kind);
>   OwningPtr<Expr> lhs = (Expr *)LHS, rhs = (Expr*)RHS;
As I said, I don't think this is sufficient. I've written a TempOwner 
that acts like OwningPtr that calls Destroy, though.
>
> For routines that have more incoming values or are variadic, the 
> pattern in ActOnCallExpr should be followed.
That pattern has the disadvantage that it sometimes requires either that 
the Expr is modifiable after creation, or weird restructuring of the 
method. Take, for example, ActOnCXXNew: if I want to pass everything to 
the constructor, I need to do three overload resolutions before creating 
the CXXNewExpr. The alternative is creating the Expr with fewer 
parameters and adding the rest later - but that means I have to add 
setters to CXXNewExpr, something I'm not a fan of.

Sebastian