Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

dangerous pessimization at throw_io_failure #73

Closed
ArtemGr opened this Issue Sep 24, 2012 · 5 comments

Comments

Projects
None yet
3 participants
Contributor

ArtemGr commented Sep 24, 2012

I've just seen this

 Invalid read of size 8
==15807==    at 0x8AFAB28: uw_update_context (unwind-dw2.c:216)
==15807==    by 0x8AFBF7B: _Unwind_RaiseException (unwind.inc:122)
==15807==    by 0x85C6A30: __cxa_throw (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17)
==15807==    by 0x7FF55B2: cppa::detail::fd_util::throw_io_failure(std::string&&, bool) (fd_util.cpp:57)
==15807==    by 0x7FF59DA: cppa::detail::fd_util::handle_write_result(long, bool) (fd_util.cpp:105)
==15807==    by 0x7FF5A4B: cppa::detail::fd_util::handle_read_result(long, bool) (fd_util.cpp:110)
==15807==    by 0x801CE45: cppa::detail::ipv4_io_stream::read(void*, unsigned long) (ipv4_io_stream.cpp:69)
==15807==    by 0x8066A77: cppa::remote_actor(std::pair<cppa::intrusive_ptr<cppa::util::input_stream>, cppa::intrusive_ptr<cppa::util::output_stream> >) (unicast_network.cpp:77)

And looking at the code, I think that
https://github.com/Neverlord/libcppa/blob/master/src/fd_util.cpp#L51
might benefit from less std::move and &&,
see http://stackoverflow.com/questions/4986673/c11-rvalues-and-move-semantics-confusion

What do you think?

Owner

Neverlord commented Sep 28, 2012

Well, the signature throw_io_failure(std::string, bool) would do as well. Though, I cannot see a bug in the code. I'll have a closer look at this next week.

Contributor

ArtemGr commented Sep 28, 2012

Well, the "Invalid read" isn't repeating, so this is some corner case, but '&&' shouldn't be used in the normal code if I read c++ FAQs correctly.

Member

mavam commented Sep 28, 2012

The && just binds to an rvalue reference argument. Because the argument is named, it is an lvalue inside the function body. Removing it only broadens the interface, i.e., it will be up to the caller to decide whether to incur a move or copy. (IIRC, it would be a glvalue then, whereas it is an rvalue right now.) Without having run the code, it does not look like the culprit to me.

The only issue I see with the above code snippet is that std::ios_base::failure does not have an rvalue constructor, so you could change

throw std::ios_base::failure(std::move(tmp));

to

throw std::ios_base::failure(tmp);
Contributor

ArtemGr commented Nov 10, 2012

Indeed, throw std::ios_base::failure(std::move(tmp)); seems to be the line the stack trace is pointing to.
Maybe this cast of tmp to rvalue is messing things for some compilers?

@Neverlord Neverlord closed this in ae663f8 Nov 12, 2012

Owner

Neverlord commented Nov 12, 2012

Even after reviewing the code again, I don't see a problem with the code. Looks like either a compiler bug or false alarm of valgrind to me. Nevertheless, I have revised the implementation and changed it to make use of std::ostringstream instead of concatenate the string by hand. This makes the implementation both shorter and should fix the strange behavior you're observing.

@Neverlord Neverlord added improvement and removed other labels Jul 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment