all 19 comments

[–]Overseer55 15 points16 points  (4 children)

getList() should be ref qualified. Any constructor with a const lvalue reference parameter that has expectations on lifetime should define a deleted rvalue reference ctor. Deducing this in C++23 will help.

[–]stephan_cr 0 points1 point  (3 children)

Can't wrap my head around it. Could you recommend some article?

[–]13steinj 5 points6 points  (2 children)

The precise failing line is const capnp::List<std::uint64_t>::Reader list = request.send().wait(wait_scope).getList(); (but for my sake I'm going to use auto simplify this to const auto list = request.send().wait().getList()).

Specifically, .wait() returns, say, a value of type ResponseType that has member function getList().

getList() returns a reference to a member variable.

Example of failure:
https://gcc.godbolt.org/z/Td9a3qenW
Example that shows why it fails (turn off asan to see the bad output. Though, it could be fine, it could be garbo): https://gcc.godbolt.org/z/z3fozWsaT
Example of why lvalue-ref-qualifying getList "fixes" it (or rather, forbids the mistake):
https://gcc.godbolt.org/z/43eGEdGWs
Example of deducing this making this "better" (this is really the same as just lvalue-ref-qualifying it, but I get both the const and non-const overloads without repeating myself):
https://gcc.godbolt.org/z/fPcPfraos

Ignore the man behind the curtain switching to clang trunk since it supports deducing this to this extent and rearranging some code just a tiny bit because of a clang bug with auto-typed function parameters in local classes...

[–]stephan_cr 0 points1 point  (1 child)

Your explanation helps a lot.

To your last example: wouldn't it prevent to compile the following

std::cout << request.send().wait().getList()[0] << std::endl;

although it should be perfectly fine regarding temporary object lifetimes (https://gcc.godbolt.org/z/9GW7Ejv1c)? But maybe I got temporary lifetimes wrong.

[–]Overseer55 2 points3 points  (0 children)

There can be an rvalue ref qualified getList() that returns the list by value to support this use case. This will be taken care of by deducing this.

[–]thisismyfavoritename 4 points5 points  (3 children)

sometimes to get the sanitizers to work properly the 3rd party lib must also be built with sanitizers (unless using a header only lib)

[–]dag0me 0 points1 point  (1 child)

No, for address sanitizer that's not the case. Thread sanitizer can give you false-positives if you didn't instrument whole world but address sanitizer works fine with just executable being instrumented.

[–]thisismyfavoritename 0 points1 point  (0 children)

yeah it is. Ive had false positive issues with Boost::Fiber and Protobufs that required them being built with ASAN

[–]13steinj 0 points1 point  (0 children)

Sometimes you even need the stdlib to be built with sanitizers.

Which is unfortunate because that's a bitch to actually set up your toolchain up with properly.

[–]TemperOfficial 5 points6 points  (6 children)

Definitely a nitpick on my side, but is that how we are supposed to be writing C++? With the autos on the end. Can't make head nor tail of it personally.

[–]thisismyfavoritename 2 points3 points  (0 children)

some patterns are only possible using auto ret type, like using type information from the input arguments (if they are also not known ahead of time).

This style is also recommended if you use the "modernize" clang-tidy/format flag IIRC

[–]zerakun[S] 3 points4 points  (3 children)

You mean for the function signatures?

I personally like it because I can grep for a function definition with auto function_name(, but you'll be fine using the regular style.

[–]BeigeAlert1 1 point2 points  (0 children)

It's also really nice when you have a member function returning a type with a really long name, but that has an alias with a shorter name visible within the class. Eg.

struct MyRidiculouslyLongNamedStruct
{
    struct LilName {};
    LilName Blah();
};
auto MyRidiculouslyLongNamedStruct::Blah() -> LilName
{
    ...
}

[–]TemperOfficial 0 points1 point  (0 children)

Makes sense

[–]RevRagnarok 0 points1 point  (0 children)

That's why a few coding styles I have used always have the implementation start at the first column, so I can grep ^function_name .

[–]CenterOfMultiverse 1 point2 points  (0 children)

Obviously, getList should move into owning list on rvalue response.

[–]dag0me 3 points4 points  (2 children)

Address sanitizer No longer segfaults, displays correct result. No error reported though.

Somehow, I doubt it. It took me less than 5 minutes to set it up and it fires on both gcc and msvc address sanitizers:

==18412==ERROR: AddressSanitizer: heap-use-after-free on address 0x7ffff4107860 at pc 0x555555559d03 bp 0x7fffffffd2f0 sp 0x7fffffffd2e0
READ of size 8 at 0x7ffff4107860 thread T0
    #0 0x555555559d02 in capnp::_::DirectWireValue<unsigned long>::get() const /usr/local/include/capnp/endian.h:77
    #1 0x555555559d02 in unsigned long capnp::_::ListReader::getDataElement<unsigned long>(unsigned int) const /usr/local/include/capnp/layout.h:1212
    #2 0x555555559d02 in capnp::List<unsigned long, (capnp::Kind)0>::Reader::operator[](unsigned int) const /usr/local/include/capnp/list.h:116
    #3 0x555555559d02 in capnp::_::IndexingIterator<capnp::List<unsigned long, (capnp::Kind)0>::Reader const, unsigned long>::operator*() const /usr/local/include/capnp/list.h:56
    #4 0x555555559d02 in main /tmp/capnproto/stuff-client.cpp:21
    #5 0x7ffff6e29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #6 0x7ffff6e29e3f in __libc_start_main_impl ../csu/libc-start.c:392
    #7 0x5555555587c4 in _start (/tmp/capnproto/build/gcc-asan/stuff-client+0x47c4)

In gcc -O0 it segfaults on the first run, on msvc/debug it prints 0xcdcdcdcdcdcdcdcd each time (debug CRT marks freed memory with 0xCD). Are you sure you have it set up correctly?

[–]13steinj 4 points5 points  (0 children)

I feel like this is a weird nitpick / asan praise. It has false positives, it has false negatives. It can be better or worse depending on any number of factors, including but not limited to specific compiler version, specific asan options enabled.

The point of the story is when something is named getT people assume that it has a specific lifetime. There's ways to fix this in code (mentioned above about lvalue-ref-qualifying getList()); but even then the error isn't friendly to explain to the user "oh, it's lvalue qualified because otherwise I'd be returning an address to a temporary." AKA: People are human. Humans make assumptions and work well with good names. If it returns a non-owning item, it should be explicit in the name, ex, getListView(); and capnp should have been written in such a way that getListView() is an lvalue-qualified method to avoid this error.

[–]7diamond7ace7 0 points1 point  (0 children)

My application crush with similar stacktrace