* [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
@ 2020-08-06 13:04 Maciej W. Rozycki
2020-08-06 20:18 ` Joseph Myers
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Maciej W. Rozycki @ 2020-08-06 13:04 UTC (permalink / raw)
To: gcc-patches
Cc: Richard Biener, Andreas Schwab, Richard Earnshaw, Kito Cheng,
Palmer Dabbelt, Andrew Waterman, Jim Wilson
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
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
2020-08-06 13:04 [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS Maciej W. Rozycki
@ 2020-08-06 20:18 ` Joseph Myers
2020-08-18 17:59 ` [PING][PATCH " Maciej W. Rozycki
2020-08-18 22:17 ` [PATCH " Richard Earnshaw
2 siblings, 0 replies; 7+ messages in thread
From: Joseph Myers @ 2020-08-06 20:18 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: gcc-patches, Andrew Waterman, Kito Cheng, Andreas Schwab,
Richard Earnshaw
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.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PING][PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
2020-08-06 13:04 [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS Maciej W. Rozycki
2020-08-06 20:18 ` Joseph Myers
@ 2020-08-18 17:59 ` Maciej W. Rozycki
2020-08-18 22:17 ` [PATCH " Richard Earnshaw
2 siblings, 0 replies; 7+ messages in thread
From: Maciej W. Rozycki @ 2020-08-18 17:59 UTC (permalink / raw)
To: gcc-patches
Cc: Richard Biener, Andreas Schwab, Richard Earnshaw, Kito Cheng,
Palmer Dabbelt, Andrew Waterman, Jim Wilson
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
2020-08-06 13:04 [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS Maciej W. Rozycki
2020-08-06 20:18 ` Joseph Myers
2020-08-18 17:59 ` [PING][PATCH " Maciej W. Rozycki
@ 2020-08-18 22:17 ` Richard Earnshaw
2020-08-19 12:54 ` Maciej W. Rozycki
2 siblings, 1 reply; 7+ messages in thread
From: Richard Earnshaw @ 2020-08-18 22:17 UTC (permalink / raw)
To: Maciej W. Rozycki, gcc-patches
Cc: Andrew Waterman, Kito Cheng, Andreas Schwab, Richard Earnshaw
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
2020-08-18 22:17 ` [PATCH " Richard Earnshaw
@ 2020-08-19 12:54 ` Maciej W. Rozycki
2020-08-19 13:36 ` Richard Earnshaw
0 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2020-08-19 12:54 UTC (permalink / raw)
To: Richard Earnshaw
Cc: gcc-patches, Andrew Waterman, Kito Cheng, Andreas Schwab,
Richard Earnshaw
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
2020-08-19 12:54 ` Maciej W. Rozycki
@ 2020-08-19 13:36 ` Richard Earnshaw
2020-08-20 18:47 ` Maciej W. Rozycki
0 siblings, 1 reply; 7+ messages in thread
From: Richard Earnshaw @ 2020-08-19 13:36 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Andreas Schwab, gcc-patches, Andrew Waterman, Richard Earnshaw,
Kito Cheng
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS
2020-08-19 13:36 ` Richard Earnshaw
@ 2020-08-20 18:47 ` Maciej W. Rozycki
0 siblings, 0 replies; 7+ messages in thread
From: Maciej W. Rozycki @ 2020-08-20 18:47 UTC (permalink / raw)
To: Richard Earnshaw
Cc: Richard Biener, Andreas Schwab, gcc-patches, Andrew Waterman,
Richard Earnshaw, Kito Cheng
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-08-20 18:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 13:04 [PATCH v2] libgcc: Use `-fasynchronous-unwind-tables' for LIB2_DIVMOD_FUNCS Maciej W. Rozycki
2020-08-06 20:18 ` Joseph Myers
2020-08-18 17:59 ` [PING][PATCH " Maciej W. Rozycki
2020-08-18 22:17 ` [PATCH " Richard Earnshaw
2020-08-19 12:54 ` Maciej W. Rozycki
2020-08-19 13:36 ` Richard Earnshaw
2020-08-20 18:47 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).