union with std::unique_ptr issue

[Daniel P]

The code below fails with vc and gcc, and I don't understand why. Can someone explain?

vc fails with a read access violation after stepping past (*), and gcc fails with Error in `./test': free(): invalid pointer: 0x0000000001984b88

#include <iostream>
#include <vector>
#include <memory>
#include <new>
#include <cassert>

namespace {

enum class token_type
{
nil,
lbrace,
rbrace,
key,
expression
};

struct lbrace_arg_t
{
explicit lbrace_arg_t() = default;
};
constexpr lbrace_arg_t lbrace_arg{};

struct rbrace_arg_t
{
explicit rbrace_arg_t() = default;
};
constexpr rbrace_arg_t rbrace_arg{};

struct expression
{
};

class token
{
public:
token_type type_;

union
{
std::unique_ptr<expression> expr_;
std::string key_;
};
public:

token(lbrace_arg_t)
: type_(token_type::lbrace)
{
}

token(rbrace_arg_t)
: type_(token_type::rbrace)
{
}

token(const std::string& key)
: type_(token_type::key)
{
new (&key_) std::string(key);
}

token(const std::unique_ptr<expression>& expr) = delete;

token(std::unique_ptr<expression>&& expr)
: type_(token_type::expression)
{
new (&expr_) std::unique_ptr<expression>(std::move(expr));
}

token(token&& other)
{
construct(std::forward<token>(other));
}

token& operator=(const token& other) = delete;

token& operator=(token&& other)
{
if (&other != this)
{
if (type_ == other.type_)
{
switch (type_)
{
case token_type::expression:
expr_ = std::move(other.expr_);
break;
case token_type::key:
key_ = std::move(other.key_);
break;
default:
break;
}
}
else
{
destroy();
construct(std::forward<token>(other));
}
}
return *this;
}

~token() noexcept
{
destroy();
}

token_type type() const
{
return type_;
}

void construct(token&& other)
{
type_ = other.type_;
switch (type_)
{
case token_type::expression:
new (&expr_) std::unique_ptr<expression>(std::move(other.expr_));
break;
case token_type::key:
new (&key_) std::string(std::move(other.key_));
break;
default:
break;
}
}

void destroy() noexcept
{
switch (type_)
{
case token_type::expression:
expr_.~unique_ptr();
break;
case token_type::key:
key_.~basic_string();
break;
default:
break;
}
}
};

std::vector<token> output_stack;

void push_token(token&& tok)
{
switch (tok.type())
{
case token_type::lbrace:
case token_type::key:
case token_type::expression:
output_stack.push_back(std::move(tok));
break;
case token_type::rbrace:
{
auto it = output_stack.rbegin();
while (it != output_stack.rend() && it->type() != token_type::lbrace)
{
assert(it->type() == token_type::expression);
auto expr = std::move(output_stack.back().expr_);
++it;
assert(it->type() == token_type::key);
auto key = std::move(output_stack.back().key_);
++it;
}
assert(it != output_stack.rend());
output_stack.erase(it.base(), output_stack.end());
assert(it->type() == token_type::lbrace);
output_stack.pop_back();
break;
}
}
}
}

int main()
{
push_token(token(lbrace_arg));
push_token(token("foo"));
push_token(token(std::make_unique<expression>()));
push_token(token(rbrace_arg));
}

[Sam]

Daniel P writes:

> The code below fails with vc and gcc, and I don't understand why. Can
> someone explain?
>
> vc fails with a read access violation after stepping past (*), and gcc fails
> with Error in `./test': free(): invalid pointer: 0x0000000001984b88

I see two possible directions here.

1) Burn a crapton of time debugging this, to get to the bottom of it.

2) Upgrade your compiler to at least the C++17 level, and replace the entire
token class with:

typedef std::variant<std::monostate, lbrace_arg_t, rbrace_arg_t,
std::unique_ptr<expression>,
std::string> token;

For C++14 or earlier, Boost's version will also work. Either does the same
thing. I don't see much of a reason to reinvent this wheel.

[Alf P. Steinbach]

This seems to "work":

case token_type::rbrace:
{
auto it = output_stack.rbegin();

while (it->type() != token_type::lbrace)

{
assert(it->type() == token_type::expression);

auto expr = std::move(it->expr_);

++it;
assert(it->type() == token_type::key);

auto key = std::move(it->key_);
++it;
}
assert(it->type() == token_type::lbrace);
output_stack.erase(it.base(), output_stack.end());
output_stack.pop_back();
break;
}


It seems connected to moving twice or more from `output_stack.back()`.

If this solves your problem then it's proof that I can fix code without
understanding it.


- Alf

PS: The two statements before `break;` can probably be expressed with
just one vector operation, more clean IMHO.

PPS: Consider using `std::variant` instead of creating your own
discriminated union.

[Daniel P]

On Sunday, June 21, 2020 at 11:35:04 PM UTC-4, Sam wrote:
>
> I see two possible directions here.
>
> 1) Burn a crapton of time debugging this, to get to the bottom of it.
>
> 2) Upgrade your compiler to at least the C++17 level, and replace the entire
> token class with:
>
> typedef std::variant<std::monostate, lbrace_arg_t, rbrace_arg_t,
> std::unique_ptr<expression>,
> std::string> token;
>
> For C++14 or earlier, Boost's version will also work. Either does the same
> thing. I don't see much of a reason to reinvent this wheel.

variant isn't an option, C++11 needs to be supported. Besides, I need to
understand what's happening here.

Daniel

[Daniel P]

On Monday, June 22, 2020 at 12:15:39 AM UTC-4, Alf P. Steinbach wrote:
> This seems to "work":
>
> case token_type::rbrace:
> {
> auto it = output_stack.rbegin();
> while (it->type() != token_type::lbrace)
> {
> assert(it->type() == token_type::expression);
> auto expr = std::move(it->expr_);
> ++it;
> assert(it->type() == token_type::key);
> auto key = std::move(it->key_);
> ++it;
> }
> assert(it->type() == token_type::lbrace);
> output_stack.erase(it.base(), output_stack.end());
> output_stack.pop_back();
> break;
> }
>
>
> It seems connected to moving twice or more from `output_stack.back()`.
>
> If this solves your problem then it's proof that I can fix code without
> understanding it.
>

Alf, A _BIG_ thanks. That was a bug, and I couldn't see it. Very much
appreciated.

Daniel