Understanding why using stack-allocated variable in BitFilter::Eval is correct

[niooss-ledger]

Hello,

While trying to understand the implementation of method BitFilter::Eval, I am wondering whether there could be a bug in it. The current code is:

unisim_archisec/unisim/util/symbolic/binsec/binsec.cc

Lines 399 to 423 in 17fcf55

	ConstNodeBase const*
	BitFilter::Eval( ConstNodeBase const** cnbs ) const
	{
	Expr scratch = cnbs[0];
	// if (rshift)
	// scratch = make_operation(Op::Lsr, scratch, make_const<shift_type>(rshift));
	int shift = extend - (rshift+select);
	Zero dz(GetType()); dz.Retain(); // Prevent deletion of this stack-allocated object
	if (shift >= 0)
	{
	scratch = make_operation( Op::ReinterpretAs, &dz, scratch );
	if (shift)
	scratch = make_operation( Op::Lsl, scratch, make_const<shift_type>(shift) );
	}
	else
	{
	scratch = make_operation( Op::Lsr, scratch, make_const<shift_type>(-shift) );
	scratch = make_operation( Op::ReinterpretAs, &dz, scratch );
	}
	Expr xshift = make_const<shift_type>(extend - select);
	ConstNodeBase const* args[2] = {scratch.ConstSimplify(), xshift.ConstSimplify()};
	return args[0]->apply(sxtend ? Op::Asr : Op::Lsr, args);
	}

On line 407, a local variable Zero dz gets allocated on the stack. There is a call to dz.Retain(); to prevent its destruction with delete in

unisim_archisec/unisim/util/symbolic/symbolic.hh

Line 123 in 17fcf55

void Release() const { if (--refs == 0) delete this; }

This variable is nonetheless passed as reference in a call to make_operation, so it is embedded in variables scratch then args, and finally in the return value. But dz location becomes invalid once the function returns, so there seems to be a possible use-after-destruction issue.

What do you think? I am feeling I am misunderstanding something in this C++ code and would be glad to know what I missed.

Context

By the way, I started looking at this function because g++ 12.2.0 on Debian 12 (Bookworm) reports a -Wfree-nonheap-object warning, which seems to be a false positive (because dz.Retain(); actually prevents Release from calling delete this):

In file included from ../../unisim/util/symbolic/vector/vector.hh:38,
                 from ../../unisim/util/symbolic/binsec/binsec.hh:38,
                 from symbolic/binsec/binsec.cc:35:
In destructor 'virtual unisim::util::symbolic::Zero::~Zero()',
    inlined from 'void unisim::util::symbolic::ExprNode::Release() const' at ../../unisim/util/symbolic/symbolic.hh:123:52,
    inlined from 'void unisim::util::symbolic::ExprNode::Release() const' at ../../unisim/util/symbolic/symbolic.hh:123:10,
    inlined from 'unisim::util::symbolic::Expr::~Expr()' at ../../unisim/util/symbolic/symbolic.hh:373:38,
    inlined from 'virtual const unisim::util::symbolic::ConstNodeBase* unisim::util::symbolic::binsec::BitFilter::Eval(const unisim::util::symbolic::ConstNodeBase**) const' at symbolic/binsec/binsec.cc:410:33:
../../unisim/util/symbolic/symbolic.hh:144:10: warning: 'void operator delete(void*)' called on unallocated object 'dz' [-Wfree-nonheap-object]
  144 |   struct Zero : public ExprNode
      |          ^~~~
symbolic/binsec/binsec.cc: In member function 'virtual const unisim::util::symbolic::ConstNodeBase* unisim::util::symbolic::binsec::BitFilter::Eval(const unisim::util::symbolic::ConstNodeBase**) const':
symbolic/binsec/binsec.cc:407:10: note: declared here
  407 |     Zero dz(GetType()); dz.Retain(); // Prevent deletion of this stack-allocated object
      |          ^~

This makes me believe this is a hack, and that dz should actually be dynamically allocated (using for example Zero *dz = new Zero(GetType());)... and while thinking more about this, I am wondering why struct ExprNode is implementing ref-counting from scratch instead of using battle-tested standard types such as std::shared_ptr.

Would you be interested in some code-refactoring Pull Requests which replaces raw-pointer management logic with smart pointers?