| Message ID | alpine.LFD.2.21.2008041716380.24175@redsun52.ssa.fujisawa.hgst.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS | expand |
On Thu, 6 Aug 2020, Maciej W. Rozycki via Gcc-patches wrote: > Given that for the `riscv64-linux-gnu' target and the ilp32d multilib > glibc currently fails to link against libgcc.a built at -O0 I first ran > reference testing with target libraries built at -O2, but comparing that > to change-under-test -O2 results revealed another issue with GCC target > libraries built at -O0 causing link failures across testsuites, namely > libgcov.a referring atomic primitives where libatomic.a has not been > linked in. I haven't figured out yet if the issue is in libgcov, the > testsuite or the specs. Examples of failures: That --as-needed -latomic --no-as-needed should be used by default to link in libatomic when required (with consequent changes needed for all testsuites) is a known issue; see bug 81358. Having such references in libgcov simply makes that known issue more visible.
On Thu, 6 Aug 2020, Maciej W. Rozycki wrote: > Complement commit b932f770f70d ("x86_64 frame unwind info"), SVN r46374, > <https://gcc.gnu.org/ml/gcc-patches/2001-10/msg00860.html>, and replace > `-fexceptions -fnon-call-exceptions' with `-fasynchronous-unwind-tables' > in LIB2_DIVMOD_FUNCS compilation flags so as to provide unwind tables > for the affected functions while not pulling the unwinder proper, which > is not required here. > > Remove the ARM overrides accordingly, retaining the hook infrastructure > however, and make the ARM test case a generic one. > > Beyond saving program space it fixes a RISC-V glibc build error due to > unsatisfied `malloc' and `free' references from the unwinder causing > link errors with `ld.so' where libgcc has been built at -O0. Ping for: <https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551508.html> Maciej
On 06/08/2020 14:04, Maciej W. Rozycki via Gcc-patches wrote: > Complement commit b932f770f70d ("x86_64 frame unwind info"), SVN r46374, > <https://gcc.gnu.org/ml/gcc-patches/2001-10/msg00860.html>, and replace > `-fexceptions -fnon-call-exceptions' with `-fasynchronous-unwind-tables' > in LIB2_DIVMOD_FUNCS compilation flags so as to provide unwind tables > for the affected functions while not pulling the unwinder proper, which > is not required here. > > Remove the ARM overrides accordingly, retaining the hook infrastructure > however, and make the ARM test case a generic one. > > Beyond saving program space it fixes a RISC-V glibc build error due to > unsatisfied `malloc' and `free' references from the unwinder causing > link errors with `ld.so' where libgcc has been built at -O0. > > gcc/ > * testsuite/gcc.target/arm/div64-unwinding.c: Rename to... > * testsuite/gcc.dg/div64-unwinding.c: ... this. > > libgcc/ > * Makefile.in [!LIB2_DIVMOD_EXCEPTION_FLAGS] > (LIB2_DIVMOD_EXCEPTION_FLAGS): Replace `-fexceptions > -fnon-call-exceptions' with `-fasynchronous-unwind-tables'. > * config/arm/t-bpabi (LIB2_DIVMOD_EXCEPTION_FLAGS): Remove > variable. > * config/arm/t-netbsd-eabi (LIB2_DIVMOD_EXCEPTION_FLAGS): > Likewise. From a quick glance, I'm not convinced this is right for Arm, since the Arm unwind format does not support anything other than call-based exceptions. How did you test it? R. > --- > Hi, > > I realised we still use handwritten ChangeLog entries (I got confused > with now different policies each of the various pieces of the GNU > toolchain has), so here's v2 of the change with a fix for that problem > being the only update. > > Also I have since run verification with the `riscv64-linux-gnu' target > and the ilp32d multilib as more representative for the change being made. > No problems were observed, although the now enabled test case scored: > > UNSUPPORTED: gcc.dg/div64-unwinding.c > > of course with the target failing the `! *-*-linux*' condition. > > Given that for the `riscv64-linux-gnu' target and the ilp32d multilib > glibc currently fails to link against libgcc.a built at -O0 I first ran > reference testing with target libraries built at -O2, but comparing that > to change-under-test -O2 results revealed another issue with GCC target > libraries built at -O0 causing link failures across testsuites, namely > libgcov.a referring atomic primitives where libatomic.a has not been > linked in. I haven't figured out yet if the issue is in libgcov, the > testsuite or the specs. Examples of failures: > > .../bin/riscv64-linux-gnu-ld: .../gcc/testsuite/g++/../../lib32/ilp32d/libgcov.a(_gcov_indirect_call_profiler_v4.o): in function `__gcov_topn_values_profiler_body': .../libgcc/libgcov-profiler.c:116: undefined reference to `__atomic_fetch_add_8' > .../bin/riscv64-linux-gnu-ld: .../libgcc/libgcov-profiler.c:129: undefined reference to `__atomic_fetch_add_8' > .../bin/riscv64-linux-gnu-ld: .../libgcc/libgcov-profiler.c:150: undefined reference to `__atomic_fetch_sub_8' > collect2: error: ld returned 1 exit status > compiler exited with status 1 > FAIL: g++.dg/other/pr55650.C -std=gnu++98 (test for excess errors) > > There were some odd Fortran failures too, with test cases failing to link, > making the results difficult to interpret. Therefore I decided to arrange > for a special build with first stage GCC built with its target libraries > at -O2, so that first stage glibc builds, and then second stage GCC built > with its target libraries at -O0 and second stage glibc omitted. That > removed the extra Fortran failures regardless of whether this change has > been applied or not, but we may consider looking overall into why a full > `riscv64-linux-gnu' build at -O0 has regressions against -O2 at least in > the ilp32d multilib. > > Meanwhile, OK to apply? > > Maciej > > Changes from v1: > > - ChangeLog entries added. > --- > gcc/testsuite/gcc.dg/div64-unwinding.c | 25 +++++++++++++++++++++++++ > gcc/testsuite/gcc.target/arm/div64-unwinding.c | 25 ------------------------- > libgcc/Makefile.in | 2 +- > libgcc/config/arm/t-bpabi | 5 ----- > libgcc/config/arm/t-netbsd-eabi | 5 ----- > 5 files changed, 26 insertions(+), 36 deletions(-) > > gcc-libgcc-divmod-asynchronous-unwind-tables.diff > Index: gcc/gcc/testsuite/gcc.dg/div64-unwinding.c > =================================================================== > --- /dev/null > +++ gcc/gcc/testsuite/gcc.dg/div64-unwinding.c > @@ -0,0 +1,25 @@ > +/* Performing a 64-bit division should not pull in the unwinder. */ > + > +/* { dg-do run { target { { ! *-*-linux* } && { ! *-*-uclinux* } } } } */ > +/* { dg-skip-if "load causes weak symbol resolution" { vxworks_kernel } } */ > +/* { dg-options "-O0" } */ > + > +#include <stdlib.h> > + > +long long > +foo (long long c, long long d) > +{ > + return c/d; > +} > + > +long long x = 0; > +long long y = 1; > + > +extern int (*_Unwind_RaiseException) (void *) __attribute__((weak)); > + > +int main(void) > +{ > + if (&_Unwind_RaiseException != NULL) > + abort ();; > + return foo (x, y); > +} > Index: gcc/gcc/testsuite/gcc.target/arm/div64-unwinding.c > =================================================================== > --- gcc.orig/gcc/testsuite/gcc.target/arm/div64-unwinding.c > +++ /dev/null > @@ -1,25 +0,0 @@ > -/* Performing a 64-bit division should not pull in the unwinder. */ > - > -/* { dg-do run { target { { ! *-*-linux* } && { ! *-*-uclinux* } } } } */ > -/* { dg-skip-if "load causes weak symbol resolution" { vxworks_kernel } } */ > -/* { dg-options "-O0" } */ > - > -#include <stdlib.h> > - > -long long > -foo (long long c, long long d) > -{ > - return c/d; > -} > - > -long long x = 0; > -long long y = 1; > - > -extern int (*_Unwind_RaiseException) (void *) __attribute__((weak)); > - > -int main(void) > -{ > - if (&_Unwind_RaiseException != NULL) > - abort ();; > - return foo (x, y); > -} > Index: gcc/libgcc/Makefile.in > =================================================================== > --- gcc.orig/libgcc/Makefile.in > +++ gcc/libgcc/Makefile.in > @@ -533,7 +533,7 @@ endif > ifeq ($(LIB2_DIVMOD_EXCEPTION_FLAGS),) > # Provide default flags for compiling divmod functions, if they haven't been > # set already by a target-specific Makefile fragment. > -LIB2_DIVMOD_EXCEPTION_FLAGS := -fexceptions -fnon-call-exceptions > +LIB2_DIVMOD_EXCEPTION_FLAGS := -fasynchronous-unwind-tables > endif > > # Build LIB2_DIVMOD_FUNCS. > Index: gcc/libgcc/config/arm/t-bpabi > =================================================================== > --- gcc.orig/libgcc/config/arm/t-bpabi > +++ gcc/libgcc/config/arm/t-bpabi > @@ -13,8 +13,3 @@ LIB2ADDEH = $(srcdir)/config/arm/unwind- > > # Add the BPABI names. > SHLIB_MAPFILES += $(srcdir)/config/arm/libgcc-bpabi.ver > - > -# On ARM, specifying -fnon-call-exceptions will needlessly pull in > -# the unwinder in simple programs which use 64-bit division. Omitting > -# the option is safe. > -LIB2_DIVMOD_EXCEPTION_FLAGS := -fexceptions > Index: gcc/libgcc/config/arm/t-netbsd-eabi > =================================================================== > --- gcc.orig/libgcc/config/arm/t-netbsd-eabi > +++ gcc/libgcc/config/arm/t-netbsd-eabi > @@ -11,8 +11,3 @@ LIB2ADDEH = > > # Add the BPABI names. > SHLIB_MAPFILES += $(srcdir)/config/arm/libgcc-bpabi.ver > - > -# On ARM, specifying -fnon-call-exceptions will needlessly pull in > -# the unwinder in simple programs which use 64-bit division. Omitting > -# the option is safe. > -LIB2_DIVMOD_EXCEPTION_FLAGS := -fexceptions >
On Tue, 18 Aug 2020, Richard Earnshaw wrote: > > Complement commit b932f770f70d ("x86_64 frame unwind info"), SVN r46374, > > <https://gcc.gnu.org/ml/gcc-patches/2001-10/msg00860.html>, and replace > > `-fexceptions -fnon-call-exceptions' with `-fasynchronous-unwind-tables' > > in LIB2_DIVMOD_FUNCS compilation flags so as to provide unwind tables > > for the affected functions while not pulling the unwinder proper, which > > is not required here. > > > > Remove the ARM overrides accordingly, retaining the hook infrastructure > > however, and make the ARM test case a generic one. [...] > From a quick glance, I'm not convinced this is right for Arm, since the > Arm unwind format does not support anything other than call-based > exceptions. How did you test it? Surely no ARM target has been verified as I have no access to such configurations; I will appreciate if either you or someone else suitably equipped does that for the sake of cross-target code unification (= fewer special cases to maintain). This has been regression-tested with RISC-V and x86-64 targets, as noted in the two submissions. Are you trying to say that `-fasynchronous-unwind-tables' has no effect on ARM? This code does not throw exceptions, so any unwinding would only happen in contexts such as in GDB poking at this code or from a signal handler such as SIGALRM or SIGFPE (if ARM does ever send the latter signal for integer division operations; I don't know offhand). The GCC option is generic and is supposed to fully support such use cases regardless of the target chosen, so shouldn't the ARM backend be wired appropriately so as to use whatever unwind format is required to handle the use cases, regardless of whether the minimal format usually used is supported by the psABI or not? There is indeed a documented provision for not supporting the option: "Generate unwind table in DWARF format, if supported by target machine." however I infer that refers to the support of the DWARF format as a whole rather than specifically minimal unwind tables, that is if the DWARF format is supported (as opposed to say stabs or mdebug only), then the option shall generate an unwind table in that format. That said I'm of course happy to keep the ARM overrides if you consider them still necessary in the context of the generic change made. Let me know what you prefer, and if required, I will submit v3 with the ARM pieces removed. Maciej
On 19/08/2020 13:54, Maciej W. Rozycki via Gcc-patches wrote: > On Tue, 18 Aug 2020, Richard Earnshaw wrote: > >>> Complement commit b932f770f70d ("x86_64 frame unwind info"), SVN r46374, >>> <https://gcc.gnu.org/ml/gcc-patches/2001-10/msg00860.html>, and replace >>> `-fexceptions -fnon-call-exceptions' with `-fasynchronous-unwind-tables' >>> in LIB2_DIVMOD_FUNCS compilation flags so as to provide unwind tables >>> for the affected functions while not pulling the unwinder proper, which >>> is not required here. >>> >>> Remove the ARM overrides accordingly, retaining the hook infrastructure >>> however, and make the ARM test case a generic one. > [...] >> From a quick glance, I'm not convinced this is right for Arm, since the >> Arm unwind format does not support anything other than call-based >> exceptions. How did you test it? > > Surely no ARM target has been verified as I have no access to such > configurations; I will appreciate if either you or someone else suitably > equipped does that for the sake of cross-target code unification (= fewer > special cases to maintain). This has been regression-tested with RISC-V > and x86-64 targets, as noted in the two submissions. > > Are you trying to say that `-fasynchronous-unwind-tables' has no effect > on ARM? This code does not throw exceptions, so any unwinding would only > happen in contexts such as in GDB poking at this code or from a signal > handler such as SIGALRM or SIGFPE (if ARM does ever send the latter signal > for integer division operations; I don't know offhand). The GCC option is > generic and is supposed to fully support such use cases regardless of the > target chosen, so shouldn't the ARM backend be wired appropriately so as > to use whatever unwind format is required to handle the use cases, > regardless of whether the minimal format usually used is supported by the > psABI or not? > > There is indeed a documented provision for not supporting the option: > "Generate unwind table in DWARF format, if supported by target machine." > however I infer that refers to the support of the DWARF format as a whole > rather than specifically minimal unwind tables, that is if the DWARF > format is supported (as opposed to say stabs or mdebug only), then the > option shall generate an unwind table in that format. > > That said I'm of course happy to keep the ARM overrides if you consider > them still necessary in the context of the generic change made. Let me > know what you prefer, and if required, I will submit v3 with the ARM > pieces removed. > > Maciej > So you've made a change to the Arm target, but not tested it. And what's more didn't even bother to mention that fact. If you make changes, you need to test them, particularly when there are likely to be target-specific implications. If you can't test yourself then you need to make that very clear in your submission. There are Arm targets in the testfarm, so it's not really an excuse for not doing testing. R.
On Wed, 19 Aug 2020, Richard Earnshaw wrote: > > That said I'm of course happy to keep the ARM overrides if you consider > > them still necessary in the context of the generic change made. Let me > > know what you prefer, and if required, I will submit v3 with the ARM > > pieces removed. [...] > So you've made a change to the Arm target, but not tested it. And > what's more didn't even bother to mention that fact. Well, I explicitly named the targets that have been tested, and it was clear ARM wasn't among them. I admit I forgot to cc ARM maintainers with v1, which I apologise for, and which mistake Richard B. has kindly corrected for me. Nobody's perfect. > If you make changes, you need to test them, particularly when there are > likely to be target-specific implications. If you can't test yourself > then you need to make that very clear in your submission. > > There are Arm targets in the testfarm, so it's not really an excuse for > not doing testing. I think it's the port maintainer's role to verify their pet target; that's what I have been doing on the binutils/GDB side when I was an active port maintainer. I did not require people to bend backwards and appreciated their effort to make the toolchain better. It takes a maintainer maybe a couple of seconds to pull a change and push it through their readily available automated verification system they surely have, while it may be a days' effort for someone who has to figure out all the details, choose all the configuration options required, avoid pitfalls, keep rebuilding until all is sound, etc. And then repeat that for every new target possibly affected. As the change was intended to address an issue observed with RISC-V targets the ARM pieces are not needed. I've sent v3 now, which keeps ARM-specific parts intact so that you won't have to be involved or otherwise spend your time on it. You're free to pick the parts removed of course and do whatever you want with them according to the GNU GPL and keeping in mind my copyright assignment with FSF. NB it is actually the case that when the original ARM fix/workaround was submitted that has introduced LIB2_DIVMOD_EXCEPTION_FLAGS, the failure, clearly not ARM-specific, should have been properly analysed and a general solution like mine proposed so as to fix all targets that use these libcalls, rather than taking care of your own business only, and making a local fix for ARM and letting other target developers rediscover the same issue. I regret now that I bothered touching the ARM part; I'll follow the example from the paragraph above and in the future I will only take care of my business, avoid going the extra mile in the future where it could only cause me trouble and give no benefit. Thank you for your review anyway, it has taught me something. Maciej
Index: gcc/gcc/testsuite/gcc.dg/div64-unwinding.c =================================================================== --- /dev/null +++ gcc/gcc/testsuite/gcc.dg/div64-unwinding.c @@ -0,0 +1,25 @@ +/* Performing a 64-bit division should not pull in the unwinder. */ + +/* { dg-do run { target { { ! *-*-linux* } && { ! *-*-uclinux* } } } } */ +/* { dg-skip-if "load causes weak symbol resolution" { vxworks_kernel } } */ +/* { dg-options "-O0" } */ + +#include <stdlib.h> + +long long +foo (long long c, long long d) +{ + return c/d; +} + +long long x = 0; +long long y = 1; + +extern int (*_Unwind_RaiseException) (void *) __attribute__((weak)); + +int main(void) +{ + if (&_Unwind_RaiseException != NULL) + abort ();; + return foo (x, y); +} Index: gcc/gcc/testsuite/gcc.target/arm/div64-unwinding.c =================================================================== --- gcc.orig/gcc/testsuite/gcc.target/arm/div64-unwinding.c +++ /dev/null @@ -1,25 +0,0 @@ -/* Performing a 64-bit division should not pull in the unwinder. */ - -/* { dg-do run { target { { ! *-*-linux* } && { ! *-*-uclinux* } } } } */ -/* { dg-skip-if "load causes weak symbol resolution" { vxworks_kernel } } */ -/* { dg-options "-O0" } */ - -#include <stdlib.h> - -long long -foo (long long c, long long d) -{ - return c/d; -} - -long long x = 0; -long long y = 1; - -extern int (*_Unwind_RaiseException) (void *) __attribute__((weak)); - -int main(void) -{ - if (&_Unwind_RaiseException != NULL) - abort ();; - return foo (x, y); -} Index: gcc/libgcc/Makefile.in =================================================================== --- gcc.orig/libgcc/Makefile.in +++ gcc/libgcc/Makefile.in @@ -533,7 +533,7 @@ endif ifeq ($(LIB2_DIVMOD_EXCEPTION_FLAGS),) # Provide default flags for compiling divmod functions, if they haven't been # set already by a target-specific Makefile fragment. -LIB2_DIVMOD_EXCEPTION_FLAGS := -fexceptions -fnon-call-exceptions +LIB2_DIVMOD_EXCEPTION_FLAGS := -fasynchronous-unwind-tables endif # Build LIB2_DIVMOD_FUNCS. Index: gcc/libgcc/config/arm/t-bpabi =================================================================== --- gcc.orig/libgcc/config/arm/t-bpabi +++ gcc/libgcc/config/arm/t-bpabi @@ -13,8 +13,3 @@ LIB2ADDEH = $(srcdir)/config/arm/unwind- # Add the BPABI names. SHLIB_MAPFILES += $(srcdir)/config/arm/libgcc-bpabi.ver - -# On ARM, specifying -fnon-call-exceptions will needlessly pull in -# the unwinder in simple programs which use 64-bit division. Omitting -# the option is safe. -LIB2_DIVMOD_EXCEPTION_FLAGS := -fexceptions Index: gcc/libgcc/config/arm/t-netbsd-eabi =================================================================== --- gcc.orig/libgcc/config/arm/t-netbsd-eabi +++ gcc/libgcc/config/arm/t-netbsd-eabi @@ -11,8 +11,3 @@ LIB2ADDEH = # Add the BPABI names. SHLIB_MAPFILES += $(srcdir)/config/arm/libgcc-bpabi.ver - -# On ARM, specifying -fnon-call-exceptions will needlessly pull in -# the unwinder in simple programs which use 64-bit division. Omitting -# the option is safe. -LIB2_DIVMOD_EXCEPTION_FLAGS := -fexceptions
Complement commit b932f770f70d ("x86_64 frame unwind info"), SVN r46374, <https://gcc.gnu.org/ml/gcc-patches/2001-10/msg00860.html>, and replace `-fexceptions -fnon-call-exceptions' with `-fasynchronous-unwind-tables' in LIB2_DIVMOD_FUNCS compilation flags so as to provide unwind tables for the affected functions while not pulling the unwinder proper, which is not required here. Remove the ARM overrides accordingly, retaining the hook infrastructure however, and make the ARM test case a generic one. Beyond saving program space it fixes a RISC-V glibc build error due to unsatisfied `malloc' and `free' references from the unwinder causing link errors with `ld.so' where libgcc has been built at -O0. gcc/ * testsuite/gcc.target/arm/div64-unwinding.c: Rename to... * testsuite/gcc.dg/div64-unwinding.c: ... this. libgcc/ * Makefile.in [!LIB2_DIVMOD_EXCEPTION_FLAGS] (LIB2_DIVMOD_EXCEPTION_FLAGS): Replace `-fexceptions -fnon-call-exceptions' with `-fasynchronous-unwind-tables'. * config/arm/t-bpabi (LIB2_DIVMOD_EXCEPTION_FLAGS): Remove variable. * config/arm/t-netbsd-eabi (LIB2_DIVMOD_EXCEPTION_FLAGS): Likewise. --- Hi, I realised we still use handwritten ChangeLog entries (I got confused with now different policies each of the various pieces of the GNU toolchain has), so here's v2 of the change with a fix for that problem being the only update. Also I have since run verification with the `riscv64-linux-gnu' target and the ilp32d multilib as more representative for the change being made. No problems were observed, although the now enabled test case scored: UNSUPPORTED: gcc.dg/div64-unwinding.c of course with the target failing the `! *-*-linux*' condition. Given that for the `riscv64-linux-gnu' target and the ilp32d multilib glibc currently fails to link against libgcc.a built at -O0 I first ran reference testing with target libraries built at -O2, but comparing that to change-under-test -O2 results revealed another issue with GCC target libraries built at -O0 causing link failures across testsuites, namely libgcov.a referring atomic primitives where libatomic.a has not been linked in. I haven't figured out yet if the issue is in libgcov, the testsuite or the specs. Examples of failures: .../bin/riscv64-linux-gnu-ld: .../gcc/testsuite/g++/../../lib32/ilp32d/libgcov.a(_gcov_indirect_call_profiler_v4.o): in function `__gcov_topn_values_profiler_body': .../libgcc/libgcov-profiler.c:116: undefined reference to `__atomic_fetch_add_8' .../bin/riscv64-linux-gnu-ld: .../libgcc/libgcov-profiler.c:129: undefined reference to `__atomic_fetch_add_8' .../bin/riscv64-linux-gnu-ld: .../libgcc/libgcov-profiler.c:150: undefined reference to `__atomic_fetch_sub_8' collect2: error: ld returned 1 exit status compiler exited with status 1 FAIL: g++.dg/other/pr55650.C -std=gnu++98 (test for excess errors) There were some odd Fortran failures too, with test cases failing to link, making the results difficult to interpret. Therefore I decided to arrange for a special build with first stage GCC built with its target libraries at -O2, so that first stage glibc builds, and then second stage GCC built with its target libraries at -O0 and second stage glibc omitted. That removed the extra Fortran failures regardless of whether this change has been applied or not, but we may consider looking overall into why a full `riscv64-linux-gnu' build at -O0 has regressions against -O2 at least in the ilp32d multilib. Meanwhile, OK to apply? Maciej Changes from v1: - ChangeLog entries added. --- gcc/testsuite/gcc.dg/div64-unwinding.c | 25 +++++++++++++++++++++++++ gcc/testsuite/gcc.target/arm/div64-unwinding.c | 25 ------------------------- libgcc/Makefile.in | 2 +- libgcc/config/arm/t-bpabi | 5 ----- libgcc/config/arm/t-netbsd-eabi | 5 ----- 5 files changed, 26 insertions(+), 36 deletions(-) gcc-libgcc-divmod-asynchronous-unwind-tables.diff