improve trivial_abi documentation to better describe the cases where it should not be used

[Quuxplusone]

Bugzilla Link	37319
Version	trunk
OS	All
CC	@dwblaikie,@efriedma-quic,@zygoloid

Extended Description

#include <assert.h>
#include <stdint.h>

#define TRIVIAL_ABI __attribute__((trivial_abi))

template<class T>
class TRIVIAL_ABI trivial_offset_ptr {
    intptr_t value_;
public:
    trivial_offset_ptr(T *p) : value_((const char*)p - (const char*)this) {}
    trivial_offset_ptr(const trivial_offset_ptr& rhs) : value_((const char*)rhs.get() - (const char*)this) {}
    T *get() const { return (T *)((const char *)this + value_); }
    trivial_offset_ptr& operator=(const trivial_offset_ptr& rhs) {
        value_ = ((const char*)rhs.get() - (const char*)this);
        return *this;
    }
    trivial_offset_ptr& operator+=(int diff) {
        value_ += (diff * sizeof (T));
        return *this;
    }
};

trivial_offset_ptr<int>
incr(trivial_offset_ptr<int> p) {
    p += 1;
    return p;
}

int main() {
    int a[10];
    trivial_offset_ptr<int> op = &a[4];
    trivial_offset_ptr<int> top = &a[4];
    top = incr(top);
    assert(top.get() == &a[5]);
}

clang++ -std=c++17 -O1 test.cc -o a.out
./a.out  # works great!

clang++ -std=c++17 -O2 test.cc -o a.out
./a.out

Assertion failed: (top.get() == &a[5]), function main, file test.cc, line 34.
Abort trap: 6

====

This is super sneaky UB-ful code in theory, I'm sure, but IMHO Clang is still doing something not-conservative-enough here. Especially, notice that if you remove the unused local variable op from main, the assertion will pass. Put the unused variable back, and the assertion will fail.

Documentation for [[trivial_abi]] is here. It does not mention any caveats about "don't implement offset_ptr with this attribute." I would grudgingly accept that outcome if it were proposed (but I'd rather the code Just Work).
https://clang.llvm.org/docs/AttributeReference.html#trivial-abi-clang-trivial-abi

P.S. I am aware that [[trivial_abi]] is super new, and I love it, and I don't mean to be harsh at all in this first corner-case bug filed against it. :)

[Quuxplusone]

$ ~/llvm/build/bin/clang++ -std=c++17 test.cc -O2 -v
clang version 7.0.0 (git@github.com:llvm-mirror/clang.git 4e9568799bf3497f82dca7771dc870324f42bcc9) (https://git.llvm.org/git/llvm.git fbad418)
Target: x86_64-apple-darwin17.5.0
Thread model: posix
InstalledDir: /Users/aodwyer/llvm/build/bin
"/Users/aodwyer/llvm/build/bin/clang-7" -cc1 -triple x86_64-apple-macosx10.13.0 -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -emit-obj -disable-free -main-file-name test.cc -mrelocation-model pic -pic-level 2 -mthread-model posix -mdisable-fp-elim -masm-verbose -munwind-tables -target-cpu penryn -dwarf-column-info -debugger-tuning=lldb -target-linker-version 305 -v -resource-dir /Users/aodwyer/llvm/build/lib/clang/7.0.0 -stdlib=libc++ -O2 -std=c++17 -fdeprecated-macro -fdebug-compilation-dir /tmp -ferror-limit 19 -fmessage-length 153 -stack-protector 1 -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fobjc-runtime=macosx-10.13.0 -fcxx-exceptions -fexceptions -fmax-type-align=16 -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops -vectorize-slp -o /var/folders/0l/9t0yv2890_g4wgmy53n_mg7w0000gy/T/test-9d1fd1.o -x c++ test.cc
clang -cc1 version 7.0.0 based upon LLVM 7.0.0svn default target x86_64-apple-darwin17.5.0
ignoring nonexistent directory "/usr/include/c++/v1"
#include "..." search starts here:
#include <...> search starts here:
/Users/aodwyer/llvm/build/include/c++/v1
/usr/local/include
/Users/aodwyer/llvm/build/lib/clang/7.0.0/include
/usr/include
/System/Library/Frameworks (framework directory)
/Library/Frameworks (framework directory)
End of search list.
"/usr/bin/ld" -demangle -lto_library /Users/aodwyer/llvm/build/lib/libLTO.dylib -dynamic -arch x86_64 -macosx_version_min 10.13.0 -o a.out /var/folders/0l/9t0yv2890_g4wgmy53n_mg7w0000gy/T/test-9d1fd1.o -lc++ -lSystem

[efriedma-quic]

The definition of trivial_abi is that when you pass the class to a function, it gets memcpy'ed. So your code to adjust the offset doesn't get called consistently. This would be obvious at -O0 if you tried to call get() inside the implementation of incr().

Your testcase "works" at lower optimization levels due to pure luck, I think: the offsets of the two skipped copy constructor calls cancel each other out, or something like that.

[zygoloid]

We should update the documentation to be a bit clearer about the implications of the fact that we will "pass and return the type using the C ABI for the underlying type" (in particular, how it will break types that contain pointers to themselves). But this is not a miscompile.

[Quuxplusone]

Okay, I can buy that.
To clarify: If I make a copy, it should still call the copy constructor exactly once; if I move, it should still call the move constructor exactly once; but it might also memcpy the bits to a new address either before or after the copy/move. If my code relies on its not doing that extra memcpy, then I shouldn't use [[trivial_abi]].

I wish there were a way to detect this situation programmatically, but I can totally believe that that's impossible.

Improving the docs to include something like the above clarification sounds to me like an appropriate resolution.

[zygoloid]

To clarify: If I make a copy, it should still call the copy constructor
exactly once; if I move, it should still call the move constructor exactly
once;

Copies and moves are still subject to elision, but other than that, yes. (And in particular, you still get one constructor call for each destructor call.)

but it might also memcpy the bits to a new address either before or
after the copy/move. If my code relies on its not doing that extra memcpy,
then I shouldn't use [[trivial_abi]].

Yes. You can think of the behavior being as if the parameter object is first constructed, and then relocated from the caller's preferred location into the callee's preferred location. And then the destructor runs in the callee (because it must, as the caller has no access to the object any more), potentially in the wrong order relative to other function parameters.

[AaronBallman]

CC @ahatanak as the original attribute author