Bug 89093 - [9 Regression] C++ exception handling clobbers d8 VFP register
Summary: [9 Regression] C++ exception handling clobbers d8 VFP register
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 9.0
: P4 normal
Target Milestone: 9.0
Assignee: Ramana Radhakrishnan
URL:
Keywords: EH, wrong-code
: 89228 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-01-28 18:41 UTC by Florian Weimer
Modified: 2019-05-01 12:46 UTC (History)
7 users (show)

See Also:
Host:
Target: armv7l-unknown-linux-gnueabihf
Build:
Known to work: 8.1.0
Known to fail:
Last reconfirmed: 2019-01-29 00:00:00


Attachments
untested prototype patch. (1.10 KB, patch)
2019-01-29 11:55 UTC, Ramana Radhakrishnan
Details | Diff
new patch. (1.57 KB, patch)
2019-01-29 14:10 UTC, Ramana Radhakrishnan
Details | Diff
gcc9-pr89093.patch (1.19 KB, patch)
2019-02-01 09:03 UTC, Jakub Jelinek
Details | Diff
updated patch. (2.65 KB, text/plain)
2019-03-22 16:26 UTC, Ramana Radhakrishnan
Details
gcc9-pr89093.patch (3.47 KB, patch)
2019-04-18 15:01 UTC, Jakub Jelinek
Details | Diff
updated patch (3.73 KB, patch)
2019-04-18 16:53 UTC, Bernd Edlinger
Details | Diff
gcc9-pr89093.patch (3.65 KB, patch)
2019-04-18 18:58 UTC, Jakub Jelinek
Details | Diff
gcc9-pr89093.patch (3.68 KB, patch)
2019-04-19 06:22 UTC, Bernd Edlinger
Details | Diff
gcc9-pr89093.patch (3.76 KB, patch)
2019-04-19 09:06 UTC, Jakub Jelinek
Details | Diff
gcc9-pr89093.patch (3.77 KB, patch)
2019-04-22 08:49 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Weimer 2019-01-28 18:41:12 UTC
In glibc, we have a test, nptl/tst-thread-exit-clobber, that attempts to verify if registers are properly restored by unwinding.  (The actual target of the the test is pthread_exit, but it covers more than that.)

This tests fails when running with GCC 9 libstdc++, even if glibc and the test were built with GCC 8, and libgcc_s is replaced with the version for GCC 8 (which works when running against GCC 8 libstdc++).

In the test, the d8 register is not restored properly during unwinding, it is set to zero.  d9, d10 etc. are restored.

I noticed that in GCC 9, __gxx_personality_v0 saves the d8 VFP register:

0007b620 <__gxx_personality_v0@@CXXABI_1.3>:
   7b620:       e92d4ff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, lr}
   7b624:       ed2d8b02        vpush   {d8}
   7b628:       e3a03000        mov     r3, #0
   7b62c:       e1a08001        mov     r8, r1

And it actually uses s16 and s17, apparently for spilling integer registers.  Perhaps the unwinder is not prepared to deal with that.

This happens with gcc version 9.0.0 20190119 (Red Hat 9.0.0-0.3) (GCC), built with:

--with-tune=generic-armv7-a --with-arch=armv7-a --with-float=hard --with-fpu=vfpv3-d16 --with-abi=aapcs-linux --build=armv7hl-redhat-linux-gnueabi
Comment 1 Andrew Pinski 2019-01-28 19:21:37 UTC
According to https://sourceware.org/ml/libc-alpha/2019-01/msg00651.html it worked in GCC 8.1.0.
Comment 2 Jakub Jelinek 2019-01-28 19:25:40 UTC
It is indeed saved in the prologue:
        @ args = 0, pretend = 0, frame = 64
        @ frame_needed = 0, uses_anonymous_args = 0
        and     ip, r0, #3
        push    {r4, r5, r6, r7, r8, r9, r10, fp, lr}
        .save {r4, r5, r6, r7, r8, r9, r10, fp, lr}
        cmp     ip, #1
        vpush.64        {d8}
        .vsave {d8}
        mov     r3, #0
        .pad #76
        sub     sp, sp, #76
but it is restored in the epilogue:
.L61:
        add     sp, sp, #76
        @ sp needed
        vldm    sp!, {d8}
        pop     {r4, r5, r6, r7, r8, r9, r10, fp, pc}
and there are no other returns that I can see.  So, why isn't it restored there?
Has the stack pointer been clobbered, or the stack slot in there?
eh_personality.cc has been compiled with
-mtune=cortex-a9 -mfloat-abi=hard -mfpu=vfpv3-d16 -mtls-dialect=gnu -marm -march=armv7-a+fp -O2 -fPIC
Comment 3 Jakub Jelinek 2019-01-28 19:29:57 UTC
Like in https://bugzilla.redhat.com/show_bug.cgi?id=1670069 (another wrong-code that has been reported to us today), eh_personality.cc (__gxx_personality_v0) is compiled this way since r266385.  Though, in this case it seems more like a missed optimization, at least I still fail to understand what is incorrect on it.
Comment 4 Florian Weimer 2019-01-28 20:28:13 UTC
So I'm not really an Arm person or a GCC person, but doesn't the personality routine call the landing pad, as identified by the LDSA?  And then that ends with a call to __cxa_end_cleanup, which is clear a no-return function (I think other targets call this function _Unwind_Resume).  So the epilogue of the personality routine never runs.

I doubt the Arm unwinding information covers the the VFP registers, so there is not much the unwinder can do about this.
Comment 5 Jakub Jelinek 2019-01-28 20:36:11 UTC
I admit I don't know much about ARM unwinding, which is different from all the other arches, but normally the personality routine doesn't call the landing pad code nor anything similar, it should set some registers in unwind context and return and through return value and whatever it stores into the unwind context tell the caller what to do (whether to install the context etc.).
Comment 6 Florian Weimer 2019-01-28 20:44:31 UTC
Okay, please assume that __gxx_personality_v0 is a red herring.  Apparently, there is unwinding information for VFP registers, too.
Comment 7 Ramana Radhakrishnan 2019-01-28 22:08:53 UTC
(In reply to Florian Weimer from comment #0)
> In glibc, we have a test, nptl/tst-thread-exit-clobber, that attempts to
> verify if registers are properly restored by unwinding.  (The actual target
> of the the test is pthread_exit, but it covers more than that.)
> 
> This tests fails when running with GCC 9 libstdc++, even if glibc and the
> test were built with GCC 8, and libgcc_s is replaced with the version for
> GCC 8 (which works when running against GCC 8 libstdc++).
> 
> In the test, the d8 register is not restored properly during unwinding, it
> is set to zero.  d9, d10 etc. are restored.
> 
> I noticed that in GCC 9, __gxx_personality_v0 saves the d8 VFP register:
> 
> 0007b620 <__gxx_personality_v0@@CXXABI_1.3>:
>    7b620:       e92d4ff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, lr}
>    7b624:       ed2d8b02        vpush   {d8}
>    7b628:       e3a03000        mov     r3, #0
>    7b62c:       e1a08001        mov     r8, r1
> 
> And it actually uses s16 and s17, apparently for spilling integer registers.
> Perhaps the unwinder is not prepared to deal with that.

d8 is composed of s16 and s17. That should just be fine. The single precision FP registers are packed into double precision registers in the VFP architecture.

Ramana
Comment 8 joseph@codesourcery.com 2019-01-29 00:11:41 UTC
The Arm rule is that the EH machinery needs to avoid using VFP (or other 
non-core) registers so that the unwinder can save them on-demand only.  
See 
<http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038b/IHI0038B_ehabi.pdf> 
section 4.7.

This means the unwind support code in libgcc, and personality routines, 
need to be built not to use such registers (possibly with a series of 
-ffixed-* options).  To complicate things, some of that code can end up 
calling memcpy.  While __aeabi_memcpy is required to use core registers 
only, that doesn't apply to memcpy.  (There are also possible glibc 
dynamic linker issues, 
<https://sourceware.org/bugzilla/show_bug.cgi?id=15792>.)

I don't guarantee this issue with demand-saving of VFP registers is what's 
involved here, but it's a potential issue I identified when looking for 
Arm ABI issues in the GNU toolchain in 2008 (and included in a list of 
such issues sent to Arm in 2010).
Comment 9 Jakub Jelinek 2019-01-29 09:27:02 UTC
If the EABI has such requirements, then libgcc/config/arm/t-arm (or whatever else) needs to pass down -msoft-float (or whatever else disables the VFP registers), rather than relying on that the compiler won't choose them.
Comment 10 Ramana Radhakrishnan 2019-01-29 11:55:46 UTC
Created attachment 45547 [details]
untested prototype patch.

