Opened 12 years ago

Closed 9 years ago

#5146 closed Bugs (invalid)

Assertion fails in visitation_impl.hpp with nested variants

Reported by: Florian Goujeon <flo.goujeon@…> Owned by: Antony Polukhin
Milestone: To Be Determined Component: variant
Version: Boost 1.45.0 Severity: Problem
Keywords: variant assert assertion nested Cc:

Description

Let test_variant be following typedef:

typedef
	variant
	<
		int,
		ptr_to_test_variant
	>
	test_variant
;

ptr_to_test_variant is a simple class which contains a test_variant object allocated on the heap (to break the circular dependency, see attachment).

Here is the test case:

int main()
{
	test_variant tv1 = 88;
	test_variant ptr_to_tv1 = ptr_to_test_variant(tv1);
	test_variant ptr_to_ptr_to_tv1 = ptr_to_test_variant(ptr_to_tv1);

	const ptr_to_test_variant& direct_ptr_to_ptr_to_tv1 = get<ptr_to_test_variant>(ptr_to_ptr_to_tv1);
	ptr_to_ptr_to_tv1 = direct_ptr_to_ptr_to_tv1.pointed_test_variant();
}

Here is the output:

/usr/include/boost/variant/detail/visitation_impl.hpp:207: typename Visitor::result_type boost::detail::variant::visitation_impl(int, int, Visitor&, VPCV, mpl_::true_, NBF, W*, S*) [with W = mpl_::int_<20>, S = boost::detail::variant::visitation_impl_step<boost::mpl::l_iter<boost::mpl::l_end>, boost::mpl::l_iter<boost::mpl::l_end> >, Visitor = boost::variant<int, ptr_to_test_variant>::convert_copy_into, VPCV = void*, NBF = boost::variant<int, ptr_to_test_variant>::has_fallback_type_, typename Visitor::result_type = int, mpl_::true_ = mpl_::bool_<true>]: Assertion `false' failed.

I'm experiencing the exact same issue with my own implementation of stack-based variant (using variadic templates) as well. I'd like to know how to fix this…

Attachments (1)

variant_bug.cpp (2.0 KB ) - added by Florian Goujeon <flo.goujeon@…> 12 years ago.

Download all attachments as: .zip

Change History (6)

by Florian Goujeon <flo.goujeon@…>, 12 years ago

Attachment: variant_bug.cpp added

comment:1 by Florian Goujeon <flo.goujeon@…>, 12 years ago

OK, I got it.

Since ptr_to_ptr_to_tv1 kinda contains direct_ptr_to_ptr_to_tv1, variant has to destruct direct_ptr_to_ptr_to_tv1 AFTER direct_ptr_to_ptr_to_tv1.pointed_test_variant() was copied into ptr_to_ptr_to_tv1.

Currently, I guess variant clears its content (i.e. calls the destructor of its contained object) then sets it with a copy of the new object. This is the most natural behavior, but it causes problems when the copy constructor of the new object needs data that are stored in the previously contained object.

I don't know the code of Boost.Variant, so I won't be able to write a patch. But basically, the changes I made in my implementation can be summarized like this:

BEFORE:

template<typename Clear, typename Set>
void
clear_and_set(const Set& value)
{
	(*reinterpret_cast<Clear*>(buffer_)).~Clear(); //clear
	new(buffer_) Set(value); //set
}

AFTER:

template<typename Clear, typename Set>
void
clear_and_set(const Set& value)
{
	char old_buffer[Size];
	for(unsigned int i = 0; i < Size; ++i)
	{
		old_buffer[i] = buffer_[i];
	}

	new(buffer_) Set(value); //set

	(*reinterpret_cast<Clear*>(old_buffer)).~Clear(); //clear
}

comment:2 by Steven Watanabe, 12 years ago

Please see The "Ideal" Solution: False Hopes for an explanation of why this solution is evil.

comment:3 by Antony Polukhin, 9 years ago

This looks more like a ptr_to_test_variant issue, not variant's one.

Take a look at this code:

  const ptr_to_test_variant& direct_ptr_to_ptr_to_tv1 = get<ptr_to_test_variant>(ptr_to_ptr_to_tv1);
  ptr_to_ptr_to_tv1 = direct_ptr_to_ptr_to_tv1.pointed_test_variant();

You've got the chain: ptr_to_ptr_to_tv1 -> direct_ptr_to_ptr_to_tv1 -> unnamed

Then you try to do the following: ptr_to_ptr_to_tv1 = unnamed;

Before assigning anything to ptr_to_ptr_to_tv1 it will delete the data it owns:

delete direct_ptr_to_ptr_to_tv1;
delete unnamed;

And only after that will attempt to do the assignment, but delete unnamed is already called.

Much better solution would be to modify code of ptr_to_test_variant, (you do know that loops are possible, so make small trick to workaround such cases):

ptr_to_test_variant&
ptr_to_test_variant::operator=(const ptr_to_test_variant& rhs)
{
  test_variant* tmp_ptr = pointed_test_variant_;
  pointed_test_variant_ = new test_variant(*rhs.pointed_test_variant_);
  // Even if there was a loop, data is copied and new pointed_test_variant_
  // does not depends on old data at all. We are free to delete the old data.
  delete tmp_ptr;
  return *this;
}

comment:4 by Antony Polukhin, 9 years ago

Owner: changed from ebf to Antony Polukhin
Status: newassigned

comment:5 by Antony Polukhin, 9 years ago

Resolution: invalid
Status: assignedclosed
Note: See TracTickets for help on using tickets.