| Message ID | 20160922132502.GF17376@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 22 September 2016 at 15:25, Jonathan Wakely <jwakely@redhat.com> wrote: > On 22/09/16 12:15 +0100, Jonathan Wakely wrote: >> >> On 22/09/16 11:16 +0100, Jonathan Wakely wrote: >>> >>> (Somebody should fix PR58938 so exception_ptr is portable). >> >> >> Christophe, would you be able to test this patch? >> >> It uses a single global mutex for exception_ptr objects, which doesn't >> scale well but that probably isn't a problem for processors without >> lock-free atomics for single words. >> >> This also solves the problem of mismatched -march options, where the >> header is compiled for a CPU that supports the atomics but >> libstdc++.so was built for an older CPU that doesn't support them, and >> linking fails (as in https://gcc.gnu.org/PR58938#c13). > > > We'd also need something like this extra piece, to ensure we don't > leak exceptions. Currently __gxx_exception_cleanup assumes that if > ATOMIC_INT_LOCK_FREE < 2 the referenceCount can never be greater than > 1, because there are not exception_ptr objects that could increase it. > > If we enable exception_ptr unconditionally then that assumption > doesn't hold. This patch uses the exception_ptr code to do the > cleanup, under the same mutex as any other increments and decrements > of the reference count. > > It's a bit of a hack though. > Should I have applied this one on top of the other? I ran a validation with it alone, and arm-none-eabi with default mode, cpu, and fpu does not build: In file included from /tmp/9260164_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_throw.cc:27:0: /tmp/9260164_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/exception_ptr.h:43:4: error: #error This platform does not support exception propagation. # error This platform does not support exception propagation. ^~~~~ make[4]: *** [eh_throw.lo] Error 1 In addition, on arm-none-eabi --with-mode=thumb --with-cpu=cortex-a9, I've noticed a regression in c++ - PASS now FAIL [PASS => FAIL]: g++.dg/opt/pr36449.C -std=gnu++11 execution test g++.dg/opt/pr36449.C -std=gnu++14 execution test g++.dg/opt/pr36449.C -std=gnu++98 execution test My logs show: qemu: uncaught target signal 11 (Segmentation fault) - core dumped The validation of the other patch is still running: I had to re-run it because the patch didn't apply because of the ChangeLog entry.
On 22/09/16 20:22 +0200, Christophe Lyon wrote: >On 22 September 2016 at 15:25, Jonathan Wakely <jwakely@redhat.com> wrote: >> On 22/09/16 12:15 +0100, Jonathan Wakely wrote: >>> >>> On 22/09/16 11:16 +0100, Jonathan Wakely wrote: >>>> >>>> (Somebody should fix PR58938 so exception_ptr is portable). >>> >>> >>> Christophe, would you be able to test this patch? >>> >>> It uses a single global mutex for exception_ptr objects, which doesn't >>> scale well but that probably isn't a problem for processors without >>> lock-free atomics for single words. >>> >>> This also solves the problem of mismatched -march options, where the >>> header is compiled for a CPU that supports the atomics but >>> libstdc++.so was built for an older CPU that doesn't support them, and >>> linking fails (as in https://gcc.gnu.org/PR58938#c13). >> >> >> We'd also need something like this extra piece, to ensure we don't >> leak exceptions. Currently __gxx_exception_cleanup assumes that if >> ATOMIC_INT_LOCK_FREE < 2 the referenceCount can never be greater than >> 1, because there are not exception_ptr objects that could increase it. >> >> If we enable exception_ptr unconditionally then that assumption >> doesn't hold. This patch uses the exception_ptr code to do the >> cleanup, under the same mutex as any other increments and decrements >> of the reference count. >> >> It's a bit of a hack though. >> >Should I have applied this one on top of the other? > >I ran a validation with it alone, and >arm-none-eabi with default mode, cpu, and fpu does not build: That's expected, the second patch requires the first one (you can't use exception_ptr unconditionally if it's only defined conditionally :-) >In file included from >/tmp/9260164_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_throw.cc:27:0: >/tmp/9260164_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/exception_ptr.h:43:4: >error: #error This platform does >not support exception propagation. > # error This platform does not support exception propagation. > ^~~~~ >make[4]: *** [eh_throw.lo] Error 1 > > >In addition, on arm-none-eabi --with-mode=thumb --with-cpu=cortex-a9, >I've noticed a regression in c++ > - PASS now FAIL [PASS => FAIL]: > > g++.dg/opt/pr36449.C -std=gnu++11 execution test > g++.dg/opt/pr36449.C -std=gnu++14 execution test > g++.dg/opt/pr36449.C -std=gnu++98 execution test > >My logs show: >qemu: uncaught target signal 11 (Segmentation fault) - core dumped Strange, I don't see how my patch could cause that. >The validation of the other patch is still running: I had to re-run it >because the >patch didn't apply because of the ChangeLog entry.
Hi Jonathan, On 23 September 2016 at 12:41, Jonathan Wakely <jwakely@redhat.com> wrote: > On 22/09/16 20:22 +0200, Christophe Lyon wrote: >> >> On 22 September 2016 at 15:25, Jonathan Wakely <jwakely@redhat.com> wrote: >>> >>> On 22/09/16 12:15 +0100, Jonathan Wakely wrote: >>>> >>>> >>>> On 22/09/16 11:16 +0100, Jonathan Wakely wrote: >>>>> >>>>> >>>>> (Somebody should fix PR58938 so exception_ptr is portable). >>>> >>>> >>>> >>>> Christophe, would you be able to test this patch? >>>> >>>> It uses a single global mutex for exception_ptr objects, which doesn't >>>> scale well but that probably isn't a problem for processors without >>>> lock-free atomics for single words. >>>> >>>> This also solves the problem of mismatched -march options, where the >>>> header is compiled for a CPU that supports the atomics but >>>> libstdc++.so was built for an older CPU that doesn't support them, and >>>> linking fails (as in https://gcc.gnu.org/PR58938#c13). >>> >>> >>> >>> We'd also need something like this extra piece, to ensure we don't >>> leak exceptions. Currently __gxx_exception_cleanup assumes that if >>> ATOMIC_INT_LOCK_FREE < 2 the referenceCount can never be greater than >>> 1, because there are not exception_ptr objects that could increase it. >>> >>> If we enable exception_ptr unconditionally then that assumption >>> doesn't hold. This patch uses the exception_ptr code to do the >>> cleanup, under the same mutex as any other increments and decrements >>> of the reference count. >>> >>> It's a bit of a hack though. >>> >> Should I have applied this one on top of the other? >> >> I ran a validation with it alone, and >> arm-none-eabi with default mode, cpu, and fpu does not build: > > > That's expected, the second patch requires the first one (you can't > use exception_ptr unconditionally if it's only defined conditionally > :-) > > >> In file included from >> >> /tmp/9260164_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_throw.cc:27:0: >> >> /tmp/9260164_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/exception_ptr.h:43:4: >> error: #error This platform does >> not support exception propagation. >> # error This platform does not support exception propagation. >> ^~~~~ >> make[4]: *** [eh_throw.lo] Error 1 >> >> >> In addition, on arm-none-eabi --with-mode=thumb --with-cpu=cortex-a9, >> I've noticed a regression in c++ >> - PASS now FAIL [PASS => FAIL]: >> >> g++.dg/opt/pr36449.C -std=gnu++11 execution test >> g++.dg/opt/pr36449.C -std=gnu++14 execution test >> g++.dg/opt/pr36449.C -std=gnu++98 execution test >> >> My logs show: >> qemu: uncaught target signal 11 (Segmentation fault) - core dumped > > > Strange, I don't see how my patch could cause that. > I've run validations with the 2 patches applied, and you can see the results here: http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/240339-pr58938-v3/report-build-info.html As you can see there are several regressions, including: 18_support/exception_ptr/40296.cc (test for excess errors) /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/18_support/exception_ptr/40296.cc: In function 'bool test01()': /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/18_support/exception_ptr/40296.cc:25: error: 'exception_ptr' is not a member of 'std' /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/18_support/exception_ptr/40296.cc:25: note: suggested alternative: 'fexcept_t' /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/18_support/exception_ptr/40296.cc:27: error: 'p' was not declared in this scope when compiling with -march=armv5t on arm*linux* targets. On arm-none-eabi, I still see the regressions I reported when gcc is configured --with-mode=thumb --with-cpu=cortex-a9 g++.dg/opt/pr36449.C -std=gnu++11 execution test g++.dg/opt/pr36449.C -std=gnu++14 execution test g++.dg/opt/pr36449.C -std=gnu++98 execution test I have no detail besides qemu: uncaught target signal 11 (Segmentation fault) - core dumped Finally, on arm-none-eabi using default mode/cpu, some tests no longer compile because: /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs/libstdc++.a(eh_ptr.o): In function `__gx x_dependent_exception_cleanup': /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:241: undefined reference to `__atomic_fetch_sub_4' /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs/libstdc++.a(eh_ptr.o): In function `eh_p tr_mutex': /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:39: undefined reference to `__sync_synchronize' /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:39: undefined reference to `__sync_synchronize' /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:39: undefined reference to `__sync_synchronize' /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:39: undefined reference to `__sync_synchronize' /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:39: undefined reference to `__sync_synchronize' /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs/libstdc++.a(eh_ptr.o):/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:39: more undefined references to `__sync_synchronize' follow /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs/libstdc++.a(eh_ptr.o): In function `std::rethrow_exception(std::__exception_ptr::exception_ptr)': /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/eh_ptr.cc:260: undefined reference to `__atomic_fetch_add_4' collect2: error: ld returned 1 exit status FAIL: g++.dg/abi/arm_cxa_vec1.C -std=c++98 (test for excess errors) Does this help? Hopefully the above link will be easy enough for you to access to the information you need. It's available for the next 2 months. Thanks, Christophe > > >> The validation of the other patch is still running: I had to re-run it >> because the >> patch didn't apply because of the ChangeLog entry.
diff --git a/libstdc++-v3/libsupc++/Makefile.am b/libstdc++-v3/libsupc++/Makefile.am index 2df31ff..dbfd00f 100644 --- a/libstdc++-v3/libsupc++/Makefile.am +++ b/libstdc++-v3/libsupc++/Makefile.am @@ -155,9 +155,9 @@ eh_terminate.o: eh_terminate.cc $(CXXCOMPILE) -std=gnu++11 -c $< eh_throw.lo: eh_throw.cc - $(LTCXXCOMPILE) -std=gnu++11 -c $< + $(LTCXXCOMPILE) -std=gnu++11 -fno-access-control -c $< eh_throw.o: eh_throw.cc - $(CXXCOMPILE) -std=gnu++11 -c $< + $(CXXCOMPILE) -std=gnu++11 -fno-access-control -c $< guard.lo: guard.cc $(LTCXXCOMPILE) -std=gnu++11 -c $< diff --git a/libstdc++-v3/libsupc++/Makefile.in b/libstdc++-v3/libsupc++/Makefile.in index e828ed9..f3e2193 100644 --- a/libstdc++-v3/libsupc++/Makefile.in +++ b/libstdc++-v3/libsupc++/Makefile.in @@ -884,9 +884,9 @@ eh_terminate.o: eh_terminate.cc $(CXXCOMPILE) -std=gnu++11 -c $< eh_throw.lo: eh_throw.cc - $(LTCXXCOMPILE) -std=gnu++11 -c $< + $(LTCXXCOMPILE) -std=gnu++11 -fno-access-control -c $< eh_throw.o: eh_throw.cc - $(CXXCOMPILE) -std=gnu++11 -c $< + $(CXXCOMPILE) -std=gnu++11 -fno-access-control -c $< guard.lo: guard.cc $(LTCXXCOMPILE) -std=gnu++11 -c $< diff --git a/libstdc++-v3/libsupc++/eh_throw.cc b/libstdc++-v3/libsupc++/eh_throw.cc index a05f4eb..0f61fbf 100644 --- a/libstdc++-v3/libsupc++/eh_throw.cc +++ b/libstdc++-v3/libsupc++/eh_throw.cc @@ -24,6 +24,7 @@ #include <bits/c++config.h> #include "unwind-cxx.h" +#include "exception_ptr.h" using namespace __cxxabiv1; @@ -42,17 +43,8 @@ __gxx_exception_cleanup (_Unwind_Reason_Code code, _Unwind_Exception *exc) if (code != _URC_FOREIGN_EXCEPTION_CAUGHT && code != _URC_NO_REASON) __terminate (header->exc.terminateHandler); -#if ATOMIC_INT_LOCK_FREE > 1 - if (__atomic_sub_fetch (&header->referenceCount, 1, __ATOMIC_ACQ_REL) == 0) - { -#endif - if (header->exc.exceptionDestructor) - header->exc.exceptionDestructor (header + 1); - - __cxa_free_exception (header + 1); -#if ATOMIC_INT_LOCK_FREE > 1 - } -#endif + std::__exception_ptr::exception_ptr ptr; + ptr._M_exception_object = header + 1; } extern "C" __cxa_refcounted_exception*