Not sure if this is complete yet but it gives a framework to dig further.
Comment 11 Jakub Jelinek 2019-01-29 12:04:54 UTC
Comment on attachment 45547 [details]
untested prototype patch.

Doesn't the whole unwinder (so eh_personality.cc (whole, not just one function in it), unwind-arm.c, unwind-c.c, maybe some other unwind-*.c)) need that?
Comment 12 Jakub Jelinek 2019-01-29 12:06:41 UTC
If one appends -mfloat-abi=soft to command lines of those files, does that imply incompatible ABI even if nothing is passed in float/VFP etc. registers nor there is any floating point code?
Comment 13 Ramana Radhakrishnan 2019-01-29 12:20:31 UTC
(In reply to Jakub Jelinek from comment #12)
> If one appends -mfloat-abi=soft to command lines of those files, does that
> imply incompatible ABI even if nothing is passed in float/VFP etc. registers
> nor there is any floating point code?

-mfloat-abi=soft is an interesting option, it means use floating point emulation code using the base pcs as well as use the base parameter passing conventions for passing floating point parameters to functions. 

so it would end up failing at link time . That's why we need an -mfpu=none option which is silent and I've not liked it for a while. 

(In reply to Jakub Jelinek from comment #11)
> Comment on attachment 45547 [details]
> untested prototype patch.
> 
> Doesn't the whole unwinder (so eh_personality.cc (whole, not just one
> function in it), unwind-arm.c, unwind-c.c, maybe some other unwind-*.c))
> need that?

Yes that would be needed. Reading the EHABI again suggests that - I don't see a macro that would help with that everywhere.
Comment 14 Jakub Jelinek 2019-01-29 12:47:07 UTC
We require GNU make, so one can use something like:
unwind-arm.o unwind-c.o libunwind.o pr-support.o: CFLAGS += -mfpu=none
or similar in libgcc/config/arm/t-arm (or similar) with a comment explaining the reason.  For eh_personality.o that needs to be done elsewhere and there are no such makefile fragments (and libtool is used).
Comment 15 Ramana Radhakrishnan 2019-01-29 14:10:16 UTC
Created attachment 45552 [details]
new patch.

