OK.
On Tue, Oct 22, 2019 at 9:04 AM Jakub Jelinek <ja...@redhat.com> wrote: > > Hi! > > As noticed by Richi, > FAIL: g++.dg/guality/pr55665.C -O2 line 23 p == 40 > FAIL: g++.dg/guality/pr55665.C -O2 line 23 p == 40 > FAIL: g++.dg/guality/pr55665.C -O3 -g line 23 p == 40 > FAIL: g++.dg/guality/pr55665.C -O3 -g line 23 p == 40 > aren't a mere wrong-debug, but actually wrong-code issues if the function > (constructor) would ever be called concurrently. > > As discussed already in > https://sourceware.org/bugzilla/show_bug.cgi?id=13344 > while synchronization primitives usually don't call back into the current > TU, we don't want to mark the synchronization primitives as leaf, as > otherwise for local statics that aren't TREE_ADDRESSABLE ipa-reference > might optimize them. > We have: > _1 = __atomic_load_1 (&_ZGVZN1AC4EiE1p, 2); > if (_1 == 0) > goto <bb 4>; [33.00%] > else > goto <bb 3>; [67.00%] > <bb 3> [local count: 956811342]: > goto <bb 6>; [100.00%] > <bb 4> [local count: 354334802]: > _2 = __cxa_guard_acquire (&_ZGVZN1AC4EiE1p); > if (_2 != 0) > goto <bb 5>; [33.00%] > else > goto <bb 3>; [67.00%] > <bb 5> [local count: 116930485]: > _3 = bar (); > p = _3; > __cxa_guard_release (&_ZGVZN1AC4EiE1p); > <bb 6> [local count: 1073741824]: > p.2_4 = p; > foo (p.2_4); > _6 = p.2_4 + 1; > p = _6; > and ipa-reference when ECF_LEAF is on __cxa_* (and probably IPA discovered > on foo) thinks it is fine to optimize by loading p from memory early at the > start of the function, then just handle it in registers and only store it > after the foo call. It is fine in single-threaded environments, but if the > ctor can be invoked multiple times concurrently, it matters that we don't > hoist or sink the non-addressable local static loads/stores in any way > across the synchronization calls. > > To quote my comment from rhbz: > "Actually, it might be a glibc problem, > http://sources.redhat.com/git/?p=glibc.git;a=commitdiff;h=aa78043a4aafe5db1a1a76d544a833b63b4c5f5c > added leaf attribute, I bet it is undesirable to use leaf attribute on any of > the pthread.h/sem.h > synchronization primitives. Although they don't call functions from the > current compilation unit, > other threads might invoke functions from the current compilation unit and by > using the synchronization > primitives we are or might be waiting on those other threads to perform some > changes in the current translation > unit." > > As discussed with Richi, ECF_LEAF is used for 3 purposes, the ipa-reference > where we want to really treat synchronization primitives as non-leaf, > for abnormal edges following calls that might perform non-local goto > (sync primitives likely won't do that, but that affects only functions with > non-local labels or similar, i.e. rare) and finally for nonfreeing_call_p > (with a few exceptions). > > We might need to remove ECF_LEAF from some other builtins that can be used > for synchronization primitives (I guess some subset of sync-builtins.def, > some omp-builtins.def, but that can be done incrementally and might not need > backporting to release branches. > > The following patch fixes that by dropping ECF_LEAF from those two calls. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > Fixed the guality tests. > > 2019-10-22 Jakub Jelinek <ja...@redhat.com> > > PR tree-optimization/85887 > * decl.c (expand_static_init): Drop ECF_LEAF from __cxa_guard_acquire > and __cxa_guard_release. > > --- gcc/cp/decl.c.jj 2019-10-22 10:13:39.038094034 +0200 > +++ gcc/cp/decl.c 2019-10-22 12:17:49.558103149 +0200 > @@ -8612,14 +8612,14 @@ expand_static_init (tree decl, tree init > (acquire_name, build_function_type_list (integer_type_node, > TREE_TYPE (guard_addr), > NULL_TREE), > - NULL_TREE, ECF_NOTHROW | ECF_LEAF); > + NULL_TREE, ECF_NOTHROW); > if (!release_fn || !abort_fn) > vfntype = build_function_type_list (void_type_node, > TREE_TYPE (guard_addr), > NULL_TREE); > if (!release_fn) > release_fn = push_library_fn (release_name, vfntype, NULL_TREE, > - ECF_NOTHROW | ECF_LEAF); > + ECF_NOTHROW); > if (!abort_fn) > abort_fn = push_library_fn (abort_name, vfntype, NULL_TREE, > ECF_NOTHROW | ECF_LEAF); > > Jakub
