> On Nov 25, 2016, at 4:17 PM, Daniel Berlin via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > > > On Fri, Nov 25, 2016 at 6:10 AM, Hubert Tong <hubert.reinterpretcast at gmail.com <mailto:hubert.reinterpretcast at gmail.com>> wrote: > On Fri, Nov 25, 2016 at 1:42 AM, Daniel Berlin <dberlin at dberlin.org <mailto:dberlin at dberlin.org>> wrote: > What is the purpose of the union there? > The purpose of the union is to increase portability by ensuring that the placement new is not being performed on insufficiently sized or aligned memory. > Gotcha > > > I ask because pretty much no compiler will respecting the unioning without visible accesses in all cases, because it would ruin most optimization[1] > > But i'm also not sure it's required in this testcase to make your testcase fail. > It isn't. The program should be valid, without the union, on platforms where int and float have the same size an alignment. > > Right, so you need to debug that first, and see what's going wrong. > Without TBAA info in the .ll file, this should just work. > > > > In practice, handling placement new properly in gcc required the equivalent of a new intrinsic (in gcc, it required adding CHANGE_DYNAMIC_TYPE_EXPR). > Sure; my question is whether or not there is already a solution in the works for LLVM. If not, then I'll try to work with some folks to propose an intrinsic. > > I would first focus on understanding why the testcase fails without any TBAA info at all. > In that case, i would expect it to work.The PR says "passes with -fno-strict-aliasing”, my understanding is that it is failing only with the TBAA indeed. You don’t need the main and the union to reproduce, extracting foo() alone in its single own file is enough: void *operator new(decltype(sizeof 0), void *) noexcept; float *qq; void foo(int *p, int *q, long unk) { for (long i = 0; i < unk; ++i) { ++*p; qq = new (static_cast<void *>(&q[i])) float(42); } } LICM will get the store to p out of the loop, conceptually turning it into: void foo(int *p, int *q, long unk) { for (long i = 0; i < unk; ++i) { qq = new (static_cast<void *>(&q[i])) float(42); } ++*p; } Now I don’t know if the use of placement new in this example is legal in the first place. I thought calling delete before using placement new was mandatory. CC Sanjoy, since he looked into TBAA recently and it reminds me a similar test case he mentioned, not with placement new but with a call to a function taking int * and float *, and passing the same address (call site was union). — Mehdi -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161125/2592a7e6/attachment.html>
Yes, I misread, because I didn't think -O1 turned on strict aliasing and missed that part On Fri, Nov 25, 2016, 8:23 PM Mehdi Amini <mehdi.amini at apple.com> wrote:> > On Nov 25, 2016, at 4:17 PM, Daniel Berlin via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > > > On Fri, Nov 25, 2016 at 6:10 AM, Hubert Tong < > hubert.reinterpretcast at gmail.com> wrote: > > On Fri, Nov 25, 2016 at 1:42 AM, Daniel Berlin <dberlin at dberlin.org> > wrote: > > What is the purpose of the union there? > > The purpose of the union is to increase portability by ensuring that the > placement new is not being performed on insufficiently sized or aligned > memory. > > Gotcha > > > > > I ask because pretty much no compiler will respecting the unioning without > visible accesses in all cases, because it would ruin most optimization[1] > > But i'm also not sure it's required in this testcase to make your testcase > fail. > > It isn't. The program should be valid, without the union, on platforms > where int and float have the same size an alignment. > > > Right, so you need to debug that first, and see what's going wrong. > Without TBAA info in the .ll file, this should just work. > > > > > > In practice, handling placement new properly in gcc required the > equivalent of a new intrinsic (in gcc, it required adding > CHANGE_DYNAMIC_TYPE_EXPR). > > Sure; my question is whether or not there is already a solution in the > works for LLVM. If not, then I'll try to work with some folks to propose an > intrinsic. > > > I would first focus on understanding why the testcase fails without any > TBAA info at all. > In that case, i would expect it to work. > > > > The PR says "passes with -fno-strict-aliasing”, my understanding is that > it is failing only with the TBAA indeed. > > You don’t need the main and the union to reproduce, extracting foo() alone > in its single own file is enough: > > void *operator new(decltype(sizeof 0), void *) noexcept; > float *qq; > void foo(int *p, int *q, long unk) { > for (long i = 0; i < unk; ++i) { > ++*p; > qq = new (static_cast<void *>(&q[i])) float(42); > } > } > > LICM will get the store to p out of the loop, conceptually turning it into: > > void foo(int *p, int *q, long unk) { > for (long i = 0; i < unk; ++i) { > qq = new (static_cast<void *>(&q[i])) float(42); > } > ++*p; > } > > > Now I don’t know if the use of placement new in this example is legal in > the first place. I thought calling delete before using placement new was > mandatory. > > CC Sanjoy, since he looked into TBAA recently and it reminds me a similar > test case he mentioned, not with placement new but with a call to a > function taking int * and float *, and passing the same address (call site > was union). > > > — > Mehdi > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161126/6b0aae53/attachment.html>
Hi, On Sat, Nov 26, 2016 at 9:53 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:> The PR says "passes with -fno-strict-aliasing”, my understanding is that it > is failing only with the TBAA indeed. > > You don’t need the main and the union to reproduce, extracting foo() alone > in its single own file is enough: > > void *operator new(decltype(sizeof 0), void *) noexcept; > float *qq; > void foo(int *p, int *q, long unk) { > for (long i = 0; i < unk; ++i) { > ++*p; > qq = new (static_cast<void *>(&q[i])) float(42); > } > } > > LICM will get the store to p out of the loop, conceptually turning it into: > > void foo(int *p, int *q, long unk) { > for (long i = 0; i < unk; ++i) { > qq = new (static_cast<void *>(&q[i])) float(42); > } > ++*p; > } > > > Now I don’t know if the use of placement new in this example is legal in the > first place. I thought calling delete before using placement new was > mandatory.So if you: 1. override operator delete to do nothing for that type (so that the placement new actually has unfreed backing storage to re-use). 2. have an empty destructor. and have the source program be void *operator new(decltype(sizeof 0), void *) noexcept; float *qq; void foo(int *p, int *q, long unk) { // unk = 1 for (long i = 0; i < unk; ++i) { ++*p; delete &q[i]; // since i is only ever 0, this does not // delete a derived pointer qq = new (static_cast<void *>(&q[i])) float(42); } } then I suspect we'll have the same problem after the destructor and the operator delete for that type has been inlined away.> CC Sanjoy, since he looked into TBAA recently and it reminds me a similar > test case he mentioned, not with placement new but with a call to a function > taking int * and float *, and passing the same address (call site was > union).The case I was talking about was something like: // C11 source file. union S { int i; float f; }; void f(int* i, float *f) { *i = 20; *f = 40.0; } void g() { union S s; f(&s.i, &s.f); } At least cursorily this looked well defined in C++ to me -- f should first write int(20) and then float(40.0) to the same location legally. However clang emits tbaa such that LLVM concludes that in f, the store to i and the store to f don't alias. However, I'm not very confident in the "this looked well defined" reading. I did not consult the standard, but instead relied on second hand stackoverflow answers, so I won't be surprised to hear that I was wrong about this. Actually I'm never surprised to hear that I'm wrong, but I'll be especially less surprised in this case. :) Thanks! -- Sanjoy
The position most compilers take is the the access must be visibly through a union if you want it to work. Otherwise, it's not possible to use tbaa really at all. Move your function f to another file, for example, and you simply could never tell. Some will give you a little more, but you are pushing your luck This was discussed in depth in the thread I quoted, and has GCC mailing list threads going back to before llvm existed :). Note that there are rules about touching members in a union other than the first one you set, but pretty much everyone guarantees type punning through unions to work. On Fri, Nov 25, 2016, 9:07 PM Sanjoy Das <sanjoy at playingwithpointers.com> wrote:> Hi, > > On Sat, Nov 26, 2016 at 9:53 AM, Mehdi Amini <mehdi.amini at apple.com> > wrote: > > The PR says "passes with -fno-strict-aliasing”, my understanding is that > it > > is failing only with the TBAA indeed. > > > > You don’t need the main and the union to reproduce, extracting foo() > alone > > in its single own file is enough: > > > > void *operator new(decltype(sizeof 0), void *) noexcept; > > float *qq; > > void foo(int *p, int *q, long unk) { > > for (long i = 0; i < unk; ++i) { > > ++*p; > > qq = new (static_cast<void *>(&q[i])) float(42); > > } > > } > > > > LICM will get the store to p out of the loop, conceptually turning it > into: > > > > void foo(int *p, int *q, long unk) { > > for (long i = 0; i < unk; ++i) { > > qq = new (static_cast<void *>(&q[i])) float(42); > > } > > ++*p; > > } > > > > > > Now I don’t know if the use of placement new in this example is legal in > the > > first place. I thought calling delete before using placement new was > > mandatory. > > So if you: > > 1. override operator delete to do nothing for that type (so that the > placement new actually has unfreed backing storage to re-use). > 2. have an empty destructor. > > and have the source program be > > void *operator new(decltype(sizeof 0), void *) noexcept; > float *qq; > void foo(int *p, int *q, long unk) { > // unk = 1 > for (long i = 0; i < unk; ++i) { > ++*p; > delete &q[i]; // since i is only ever 0, this does not > // delete a derived pointer > qq = new (static_cast<void *>(&q[i])) float(42); > } > } > > then I suspect we'll have the same problem after the destructor and > the operator delete for that type has been inlined away. > > > CC Sanjoy, since he looked into TBAA recently and it reminds me a similar > > test case he mentioned, not with placement new but with a call to a > function > > taking int * and float *, and passing the same address (call site was > > union). > > The case I was talking about was something like: > > // C11 source file. > > union S { > int i; > float f; > }; > > void f(int* i, float *f) { > *i = 20; > *f = 40.0; > } > > void g() { > union S s; > f(&s.i, &s.f); > } > > > At least cursorily this looked well defined in C++ to me -- f should > first write int(20) and then float(40.0) to the same location legally. > However clang emits tbaa such that LLVM concludes that in f, the store > to i and the store to f don't alias. > > However, I'm not very confident in the "this looked well defined" > reading. I did not consult the standard, but instead relied on second > hand stackoverflow answers, so I won't be surprised to hear that I was > wrong about this. Actually I'm never surprised to hear that I'm > wrong, but I'll be especially less surprised in this case. :) > > Thanks! > -- Sanjoy >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161126/0d9ae5ee/attachment-0001.html>
On Fri, Nov 25, 2016 at 11:23 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:> You don’t need the main and the union to reproduce, extracting foo() alone > in its single own file is enough: > > void *operator new(decltype(sizeof 0), void *) noexcept; > float *qq; > void foo(int *p, int *q, long unk) { > for (long i = 0; i < unk; ++i) { > ++*p; > qq = new (static_cast<void *>(&q[i])) float(42); > } > } > > LICM will get the store to p out of the loop, conceptually turning it into: > > void foo(int *p, int *q, long unk) { > for (long i = 0; i < unk; ++i) { > qq = new (static_cast<void *>(&q[i])) float(42); > } > ++*p; > } > > > Now I don’t know if the use of placement new in this example is legal in > the first place. I thought calling delete before using placement new was > mandatory. >There's wording describing the reuse of storage and the situation of neither calling the destructor nor using a *delete-expression*. N4604 subclause 3.8 [basic.life] paragraph 5: For an object of a class type with a non-trivial destructor, the program is not required to call the destructor explicitly before the storage which the object occupies is reused or released; however, if there is no explicit call to the destructor or if a *delete-expression* is not used to release the storage, the destructor shall not be implicitly called and any program that depends on the side effects produced by the destructor has undefined behavior. I read the above wording as saying that, even if the storage is currently occupied by an object of class type with a non-trivial destructor, the storage can be "simply" reused (without undefined behaviour). The lifetime of the previous object simply ends when the placement new occurs (more wording below). 3.8 [basic.life] paragraph 1: The lifetime of an object *o* of type T ends when [ ... ] the storage which the object occupies is released, or is reused by an object that is not nested within *o*. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161126/3a2e2e19/attachment.html>
On Sat, Nov 26, 2016 at 12:07 AM, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:> So if you: > > 1. override operator delete to do nothing for that type (so that the > placement new actually has unfreed backing storage to re-use). > 2. have an empty destructor. >So, void *operator new(decltype(sizeof 0), void *) noexcept; struct MyInt { static void operator delete(void *) { } MyInt() { x = 0; } ~MyInt() { } int x; }; float *qq; void foo(void *p, void *q, long unk) { // unk = 1 for (long i = 0; i < unk; ++i) { auto pp = new (p) MyInt; delete pp; // or pp->~MyInt(); qq = new (static_cast<void *>(&reinterpret_cast<char *>(q)[i])) float(42); } } then I suspect we'll have the same problem after the destructor and> the operator delete for that type has been inlined away. >Yes; the same problem occurs. So, it sounds like a new intrinsic is in order? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161201/8acb8a70/attachment.html>