Testing this and would be grateful for a test run.
Comment 16 Ramana Radhakrishnan 2019-01-29 14:11:38 UTC
(In reply to Jakub Jelinek from comment #14)
> We require GNU make, so one can use something like:
> unwind-arm.o unwind-c.o libunwind.o pr-support.o: CFLAGS += -mfpu=none
> or similar in libgcc/config/arm/t-arm (or similar) with a comment explaining
> the reason.  For eh_personality.o that needs to be done elsewhere and there
> are no such makefile fragments (and libtool is used).

Sadly that doesn't work for -mfpu=none in t-arm because we still need gcc-9 to build with older binutils that don't necessarily support -mfpu=none, thus for now let's hide this with target pragmas.
Comment 17 Florian Weimer 2019-01-29 14:57:48 UTC
(In reply to Ramana Radhakrishnan from comment #15)
> Created attachment 45552 [details]
> new patch.
> 
> Testing this and would be grateful for a test run.

I believe the #pragma GCC push_options needs to come first, but it shouldn't matter in this context because the option set is never actually restored.

(I have not tested the patch yet.)
Comment 18 Florian Weimer 2019-01-29 16:37:00 UTC
(In reply to Ramana Radhakrishnan from comment #15)
> Created attachment 45552 [details]
> new patch.
> 
> Testing this and would be grateful for a test run.

Is this hunk needed as well, or will the unwinding information take care of this?  (__cxa_call_unexpected has another d8 register spill.)

Index: libstdc++-v3/libsupc++/eh_call.cc
===================================================================
--- libstdc++-v3/libsupc++/eh_call.cc   (revision 268364)
+++ libstdc++-v3/libsupc++/eh_call.cc   (working copy)
@@ -22,6 +22,11 @@
 // see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 // <http://www.gnu.org/licenses/>.
 
+#ifdef __arm__
+#pragma GCC target ("fpu=none")
+#pragma GCC push_options
+#endif
+
 #include <bits/c++config.h>
 #include <cstdlib>
 #include <bits/exception_defines.h>
Comment 19 Jakub Jelinek 2019-01-29 16:42:56 UTC
(In reply to Florian Weimer from comment #18)
> (In reply to Ramana Radhakrishnan from comment #15)
> > Testing this and would be grateful for a test run.
> 
> Is this hunk needed as well, or will the unwinding information take care of
> this?  (__cxa_call_unexpected has another d8 register spill.)

No idea here.

> --- libstdc++-v3/libsupc++/eh_call.cc   (revision 268364)
> +++ libstdc++-v3/libsupc++/eh_call.cc   (working copy)
> @@ -22,6 +22,11 @@
>  // see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  // <http://www.gnu.org/licenses/>.
>  
> +#ifdef __arm__
> +#pragma GCC target ("fpu=none")
> +#pragma GCC push_options
> +#endif

But why the #pragma GCC push_options?  That makes no sense.
Either you need to push options before GCC target and pop later on, but if you pop at the end of TU and don't really expect anything else to be emitted there, only #pragma GCC target should be enough (that applies to the other patch too).
Comment 20 Florian Weimer 2019-01-29 16:46:13 UTC
(In reply to Ramana Radhakrishnan from comment #15)
> Created attachment 45552 [details]
> new patch.
> 
> Testing this and would be grateful for a test run.

I can confirm that this patch fixes the glibc test suite failure, nptl/tst-thread-exit-clobber.
Comment 21 Ramana Radhakrishnan 2019-01-29 17:42:10 UTC
(In reply to Jakub Jelinek from comment #19)
> (In reply to Florian Weimer from comment #18)
> > (In reply to Ramana Radhakrishnan from comment #15)
> > > Testing this and would be grateful for a test run.
> > 
> > Is this hunk needed as well, or will the unwinding information take care of
> > this?  (__cxa_call_unexpected has another d8 register spill.)
> 
> No idea here.

I'll try and analyse that - The key is ensuring that there is absolutely no floating point code in eh_call.cc , if there is likely to be floating point anywhere this isn't correct 

> 
> > --- libstdc++-v3/libsupc++/eh_call.cc   (revision 268364)
> > +++ libstdc++-v3/libsupc++/eh_call.cc   (working copy)
> > @@ -22,6 +22,11 @@
> >  // see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> >  // <http://www.gnu.org/licenses/>.
> >  
> > +#ifdef __arm__
> > +#pragma GCC target ("fpu=none")
> > +#pragma GCC push_options
> > +#endif
> 
> But why the #pragma GCC push_options?  That makes no sense.
> Either you need to push options before GCC target and pop later on, but if
> you pop at the end of TU and don't really expect anything else to be emitted
> there, only #pragma GCC target should be enough (that applies to the other
> patch too).

I think that's just percolated my quick hack to discuss the issue. 

It should be enough to do #pragma GCC target . The final patch I have does that . Thanks for confirming that the patch in it's essence fixes up the issue.

regards
Ramana
Comment 22 Jakub Jelinek 2019-01-29 21:31:10 UTC
One more issue, shouldn't the #pragma GCC target be added before all include files?  Various define many inline functions, e.g. unwind-pe.h or unwind-cxx.h.
Comment 23 Jakub Jelinek 2019-02-01 09:03:57 UTC
Created attachment 45580 [details]
gcc9-pr89093.patch

This is what we are successfully using in Fedora for now (passed bootstrap/regtest and fixed the issues).
Comment 24 Andrew Pinski 2019-02-06 23:11:16 UTC
*** Bug 89228 has been marked as a duplicate of this bug. ***
Comment 25 Bernd Edlinger 2019-02-07 08:38:30 UTC
you might consider adding something like that to your patch:

Index: elf.h
===================================================================
--- elf.h	(revision 268337)
+++ elf.h	(working copy)
@@ -64,7 +64,7 @@
 %{mapcs-*:-mapcs-%*} \
 %(subtarget_asm_float_spec) \
 %{mthumb-interwork:-mthumb-interwork} \
-%{mfloat-abi=*} %{!mfpu=auto: %{mfpu=*}} \
+%{mfloat-abi=*} %{!mfpu=auto: %{!mfpu=none: %{mfpu=*}}} \
 %(subtarget_extra_asm_spec)"
 #endif
 


otherwise using -mfpu=none won't work on the command line.
becuse gas does not understand it.
Comment 26 Jakub Jelinek 2019-02-07 08:40:40 UTC
(In reply to Bernd Edlinger from comment #25)
> you might consider adding something like that to your patch:
> 
> Index: elf.h
> ===================================================================
> --- elf.h	(revision 268337)
> +++ elf.h	(working copy)
> @@ -64,7 +64,7 @@
>  %{mapcs-*:-mapcs-%*} \
>  %(subtarget_asm_float_spec) \
>  %{mthumb-interwork:-mthumb-interwork} \
> -%{mfloat-abi=*} %{!mfpu=auto: %{mfpu=*}} \
> +%{mfloat-abi=*} %{!mfpu=auto: %{!mfpu=none: %{mfpu=*}}} \
>  %(subtarget_extra_asm_spec)"
>  #endif
>  
> 
> 
> otherwise using -mfpu=none won't work on the command line.
> becuse gas does not understand it.

And if that works, then it might be cleaner to add -mfpu=none in libgcc/config/arm/t-arm for the libgcc objects.
Comment 27 Ramana Radhakrishnan 2019-02-08 11:38:09 UTC
(In reply to Bernd Edlinger from comment #25)
> you might consider adding something like that to your patch:
> 
> Index: elf.h
> ===================================================================
> --- elf.h	(revision 268337)
> +++ elf.h	(working copy)
> @@ -64,7 +64,7 @@
>  %{mapcs-*:-mapcs-%*} \
>  %(subtarget_asm_float_spec) \
>  %{mthumb-interwork:-mthumb-interwork} \
> -%{mfloat-abi=*} %{!mfpu=auto: %{mfpu=*}} \
> +%{mfloat-abi=*} %{!mfpu=auto: %{!mfpu=none: %{mfpu=*}}} \
>  %(subtarget_extra_asm_spec)"
>  #endif
>  
> 
> 
> otherwise using -mfpu=none won't work on the command line.
> becuse gas does not understand it.

Yes, that's what I've been playing with. I've run out of time this week because of other work commitments, I hope to get back to this early next week.

Ramana
Comment 28 Bernd Edlinger 2019-02-09 19:12:53 UTC
(In reply to Ramana Radhakrishnan from comment #27)
> (In reply to Bernd Edlinger from comment #25)
> > you might consider adding something like that to your patch:
> > 
> > Index: elf.h
> > ===================================================================
> > --- elf.h	(revision 268337)
> > +++ elf.h	(working copy)
> > @@ -64,7 +64,7 @@
> >  %{mapcs-*:-mapcs-%*} \
> >  %(subtarget_asm_float_spec) \
> >  %{mthumb-interwork:-mthumb-interwork} \
> > -%{mfloat-abi=*} %{!mfpu=auto: %{mfpu=*}} \
> > +%{mfloat-abi=*} %{!mfpu=auto: %{!mfpu=none: %{mfpu=*}}} \
> >  %(subtarget_extra_asm_spec)"
> >  #endif
> >  
> > 
> > 
> > otherwise using -mfpu=none won't work on the command line.
> > becuse gas does not understand it.
> 
> Yes, that's what I've been playing with. I've run out of time this week
> because of other work commitments, I hope to get back to this early next
> week.
> 
> Ramana

It is a bit unfortunate that -mfpu= on the command line seems to have
an impact on the eabi_attribute section, while the pragma does not:

          if (TARGET_HARD_FLOAT && TARGET_VFP_SINGLE)
            arm_emit_eabi_attribute ("Tag_ABI_HardFP_use", 27, 1);

          if (TARGET_HARD_FLOAT_ABI)
            arm_emit_eabi_attribute ("Tag_ABI_VFP_args", 28, 1);

where TARGET_HARD_FLOAT and TARGET_VFP_SINGLE are defined as follows:

#define TARGET_HARD_FLOAT       (arm_float_abi != ARM_FLOAT_ABI_SOFT    \
                                 && bitmap_bit_p (arm_active_target.isa, \
                                                  isa_bit_vfpv2) \
                                 && TARGET_32BIT)

#define TARGET_VFP_DOUBLE (bitmap_bit_p (arm_active_target.isa, isa_bit_fp_dbl))

#define TARGET_VFP_SINGLE (!TARGET_VFP_DOUBLE)


But arm_active_target.isa seems to depend on the effective -mfpu= command line
option, while later in the code generation the same macros are used to
select the use of VFP instructions.

I wonder if it would be better to have an orthogonal way to specify
the used ABI and the used register banks.

So like -mfpu=vfpv3-d16 and -mno-vfp,
where -mfpu= affects the eabi_attribute only
and -mno-vfp makes sure that no VFP registers are used, and, in particular
if functions are defined or called, where the ABI is incompatible because it
passes the values in VFP registers, that should be diagnosed, because it
will not work as expected.
Comment 29 Jakub Jelinek 2019-02-21 11:52:20 UTC
Ramana, any progress on this?
Comment 30 Ramana Radhakrishnan 2019-02-22 11:53:57 UTC
(In reply to Jakub Jelinek from comment #29)
> Ramana, any progress on this?

I'm still trying to get the various spec files and the t-multilib bits sorted and half-term has intervened here in the UK.
Comment 31 Jakub Jelinek 2019-03-22 12:41:46 UTC
(In reply to Ramana Radhakrishnan from comment #30)
> (In reply to Jakub Jelinek from comment #29)
> > Ramana, any progress on this?
> 
> I'm still trying to get the various spec files and the t-multilib bits
> sorted and half-term has intervened here in the UK.

Any further progress?  This is a wrong-code P1, so really has to be fixed for GCC9.
Comment 32 Ramana Radhakrishnan 2019-03-22 16:26:02 UTC
Created attachment 46013 [details]
updated patch.

Having discussed this with Richard further , instead of adding -mfpu=none , we would prefer a mgeneral-regs-only option which keeps the 2 separate. 

I'm sorry about the time it has taken to get back but this is what I have right now. The hunk in eh_personality.cc isn't very pleasing for me but that's because of the warning in the ABI code which triggers on the build for a hard float build because we do have inline functions consuming floating point. 


Either I drop the warning or I keep the hunk in eh_personality.cc - any preferences / thoughts ?
Comment 33 Bernd Edlinger 2019-03-22 18:10:57 UTC
(In reply to Ramana Radhakrishnan from comment #32)
> 
> Either I drop the warning or I keep the hunk in eh_personality.cc - any
> preferences / thoughts ?

It would feel safer, if only the functions that need it
had a target attribute like:

_Unwind_Reason_Code
#ifdef __ARM_EABI_UNWINDER__
__attribute__((target("general-regs-only")))
PERSONALITY_FUNCTION (_Unwind_State state,
                      struct _Unwind_Exception* ue_header,
                      struct _Unwind_Context* context)
Comment 34 Jakub Jelinek 2019-04-09 13:35:07 UTC
@@ -30877,6 +30883,11 @@ arm_valid_target_attribute_rec (tree args, struct gcc_options *opts)
       else if (!strncmp (q, "arm", 3))
 	  opts->x_target_flags &= ~MASK_THUMB;
 
+      else if (!strncmp (q, "general-regs-only", strlen ("general-regs-only")))
+	{
+	  opts->x_target_flags |= MASK_GENERAL_REGS_ONLY;
+	}
+
       else if (!strncmp (q, "fpu=", 4))
 	{
 	  int fpu_index;

I'm really not sure I understand this strncmp (but also the ones for arm and thumb), does that mean you want to accept also general-regs-only123 or
general-regs-onlycorgewaldo or thumb__ ?  If you want to support e.g. only optional whitespace after the string, each handled case should update the pointer and then something after it should verify/diagnose.
Also, single stmt if bodies shouldn't be wrapped with {} and the arm case is misindented.

Otherwise, I think the eh_personality.cc change is acceptable to me (but ask Jonathan or other libstdc++ maintainers).
Comment 35 Jonathan Wakely 2019-04-09 19:33:13 UTC
(In reply to Bernd Edlinger from comment #33)
> (In reply to Ramana Radhakrishnan from comment #32)
> > 
> > Either I drop the warning or I keep the hunk in eh_personality.cc - any
> > preferences / thoughts ?
> 
> It would feel safer, if only the functions that need it
> had a target attribute like:
> 
> _Unwind_Reason_Code
> #ifdef __ARM_EABI_UNWINDER__
> __attribute__((target("general-regs-only")))
> PERSONALITY_FUNCTION (_Unwind_State state,
>                       struct _Unwind_Exception* ue_header,
>                       struct _Unwind_Context* context)

Agreed - will this work instead?
Comment 36 Jakub Jelinek 2019-04-10 09:07:52 UTC
(In reply to Jonathan Wakely from comment #35)
> (In reply to Bernd Edlinger from comment #33)
> > (In reply to Ramana Radhakrishnan from comment #32)
> > > 
> > > Either I drop the warning or I keep the hunk in eh_personality.cc - any
> > > preferences / thoughts ?
> > 
> > It would feel safer, if only the functions that need it
> > had a target attribute like:
> > 
> > _Unwind_Reason_Code
> > #ifdef __ARM_EABI_UNWINDER__
> > __attribute__((target("general-regs-only")))
> > PERSONALITY_FUNCTION (_Unwind_State state,
> >                       struct _Unwind_Exception* ue_header,
> >                       struct _Unwind_Context* context)
> 
> Agreed - will this work instead?

Aren't there many functions that are inlined into it (then it would be fine) but could not be inlined (and then it would be a problem, at least in theory)?
Though, if there are inlines that are used in other TUs and those TUs are not general-regs-only and we decide not to inline, then it might be pure luck if COMDAT is won bu eh-personality.o versions or some other ones.
Comment 37 Bernd Edlinger 2019-04-10 10:15:59 UTC
If a non-general-regs-only function is called from here,
it will only preserve d8-d15, and the call-clobbered registers
d0-d7 would of course be modified.
But is that a problem at all, if the call-clobbered registers are not restored?
Comment 38 Richard Biener 2019-04-11 09:43:29 UTC
Isn't the issue also latent on all branches?
Comment 39 Jakub Jelinek 2019-04-11 09:55:33 UTC
(In reply to Richard Biener from comment #38)
> Isn't the issue also latent on all branches?

It is, but we have been lucky that the RA didn't decide to emit that.
On the trunk (unless something changed in RA since end of January) it unfortunately results in real issues, so we really can't ship GCC 9 with that.

(In reply to Bernd Edlinger from comment #37)
> If a non-general-regs-only function is called from here,
> it will only preserve d8-d15, and the call-clobbered registers
> d0-d7 would of course be modified.
> But is that a problem at all, if the call-clobbered registers are not
> restored?

I'm afraid I don't understand enough why the floating registers can't be used in the personality routine and other unwinder routines.  I'd think that even in the
personality routine itself the d8-d15 registers if they are used will be saved/restored first, so if that wasn't working, I'd say any use of those registers in the personality routine and anything it calls are problematic (and, note, not just C++ personality routine, we have also libgcc/unwind-c.c with C personality routine (also changed in the patch) and perhaps personality routines for other languages; I see e.g. libobjc/exception.c or gcc/ada/raise-gcc.c not patched though, and libphobos/libdruntime/gcc/deh.d), but perhaps it only cares about the personality routine itself and not on what it calls.  This needs to be clarified.
Comment 40 Bernd Edlinger 2019-04-11 14:23:33 UTC
Not that I invented this, but as far as I understand,
normally the interrupted execution context registers are saved on a
register file in memory.  But not on ARM.
On arm only the core registers are saved on a register file,
and the non-core, VFP registers are only saved when needed.
When the unwinder needs to restore a VFP register it is
done in _Unwind_VRS_Pop, and when in phase 1 this function
does save the current VFP register bank and does modify the
live VFP registers. Only when core registers are modified that
operates on the register file.
Therefore only the functions that invoke __gnu_unwind_execute
directly or indirectly must not save & restore any non-core registers
in the prologue and epilogue.
When function that saves & restores the d8-d15 but does clobber
d0-d7, that could cause those registers to be different than what
they would be otherwise.
It is possible that _Unwind_VRS_Pop could restore also call-clobbered
registers d0-d7, but I don't know if those registers would
actually need to be restored to their previous state.
Comment 41 Bernd Edlinger 2019-04-11 15:26:43 UTC
(In reply to Jakub Jelinek from comment #39)
> (and, note, not just C++ personality routine, we have also libgcc/unwind-c.c
> with C personality routine (also changed in the patch) and perhaps
> personality routines for other languages; I see e.g. libobjc/exception.c or
> gcc/ada/raise-gcc.c not patched though, and
> libphobos/libdruntime/gcc/deh.d), but perhaps it only cares about the
> personality routine itself and not on what it calls.  This needs to be
> clarified.

Yes you are absolutely right about objC, Ada, and D.

At the very least the personality routine, and all functions
that are calling __gnu_unwind_frame need that pragma.
in obj-C it is a macro CONTINUE_UNWINDING, that is fine.

In ada it is continue_unwind a function, that calls __gnu_unwind_frame,
and personality_body calling continue_unwind,
and the personality function itself.

In D it is CONTINUE_UNWINDING, a function.
scanLSDA, __gdc_personality, and gdc_personality itself.
Comment 42 Jakub Jelinek 2019-04-11 15:38:05 UTC
Thanks for the explanation.
In that case, I think it would be better to just add
__attribute__((target("general-regs-only")))
to the 
#ifdef __ARM_EABI_UNWINDER__
_Unwind_Reason_Code
PERSONALITY_FUNCTION (_Unwind_State, struct _Unwind_Exception *,
                      struct _Unwind_Context *);
decl in unwind-c.c and similarly for eh_personality.cc and to other personality routines that use CONTINUE_UNWINDING as well (plus to unwind-arm.c and pr-support.c using pragma for everything).
Comment 43 Bernd Edlinger 2019-04-11 21:27:04 UTC
does anybody know what is the Ada and/or D syntax for that?
Comment 44 Bernd Edlinger 2019-04-12 05:33:15 UTC
Comment on attachment 46013 [details]
updated patch.

@@ -122,12 +122,21 @@ extern tree arm_fp16_type_node;
 #define TARGET_32BIT_P(flags)  (TARGET_ARM_P (flags) || TARGET_THUMB2_P (flags))
 
 /* Run-time Target Specification.  */
-/* Use hardware floating point instructions. */
+/* Use hardware floating point instructions. -mgeneral-regs-only prevents
+the use of floating point instructions and registers but does not prevent
+emission of floating point pcs attributes.  */
 #define TARGET_HARD_FLOAT	(arm_float_abi != ARM_FLOAT_ABI_SOFT	\
+				 && bitmap_bit_p (arm_active_target.isa, \
+						  isa_bit_vfpv2) \
+				 && TARGET_32BIT \
+				 && !TARGET_GENERAL_REGS_ONLY)
+
+#define TARGET_HARD_FLOAT_SUB	(arm_float_abi != ARM_FLOAT_ABI_SOFT	\
 				 && bitmap_bit_p (arm_active_target.isa, \
 						  isa_bit_vfpv2) \
 				 && TARGET_32BIT)


BTW, you could define TARGET_HARD_FLOAT in terms of TARGET_HARD_FLOAT_SUB and
!TARGET_GENERAL_REGS_ONLY.
Comment 45 Ramana Radhakrishnan 2019-04-12 15:12:58 UTC
(In reply to Jakub Jelinek from comment #42)
> Thanks for the explanation.
> In that case, I think it would be better to just add
> __attribute__((target("general-regs-only")))
> to the 
> #ifdef __ARM_EABI_UNWINDER__
> _Unwind_Reason_Code
> PERSONALITY_FUNCTION (_Unwind_State, struct _Unwind_Exception *,
>                       struct _Unwind_Context *);
> decl in unwind-c.c and similarly for eh_personality.cc and to other
> personality routines that use CONTINUE_UNWINDING as well (plus to
> unwind-arm.c and pr-support.c using pragma for everything).

Thanks for all the analysis, this is what I had  - I've been swamped this week on a few other things, let me get this wrapped up soonish. (read it as during next week).(In reply to Bernd Edlinger from comment #44)
> Comment on attachment 46013 [details]
> updated patch.
> 
> @@ -122,12 +122,21 @@ extern tree arm_fp16_type_node;
>  #define TARGET_32BIT_P(flags)  (TARGET_ARM_P (flags) || TARGET_THUMB2_P
> (flags))
>  
>  /* Run-time Target Specification.  */
> -/* Use hardware floating point instructions. */
> +/* Use hardware floating point instructions. -mgeneral-regs-only prevents
> +the use of floating point instructions and registers but does not prevent
> +emission of floating point pcs attributes.  */
>  #define TARGET_HARD_FLOAT	(arm_float_abi != ARM_FLOAT_ABI_SOFT	\
> +				 && bitmap_bit_p (arm_active_target.isa, \
> +						  isa_bit_vfpv2) \
> +				 && TARGET_32BIT \
> +				 && !TARGET_GENERAL_REGS_ONLY)
> +
> +#define TARGET_HARD_FLOAT_SUB	(arm_float_abi != ARM_FLOAT_ABI_SOFT	\
>  				 && bitmap_bit_p (arm_active_target.isa, \
>  						  isa_bit_vfpv2) \
>  				 && TARGET_32BIT)
> 
> 
> BTW, you could define TARGET_HARD_FLOAT in terms of TARGET_HARD_FLOAT_SUB and
> !TARGET_GENERAL_REGS_ONLY.

Yep I could - been traveling quite a lot and I haven't managed to find someone else to catch this - I will pick this up next week .

My fault, apologies.

Ramana
Comment 46 Jakub Jelinek 2019-04-13 15:21:17 UTC
Author: jakub
Date: Sat Apr 13 15:20:46 2019
New Revision: 270339

URL: https://gcc.gnu.org/viewcvs?rev=270339&root=gcc&view=rev
Log:
	PR target/89093
	* config/arm/arm.c (arm_valid_target_attribute_rec): Use strcmp
	instead of strncmp when checking for thumb and arm.  Formatting fixes.

	* gcc.target/arm/pr89093.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/arm/pr89093.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog
Comment 47 Florian Weimer 2019-04-15 12:24:19 UTC
(In reply to Bernd Edlinger from comment #43)
> does anybody know what is the Ada and/or D syntax for that?

Syntax for what?

I fear we need eagerly load all vector registers before calling the personality routine.
Comment 48 Bernd Edlinger 2019-04-15 14:18:45 UTC
(In reply to Florian Weimer from comment #47)
> (In reply to Bernd Edlinger from comment #43)
> > does anybody know what is the Ada and/or D syntax for that?
> 
> Syntax for what?

I mean the Ada and D equivalent of
#pragma GCC target ("general-regs-only")
and/or
__attribute__((target("general-regs-only")))

> 
> I fear we need eagerly load all vector registers before calling the
> personality routine.

I am afraid the unwind handler will probably not know which coprocessor
registers exist before the unwind code hits them.
Comment 49 Florian Weimer 2019-04-16 12:14:22 UTC
(In reply to Bernd Edlinger from comment #48)
> (In reply to Florian Weimer from comment #47)
> > (In reply to Bernd Edlinger from comment #43)
> > > does anybody know what is the Ada and/or D syntax for that?
> > 
> > Syntax for what?
> 
> I mean the Ada and D equivalent of
> #pragma GCC target ("general-regs-only")
> and/or
> __attribute__((target("general-regs-only")))

I don't think the target pragma/attribute is supported in Ada.

   pragma Machine_Attribute (Subprogram, "target", "general-regs-only");

appears to be ignored (even if I use a string which would be accepted by the C/C++ pragma for that target, and not "general-regs-only".  But it's been years since I did much Ada programming.
Comment 50 Jakub Jelinek 2019-04-16 16:44:06 UTC
(In reply to Florian Weimer from comment #49)
> (In reply to Bernd Edlinger from comment #48)
> > (In reply to Florian Weimer from comment #47)
> > > (In reply to Bernd Edlinger from comment #43)
> > > > does anybody know what is the Ada and/or D syntax for that?
> > > 
> > > Syntax for what?
> > 
> > I mean the Ada and D equivalent of
> > #pragma GCC target ("general-regs-only")
> > and/or
> > __attribute__((target("general-regs-only")))
> 
> I don't think the target pragma/attribute is supported in Ada.
> 
>    pragma Machine_Attribute (Subprogram, "target", "general-regs-only");
> 
> appears to be ignored (even if I use a string which would be accepted by the
> C/C++ pragma for that target, and not "general-regs-only".  But it's been
> years since I did much Ada programming.

I don't understand why we are discussing Ada, because gcc/ada/raise_gcc.c, the TU containing the Ada personality routine, is written in C.
The only problem is the D personality routine, which is written in D.  And in that case IMHO we can just use -mgeneral-regs-only on the command line for arm in the Makefiles or something similar.
Comment 51 Jakub Jelinek 2019-04-17 08:31:17 UTC
Author: jakub
Date: Wed Apr 17 08:30:44 2019
New Revision: 270404

URL: https://gcc.gnu.org/viewcvs?rev=270404&root=gcc&view=rev
Log:
	PR target/89093
	* config/arm/arm.c (arm_valid_target_attribute_rec): Don't skip
	whitespace at the start of target attribute string.

	* gcc.target/arm/pr89093-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/arm/pr89093-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog
Comment 52 Bernd Edlinger 2019-04-17 09:36:03 UTC
I digged a bit, and found a D syntax for the target attribute,
it is a bit of a complication since D does not have a pre-processor,
but an empty target attribute does seem to be ignored without warnings.


--- libphobos/libdruntime/gcc/deh.d	2019-01-01 13:31:55.000000000 +0100
+++ libphobos/libdruntime/gcc/deh.d	2019-04-17 11:24:24.171579381 +0200
@@ -28,6 +28,7 @@ import gcc.unwind;
 import gcc.unwind.pe;
 import gcc.builtins;
 import gcc.config;
+import gcc.attribute;
 
 extern(C)
 {
@@ -519,10 +520,19 @@ extern(C) void _d_throw(Throwable object
     terminate("unwind error", __LINE__);
 }
 
+static if (GNU_ARM_EABI_Unwinder)
+{
+    enum TARGET_ATTRIBUTE = "general-regs-only";
+}
+else
+{
+    enum TARGET_ATTRIBUTE = "";
+}
 
 /**
  * Read and extract information from the LSDA (.gcc_except_table section).
  */
+@attribute("target", (TARGET_ATTRIBUTE))
 _Unwind_Reason_Code scanLSDA(const(ubyte)* lsda, _Unwind_Exception_Class exceptionClass,
                              _Unwind_Action actions, _Unwind_Exception* unwindHeader,
                              _Unwind_Context* context, _Unwind_Word cfa,
@@ -772,6 +782,7 @@ int actionTableLookup(_Unwind_Action act
  * Called when the personality function has found neither a cleanup or handler.
  * To support ARM EABI personality routines, that must also unwind the stack.
  */
+@attribute("target", (TARGET_ATTRIBUTE))
 _Unwind_Reason_Code CONTINUE_UNWINDING(_Unwind_Exception* unwindHeader, _Unwind_Context* context)
 {
     static if (GNU_ARM_EABI_Unwinder)
@@ -814,6 +825,7 @@ else
 static if (GNU_ARM_EABI_Unwinder)
 {
     pragma(mangle, PERSONALITY_FUNCTION)
+    @attribute("target", (TARGET_ATTRIBUTE))
     extern(C) _Unwind_Reason_Code gdc_personality(_Unwind_State state,
                                                   _Unwind_Exception* unwindHeader,
                                                   _Unwind_Context* context)
@@ -873,6 +885,7 @@ else
     }
 }
 
+@attribute("target", (TARGET_ATTRIBUTE))
 private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                               _Unwind_Exception_Class exceptionClass,
                                               _Unwind_Exception* unwindHeader,
Comment 53 Jakub Jelinek 2019-04-17 09:48:43 UTC
(In reply to Bernd Edlinger from comment #52)
> I digged a bit, and found a D syntax for the target attribute,
> it is a bit of a complication since D does not have a pre-processor,
> but an empty target attribute does seem to be ignored without warnings.
> 
> 
> --- libphobos/libdruntime/gcc/deh.d	2019-01-01 13:31:55.000000000 +0100
> +++ libphobos/libdruntime/gcc/deh.d	2019-04-17 11:24:24.171579381 +0200
> @@ -28,6 +28,7 @@ import gcc.unwind;
>  import gcc.unwind.pe;
>  import gcc.builtins;
>  import gcc.config;
> +import gcc.attribute;
>  
>  extern(C)
>  {
> @@ -519,10 +520,19 @@ extern(C) void _d_throw(Throwable object
>      terminate("unwind error", __LINE__);
>  }
>  
> +static if (GNU_ARM_EABI_Unwinder)
> +{
> +    enum TARGET_ATTRIBUTE = "general-regs-only";
> +}
> +else
> +{
> +    enum TARGET_ATTRIBUTE = "";
> +}
>  
>  /**
>   * Read and extract information from the LSDA (.gcc_except_table section).
>   */
> +@attribute("target", (TARGET_ATTRIBUTE))
>  _Unwind_Reason_Code scanLSDA(const(ubyte)* lsda, _Unwind_Exception_Class
> exceptionClass,
>                               _Unwind_Action actions, _Unwind_Exception*
> unwindHeader,
>                               _Unwind_Context* context, _Unwind_Word cfa,
> @@ -772,6 +782,7 @@ int actionTableLookup(_Unwind_Action act
>   * Called when the personality function has found neither a cleanup or
> handler.
>   * To support ARM EABI personality routines, that must also unwind the
> stack.
>   */
> +@attribute("target", (TARGET_ATTRIBUTE))
>  _Unwind_Reason_Code CONTINUE_UNWINDING(_Unwind_Exception* unwindHeader,
> _Unwind_Context* context)
>  {
>      static if (GNU_ARM_EABI_Unwinder)
> @@ -814,6 +825,7 @@ else
>  static if (GNU_ARM_EABI_Unwinder)
>  {
>      pragma(mangle, PERSONALITY_FUNCTION)
> +    @attribute("target", (TARGET_ATTRIBUTE))
>      extern(C) _Unwind_Reason_Code gdc_personality(_Unwind_State state,
>                                                    _Unwind_Exception*
> unwindHeader,
>                                                    _Unwind_Context* context)
> @@ -873,6 +885,7 @@ else
>      }
>  }
>  
> +@attribute("target", (TARGET_ATTRIBUTE))
>  private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
>                                                _Unwind_Exception_Class
> exceptionClass,
>                                                _Unwind_Exception*
> unwindHeader,

That is not going to work I'm afraid, many targets don't support target attribute at all.
default_target_option_valid_attribute_p
will then just complain.
Only i386, rs6000, s390, arm, aarch64 and nios2 backends do support those.
On the other side, given the above, I thought all you want to ensure is that
the attribute is on the personality routine, not on the other ones, and the
gdc_personality definition is in
static if (GNU_ARM_EABI_Unwinder)
{
    pragma(mangle, PERSONALITY_FUNCTION)
    extern(C) _Unwind_Reason_Code gdc_personality(_Unwind_State state,
so can't you just stick @attribute("target", "general-regs-only") to there?
Comment 54 Bernd Edlinger 2019-04-17 12:38:23 UTC
Hmm, I see.  What I am trying to accomplish is, put the target
attribute on every function that calls directly or in-directly
to CONTINUE_UNWINDING.  And do that only for ARM.

For gdc_personality it is straight forward to do, as you pointed out.
But for __gdc_personality and scanLSDA what I would like to do is

static if (GNU_ARM_EABI_Unwinder)
{
  @attribute("target", ("general-regs-only"))
}
private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                              _Unwind_Exception_Class exceptionClass,
                                              _Unwind_Exception* unwindHeader,
                                              _Unwind_Context* context)
{
...


but that does not work, what would work is

static if (GNU_ARM_EABI_Unwinder)
{
  @attribute("target", ("general-regs-only"))
  private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                              _Unwind_Exception_Class exceptionClass,
                                              _Unwind_Exception* unwindHeader,
                                              _Unwind_Context* context)
  {
   ...
  }
}
else
{
  private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                              _Unwind_Exception_Class exceptionClass,
                                              _Unwind_Exception* unwindHeader,
                                              _Unwind_Context* context)
  {
   ...
  }
}

duplicating all that code is of course not an option.
Comment 55 Bernd Edlinger 2019-04-17 12:40:14 UTC
But, how about that:

Index: deh.d
===================================================================
--- deh.d	(revision 270395)
+++ deh.d	(working copy)
@@ -28,6 +28,7 @@
 import gcc.unwind.pe;
 import gcc.builtins;
 import gcc.config;
+import gcc.attribute;
 
 extern(C)
 {
@@ -519,10 +520,21 @@
     terminate("unwind error", __LINE__);
 }
 
+static if (GNU_ARM_EABI_Unwinder)
+{
+    enum ATTRIBUTE_NAME = "target";
+    enum ATTRIBUTE_ARGS = "general-regs-only";
+}
+else
+{
+    enum ATTRIBUTE_NAME = "";
+    enum ATTRIBUTE_ARGS = "";
+}
 
 /**
  * Read and extract information from the LSDA (.gcc_except_table section).
  */
+@attribute(ATTRIBUTE_NAME, (ATTRIBUTE_ARGS))
 _Unwind_Reason_Code scanLSDA(const(ubyte)* lsda, _Unwind_Exception_Class exceptionClass,
                              _Unwind_Action actions, _Unwind_Exception* unwindHeader,
                              _Unwind_Context* context, _Unwind_Word cfa,
@@ -772,6 +784,7 @@
  * Called when the personality function has found neither a cleanup or handler.
  * To support ARM EABI personality routines, that must also unwind the stack.
  */
+@attribute(ATTRIBUTE_NAME, (ATTRIBUTE_ARGS))
 _Unwind_Reason_Code CONTINUE_UNWINDING(_Unwind_Exception* unwindHeader, _Unwind_Context* context)
 {
     static if (GNU_ARM_EABI_Unwinder)
@@ -814,6 +827,7 @@
 static if (GNU_ARM_EABI_Unwinder)
 {
     pragma(mangle, PERSONALITY_FUNCTION)
+    @attribute(ATTRIBUTE_NAME, (ATTRIBUTE_ARGS))
     extern(C) _Unwind_Reason_Code gdc_personality(_Unwind_State state,
                                                   _Unwind_Exception* unwindHeader,
                                                   _Unwind_Context* context)
@@ -873,6 +887,7 @@
     }
 }
 
+@attribute(ATTRIBUTE_NAME, (ATTRIBUTE_ARGS))
 private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                               _Unwind_Exception_Class exceptionClass,
                                               _Unwind_Exception* unwindHeader,
Comment 56 Jakub Jelinek 2019-04-17 12:43:46 UTC
Can't you just add prototypes?
Like:
static if (GNU_ARM_EABI_Unwinder)
{
  @attribute("target", ("general-regs-only"))
  private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                              _Unwind_Exception_Class exceptionClass,
                                              _Unwind_Exception* unwindHeader,
                                              _Unwind_Context* context);
}
etc. before the actual definitions?  attribute ("", ("")) seems ugly.
Comment 57 Bernd Edlinger 2019-04-17 12:48:36 UTC
(In reply to Jakub Jelinek from comment #56)
> Can't you just add prototypes?
> Like:
> static if (GNU_ARM_EABI_Unwinder)
> {
>   @attribute("target", ("general-regs-only"))
>   private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
>                                               _Unwind_Exception_Class
> exceptionClass,
>                                               _Unwind_Exception*
> unwindHeader,
>                                               _Unwind_Context* context);
> }
> etc. before the actual definitions?  attribute ("", ("")) seems ugly.

I think that is worth a try.  hang on...
Comment 58 Bernd Edlinger 2019-04-17 13:50:26 UTC
No, sorry, the attribute on the prototype gets ignored, as the following
is compiled without generating an error:


private int test(double x)
{
  return x > 1.0;
}

static if (GNU_ARM_EABI_Unwinder)
{
    @attribute("target", ("general-regs-only"))
    private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                                  _Unwind_Exception_Class exceptionClass,
                                                  _Unwind_Exception* unwindHeader,
                                                  _Unwind_Context* context);
}

private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                              _Unwind_Exception_Class exceptionClass,
                                              _Unwind_Exception* unwindHeader,
                                              _Unwind_Context* context)
{
    const(ubyte)* lsda;
    _Unwind_Ptr landingPad;
    _Unwind_Word cfa;
    int handler;

test(3.14);
    // Shortcut for phase 2 found handler for domestic exception.
    if (actions == (_UA_CLEANUP_PHASE | _UA_HANDLER_FRAME)
        && isGdcExceptionClass(exceptionClass))
    {
        ExceptionHeader.restore(unwindHeader, handler, lsda, landingPad, cfa);

but with the previous patch calling test(3.14) gives this:
../../../../gcc-trunk/libphobos/libdruntime/gcc/deh.d: In function '__gdc_personality':
../../../../gcc-trunk/libphobos/libdruntime/gcc/deh.d:906:1: error: argument of type 'double' not permitted with -mgeneral-regs-only
  906 | test(3.14);
      | ^
Comment 59 Jakub Jelinek 2019-04-17 13:53:53 UTC
That looks like a D FE bug then.
In any case, why can't you just use -mgeneral-regs-only on the deh.d compilation command line?
Comment 60 Bernd Edlinger 2019-04-17 16:30:47 UTC
(In reply to Jakub Jelinek from comment #59)
> That looks like a D FE bug then.
> In any case, why can't you just use -mgeneral-regs-only on the deh.d
> compilation command line?

Could work, just anxious, that something in comdat segment depends on this flag.

Another alternative would be to re-factor the code so that
CONTINUE_UNWINDING just returns _URC_CONTINUE_UNWIND,
and _only_ gdc_personality does call __gnu_unwind_frame and has the
@attribute("target", ("general-regs-only")).
Comment 61 Jakub Jelinek 2019-04-17 16:51:24 UTC
At least looking at x86_64-linux gcc/deh.o, I really don't see any .text comdats, only data comdats, all STT_FUNC symbols are in the same section, except for the global ctors in .text.startup and dtors in .text.exit.
Comment 62 Iain Buclaw 2019-04-18 08:37:45 UTC
(In reply to Bernd Edlinger from comment #55)
> But, how about that:
> 

The gcc.attribute module just reuses the UDA mechanism, the value after @ can be any kind of compile-time evaluated tuple or literal.

So you can do instead. if it makes things much nicer for you.

static if (GNU_ARM_EABI_Unwinder)
    enum personality_fn_attributes = attribute("target", ("general-regs-only"))
else
    enum personality_fn_attributes = "";

@personality_fn_attributes
private _Unwind_Reason_Code __gdc_personality(...)
Comment 63 Iain Buclaw 2019-04-18 08:40:09 UTC
(In reply to Jakub Jelinek from comment #59)
> That looks like a D FE bug then.

That shouldn't be difficult, I've create PR d/90136 to keep track of that.
Comment 64 Bernd Edlinger 2019-04-18 12:16:35 UTC
Okay, using Ian's suggestion I've got this now:

Index: libphobos/libdruntime/gcc/deh.d
===================================================================
--- libphobos/libdruntime/gcc/deh.d	(revision 270395)
+++ libphobos/libdruntime/gcc/deh.d	(working copy)
@@ -28,6 +28,7 @@
 import gcc.unwind.pe;
 import gcc.builtins;
 import gcc.config;
+import gcc.attribute;
 
 extern(C)
 {
@@ -519,10 +520,19 @@
     terminate("unwind error", __LINE__);
 }
 
+static if (GNU_ARM_EABI_Unwinder)
+{
+    enum personality_fn_attributes = attribute("target", ("general-regs-only"));
+}
+else
+{
+    enum personality_fn_attributes = "";
+}
 
 /**
  * Read and extract information from the LSDA (.gcc_except_table section).
  */
+@personality_fn_attributes
 _Unwind_Reason_Code scanLSDA(const(ubyte)* lsda, _Unwind_Exception_Class exceptionClass,
                              _Unwind_Action actions, _Unwind_Exception* unwindHeader,
                              _Unwind_Context* context, _Unwind_Word cfa,
@@ -772,6 +782,7 @@
  * Called when the personality function has found neither a cleanup or handler.
  * To support ARM EABI personality routines, that must also unwind the stack.
  */
+@personality_fn_attributes
 _Unwind_Reason_Code CONTINUE_UNWINDING(_Unwind_Exception* unwindHeader, _Unwind_Context* context)
 {
     static if (GNU_ARM_EABI_Unwinder)
@@ -814,6 +825,7 @@
 static if (GNU_ARM_EABI_Unwinder)
 {
     pragma(mangle, PERSONALITY_FUNCTION)
+    @personality_fn_attributes
     extern(C) _Unwind_Reason_Code gdc_personality(_Unwind_State state,
                                                   _Unwind_Exception* unwindHeader,
                                                   _Unwind_Context* context)
@@ -873,6 +885,7 @@
     }
 }
 
+@personality_fn_attributes
 private _Unwind_Reason_Code __gdc_personality(_Unwind_Action actions,
                                               _Unwind_Exception_Class exceptionClass,
                                               _Unwind_Exception* unwindHeader,
Comment 65 Jakub Jelinek 2019-04-18 15:01:41 UTC
Created attachment 46198 [details]
gcc9-pr89093.patch

So, can we converge to a single patch that does everything?  Attached is completely untested compilation of Ramana's above patch, the above discussions and some cleanups/fixes, but I don't have cycles to actually verify it and test it.
Comment 66 Bernd Edlinger 2019-04-18 15:41:04 UTC
(In reply to Jakub Jelinek from comment #65)
> Created attachment 46198 [details]
> gcc9-pr89093.patch
> 
> So, can we converge to a single patch that does everything?  Attached is
> completely untested compilation of Ramana's above patch, the above
> discussions and some cleanups/fixes, but I don't have cycles to actually
> verify it and test it.

Thanks, it misses the go and obj-c personality functions.
I can quickly add that (and what I mentioned above regarding
defining TARGET_HARD_FLOAT as TARGET_HARD_FLOAT_SUB && !TARGET_GENERAL_REGS_ONLY.

I can do a native armhf boot/reg-test but that will take 4-5 days.
Comment 67 Bernd Edlinger 2019-04-18 16:53:03 UTC
Created attachment 46200 [details]
updated patch

So, that is what I am going to bootstrap now.
Adds a libgo patch and some minor changes, mostly where to
put the target-attribute (immediately before the function name).
I assume we can apply that to libgo when its ready.
(I wrote a change log entry but actually there is no libgo/ChangeLog)
Comment 68 Jakub Jelinek 2019-04-18 18:50:21 UTC
Comment on attachment 46200 [details]
updated patch

I believe the second hunk in libgo/runtime/go-unwind.c is incorrect, that is on code not guarded with #ifdef __ARM_EABI_UNWINDER__, so that will break all other targets.
I can do a distro build which includes --enable-checking=release bootstrap/regtest within ~ 24 hours or so, but all that is done without me having easy access to it, so if it works, fine, if it doesn't, I'll just know it does not.
Comment 69 Jakub Jelinek 2019-04-18 18:58:02 UTC
Created attachment 46202 [details]
gcc9-pr89093.patch

So, here is the above patch updated in the one libgo hunk.
Comment 70 Bernd Edlinger 2019-04-18 19:10:36 UTC
Yes, thanks, now switching to your latest patch.
Comment 71 Bernd Edlinger 2019-04-19 05:44:04 UTC
I am sorry, but my native arm bootstrap Fails:

g++ -std=gnu++98 -fno-PIE -c  -I../../gcc-trunk-r270444/gcc/../libgcc -DEH_MECHANISM_arm -DIN_GCC_FRONTEND -g -DIN_GCC     -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-format  -Wmissing-format-attribute -Woverloaded-virtual -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -I. -Iada -I../../gcc-trunk-r270444/gcc -I../../gcc-trunk-r270444/gcc/ada -I../../gcc-trunk-r270444/gcc/../include -I../../gcc-trunk-r270444/gcc/../libcpp/include -I/home/ed/gnu/gcc-build-arm-linux-gnueabihf/./gmp -I/home/ed/gnu/gcc-trunk-r270444/gmp -I/home/ed/gnu/gcc-build-arm-linux-gnueabihf/./mpfr/src -I/home/ed/gnu/gcc-trunk-r270444/mpfr/src -I/home/ed/gnu/gcc-trunk-r270444/mpc/src  -I../../gcc-trunk-r270444/gcc/../libdecnumber -I../../gcc-trunk-r270444/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc-trunk-r270444/gcc/../libbacktrace -I/home/ed/gnu/gcc-build-arm-linux-gnueabihf/./isl/include -I/home/ed/gnu/gcc-trunk-r270444/isl/include  -o ada/raise-gcc.o -MT ada/raise-gcc.o -MMD -MP -MF ada/.deps/raise-gcc.TPo ../../gcc-trunk-r270444/gcc/ada/raise-gcc.c
g++ -std=gnu++98 -fno-PIE -c  -DIN_GCC_FRONTEND -g -DIN_GCC     -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-format  -Wmissing-format-attribute -Woverloaded-virtual -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -I. -Iada -I../../gcc-trunk-r270444/gcc -I../../gcc-trunk-r270444/gcc/ada -I../../gcc-trunk-r270444/gcc/../include -I../../gcc-trunk-r270444/gcc/../libcpp/include -I/home/ed/gnu/gcc-build-arm-linux-gnueabihf/./gmp -I/home/ed/gnu/gcc-trunk-r270444/gmp -I/home/ed/gnu/gcc-build-arm-linux-gnueabihf/./mpfr/src -I/home/ed/gnu/gcc-trunk-r270444/mpfr/src -I/home/ed/gnu/gcc-trunk-r270444/mpc/src  -I../../gcc-trunk-r270444/gcc/../libdecnumber -I../../gcc-trunk-r270444/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc-trunk-r270444/gcc/../libbacktrace -I/home/ed/gnu/gcc-build-arm-linux-gnueabihf/./isl/include -I/home/ed/gnu/gcc-trunk-r270444/isl/include  -o ada/seh_init.o -MT ada/seh_init.o -MMD -MP -MF ada/.deps/seh_init.TPo ../../gcc-trunk-r270444/gcc/ada/seh_init.c
../../gcc-trunk-r270444/gcc/ada/raise-gcc.c:1165:55: error: unknown target attribute or pragma 'general-regs-only'
 1165 |    struct _Unwind_Context* uw_context ATTRIBUTE_UNUSED)
      |                                                       ^
../../gcc-trunk-r270444/gcc/ada/raise-gcc.c:1183:32: error: unknown target attribute or pragma 'general-regs-only'
 1183 |     _Unwind_Context *uw_context)
      |                                ^
../../gcc-trunk-r270444/gcc/ada/raise-gcc.c:1354:43: error: unknown target attribute or pragma 'general-regs-only'
 1354 |         struct _Unwind_Context* uw_context)
      |                                           ^
make[3]: *** [ada/raise-gcc.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: Leaving directory `/home/ed/gnu/gcc-build-arm-linux-gnueabihf/gcc'
make[2]: *** [all-stage1-gcc] Error 2
make[2]: Leaving directory `/home/ed/gnu/gcc-build-arm-linux-gnueabihf'
make[1]: *** [stage1-bubble] Error 2
make[1]: Leaving directory `/home/ed/gnu/gcc-build-arm-linux-gnueabihf'
make: *** [all] Error 2
Comment 72 Bernd Edlinger 2019-04-19 05:54:02 UTC
I use host Compiler from last week:
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/home/ed/gnu/arm-linux-gnueabihf/libexec/gcc/armv7l-unknown-linux-gnueabihf/9.0.1/lto-wrapper
Target: armv7l-unknown-linux-gnueabihf
Configured with: ../gcc-9-20190331/configure --prefix=/home/ed/gnu/arm-linux-gnueabihf --enable-languages=all --with-arch=armv7-a --with-tune=cortex-a9 --with-fpu=vfpv3-d16 --with-float=hard
Thread model: posix
gcc version 9.0.1 20190331 (experimental) (GCC)

and libgnat, is definietly affected since the personality function pushes d8-d9
Comment 73 Bernd Edlinger 2019-04-19 06:05:46 UTC
Okay, the requirement is only to be able to boot-strap with
a released gcc version, so gcc-8 should not use the pragma,
while gcc-9 should use the pagma.
I was able to bootstrap from x86_64 -> arm cross -> arm native
that worked fine.
I will guard the pragma with a gcc-version check.
Comment 74 Bernd Edlinger 2019-04-19 06:22:14 UTC
Created attachment 46203 [details]
gcc9-pr89093.patch

Same patch, just guard the target attribute with a version check,
so that supported boot-strap configurations should work
Comment 75 Jakub Jelinek 2019-04-19 08:03:52 UTC
It failed for me as well.  And a GCC version check won't really help when using earlier GCC 9 snapshot as system compiler (though, admittedly that isn't supported).
Another option would be to define the attribute only #ifdef IN_RTS, because when it is #ifdef IN_GCC_FRONTEND, it is compiled with -fno-exceptions and so won't be really invoked.
Comment 76 Jakub Jelinek 2019-04-19 09:06:53 UTC
Created attachment 46205 [details]
gcc9-pr89093.patch

Updated patch.
Comment 77 Jakub Jelinek 2019-04-22 08:49:50 UTC
Created attachment 46220 [details]
gcc9-pr89093.patch

The above patch passed bootstrap, but there is a regression:
+FAIL: compiler driver --help=target option(s): "^ +-.*[^:.]\$" absent from output: "  -mgeneral-regs-only         Generate code which uses the cor
e registers only(r0-r14)"
because there is no full stop at the end of description.  Fixed in this patch (I've also added a space before (.

I've also seen
+FAIL: TestAbort
+FAIL: TestBreakpoint
+FAIL: TestCgoCrashHandler
+FAIL: TestCgoSignalDeadlock
+FAIL: TestSelectStackAdjust
regressions, but 1) not sure if they are reproducible or not, go has usually lots of random intermitent failures 2) even if yes, the libgo part of the patch I believe can't be committed together with the rest, as libgo has a different upstream and will need to guard it on being compiled with gcc 9+, because it can be built with other compilers too.
Comment 78 Jakub Jelinek 2019-04-23 10:04:12 UTC
Author: jakub
Date: Tue Apr 23 10:03:41 2019
New Revision: 270504

URL: https://gcc.gnu.org/viewcvs?rev=270504&root=gcc&view=rev
Log:
	PR target/89093
	* config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate): Diagnose
	if used with general-regs-only.
	(arm_conditional_register_usage): Don't add non-general regs if
	general-regs-only.
	(arm_valid_target_attribute_rec): Handle general-regs-only.
	* config/arm/arm.h (TARGET_HARD_FLOAT): Return false if
	general-regs-only.
	(TARGET_HARD_FLOAT_SUB): Define.
	(TARGET_SOFT_FLOAT): Define as negation of TARGET_HARD_FLOAT_SUB.
	(TARGET_REALLY_IWMMXT): Add && !TARGET_GENERAL_REGS_ONLY.
	(TARGET_REALLY_IWMMXT2): Likewise.
	* config/arm/arm.opt: Add -mgeneral-regs-only.
	* doc/extend.texi: Document ARM general-regs-only target.
	* doc/invoke.texi: Document ARM -mgeneral-regs-only.
libgcc/
	* config/arm/pr-support.c: Add #pragma GCC target("general-regs-only").
	* config/arm/unwind-arm.c: Likewise.
	* unwind-c.c (PERSONALITY_FUNCTION): Add general-regs-only target
	attribute for ARM.
libobjc/
	* exception.c (PERSONALITY_FUNCTION): Add general-regs-only target
	attribute for ARM.
libphobos/
	* libdruntime/gcc/deh.d: Import gcc.attribute.
	(personality_fn_attributes): New enum.
	(scanLSDA, CONTINUE_UNWINDING, gdc_personality, __gdc_personality):
	Add @personality_fn_attributes.
libstdc++-v3/
	* libsupc++/eh_personality.cc (PERSONALITY_FUNCTION): Add
	general-regs-only target attribute for ARM.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/config/arm/arm.h
    trunk/gcc/config/arm/arm.opt
    trunk/gcc/doc/extend.texi
    trunk/gcc/doc/invoke.texi
    trunk/libgcc/ChangeLog
    trunk/libgcc/config/arm/pr-support.c
    trunk/libgcc/config/arm/unwind-arm.c
    trunk/libgcc/unwind-c.c
    trunk/libobjc/ChangeLog
    trunk/libobjc/exception.c
    trunk/libphobos/ChangeLog
    trunk/libphobos/libdruntime/gcc/deh.d
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/libsupc++/eh_personality.cc
Comment 79 Jakub Jelinek 2019-04-23 10:10:25 UTC
Fixed for all languages but Ada and Go, neither of them is release critical.
Comment 80 Jakub Jelinek 2019-04-24 08:16:38 UTC
Author: jakub
Date: Wed Apr 24 08:16:07 2019
New Revision: 270535

URL: https://gcc.gnu.org/viewcvs?rev=270535&root=gcc&view=rev
Log:
	PR target/89093
	* raise-gcc.c (TARGET_ATTRIBUTE): Define.
	(continue_unwind, personality_body, PERSONALITY_FUNCTION): Add
	TARGET_ATTRIBUTE.

Modified:
    trunk/gcc/ada/ChangeLog
    trunk/gcc/ada/raise-gcc.c
Comment 81 Jakub Jelinek 2019-04-24 08:30:28 UTC
Fixed for Ada as well, only Go left to do.
Comment 82 ian@gcc.gnu.org 2019-04-24 12:46:17 UTC
Author: ian
Date: Wed Apr 24 12:45:45 2019
New Revision: 270542

URL: https://gcc.gnu.org/viewcvs?rev=270542&root=gcc&view=rev
Log:
	PR target/89093
    runtime: mark unwind functions general-regs-only on ARM
    
    For https://gcc.gnu.org/PR89093.
    
    Change-Id: Ic426b43d633c77104bda01d4e7835bc9ab4695ef
    Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/173657
    Reviewed-by: Ian Lance Taylor <iant@golang.org>


Modified:
    trunk/gcc/go/gofrontend/MERGE
    trunk/libgo/runtime/go-unwind.c
Comment 83 Jakub Jelinek 2019-04-24 13:37:14 UTC
Fixed then.
Comment 84 Jakub Jelinek 2019-04-30 12:07:59 UTC
Author: jakub
Date: Tue Apr 30 12:07:27 2019
New Revision: 270690

URL: https://gcc.gnu.org/viewcvs?rev=270690&root=gcc&view=rev
Log:
	PR target/89093
	* config/aarch64/aarch64.c (aarch64_process_one_target_attr): Don't skip
	whitespace at the start of target attribute string.

	* gcc.target/aarch64/pr89093.c: New test.
	* gcc.target/aarch64/pr63304_1.c: Remove space from target string.

Added:
    trunk/gcc/testsuite/gcc.target/aarch64/pr89093.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/aarch64/pr63304_1.c
Comment 85 Tamar Christina 2019-04-30 16:24:17 UTC
Hi Jakub,

commit r270690 seems to have caused a regression on gcc.target/aarch64/return_address_sign_3.c
Comment 86 Tamar Christina 2019-04-30 16:25:42 UTC
for aarch64-none-linux-gnu. I am still building the toolchain to take a look so not able to give more detail.
Comment 87 Jakub Jelinek 2019-04-30 16:31:16 UTC
Author: jakub
Date: Tue Apr 30 16:30:44 2019
New Revision: 270705

URL: https://gcc.gnu.org/viewcvs?rev=270705&root=gcc&view=rev
Log:
	PR target/89093
	* gcc.target/aarch64/return_address_sign_3.c: Remove extra space in
	target attribute.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/aarch64/return_address_sign_3.c
Comment 88 Tamar Christina 2019-05-01 12:46:00 UTC
Thanks Jakub!