| Message ID | 1343853146-15498-5-git-send-email-amartin@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
On 08/01/2012 02:32 PM, Allen Martin wrote: > Add function required by some thumb switch statements > diff --git a/arch/arm/lib/_thumb1_case_uqi.S b/arch/arm/lib/_thumb1_case_uqi.S > + .force_thumb I believe that line should be removed. The issue here is that when gcc emits Thumb code to call this function, it actually emits: > 108af8: f000 f94a bl 108d90 <____gnu_thumb1_case_uqi_from_thumb> which is implemented as: > 00108d90 <____gnu_thumb1_case_uqi_from_thumb>: > 108d90: 4778 bx pc > 108d92: 46c0 nop ; (mov r8, r8) > 108d94: eafffde1 b 108520 <__gnu_thumb1_case_uqi> i.e. it switches to ARM mode then jumps to that function. Hence, __gnu_thumb1_case_uqi must be compiled as ARM, not as Thumb. (renaming the function to ____gnu_thumb1_case_uqi_from_thumb in the hope it'll be called directly instead of going through a stub doesn't seem to work) If I make that change, then this patch series starts working on Whistler, which for reference, uses UARTA, so triggers funcmux.c's funcmux_select() to enter case PERIPH_ID_UART1, which then uses ____gnu_thumb1_case_uqi_from_thumb to perform the nested switch (config).
On Mon, Aug 13, 2012 at 04:44:05PM -0700, Stephen Warren wrote: > On 08/01/2012 02:32 PM, Allen Martin wrote: > > Add function required by some thumb switch statements > > > diff --git a/arch/arm/lib/_thumb1_case_uqi.S b/arch/arm/lib/_thumb1_case_uqi.S > > > + .force_thumb > > I believe that line should be removed. > > The issue here is that when gcc emits Thumb code to call this function, > it actually emits: > > > 108af8: f000 f94a bl 108d90 <____gnu_thumb1_case_uqi_from_thumb> > > which is implemented as: > > > 00108d90 <____gnu_thumb1_case_uqi_from_thumb>: > > 108d90: 4778 bx pc > > 108d92: 46c0 nop ; (mov r8, r8) > > 108d94: eafffde1 b 108520 <__gnu_thumb1_case_uqi> > > i.e. it switches to ARM mode then jumps to that function. Hence, > __gnu_thumb1_case_uqi must be compiled as ARM, not as Thumb. The function is supposed to be thumb code, it's basically a thumb optimization of a switch statement, and in the ARM case the compiler just emits the code directly instead of calling into libgcc. So it doesn't really make sense for the function to be anything but thumb. I think the real problem is the linker incorrectly thought the code was ARM so it generated a thumb to ARM interworking veneer. Can you try adding a ".type __gnu_thumb1_case_uqi STT_FUNC" instead? I've seen cases where the assembler generates the wrong type of symbol without some explicit guidance which messes up the linker's interworking generator. This might be one of those. > > (renaming the function to ____gnu_thumb1_case_uqi_from_thumb in the hope > it'll be called directly instead of going through a stub doesn't seem to > work) > > If I make that change, then this patch series starts working on > Whistler, which for reference, uses UARTA, so triggers funcmux.c's > funcmux_select() to enter case PERIPH_ID_UART1, which then uses > ____gnu_thumb1_case_uqi_from_thumb to perform the nested switch (config). -Allen
On Mon, Aug 13, 2012 at 5:36 PM, Allen Martin <amartin@nvidia.com> wrote: > On Mon, Aug 13, 2012 at 04:44:05PM -0700, Stephen Warren wrote: >> On 08/01/2012 02:32 PM, Allen Martin wrote: >> > Add function required by some thumb switch statements >> >> > diff --git a/arch/arm/lib/_thumb1_case_uqi.S b/arch/arm/lib/_thumb1_case_uqi.S >> >> > + .force_thumb >> >> I believe that line should be removed. >> >> The issue here is that when gcc emits Thumb code to call this function, >> it actually emits: >> >> > 108af8: f000 f94a bl 108d90 <____gnu_thumb1_case_uqi_from_thumb> >> >> which is implemented as: >> >> > 00108d90 <____gnu_thumb1_case_uqi_from_thumb>: >> > 108d90: 4778 bx pc >> > 108d92: 46c0 nop ; (mov r8, r8) >> > 108d94: eafffde1 b 108520 <__gnu_thumb1_case_uqi> >> >> i.e. it switches to ARM mode then jumps to that function. Hence, >> __gnu_thumb1_case_uqi must be compiled as ARM, not as Thumb. > > The function is supposed to be thumb code, it's basically a thumb > optimization of a switch statement, and in the ARM case the compiler > just emits the code directly instead of calling into libgcc. So it > doesn't really make sense for the function to be anything but thumb. > > I think the real problem is the linker incorrectly thought the code > was ARM so it generated a thumb to ARM interworking veneer. > > Can you try adding a ".type __gnu_thumb1_case_uqi STT_FUNC" instead? > I've seen cases where the assembler generates the wrong type of symbol > without some explicit guidance which messes up the linker's > interworking generator. This might be one of those. Yes, not having STT_FUNC or %function is troublesome. I have faced this before. Maybe you should wrap the assembly functions with standard ENTRY, END_PROC macros from linkage.h. This will take care of STT_FUNC best regards, Aneesh
On 08/13/2012 08:39 PM, V, Aneesh wrote: > On Mon, Aug 13, 2012 at 5:36 PM, Allen Martin <amartin@nvidia.com> wrote: >> On Mon, Aug 13, 2012 at 04:44:05PM -0700, Stephen Warren wrote: >>> On 08/01/2012 02:32 PM, Allen Martin wrote: >>>> Add function required by some thumb switch statements >>> >>>> diff --git a/arch/arm/lib/_thumb1_case_uqi.S b/arch/arm/lib/_thumb1_case_uqi.S >>> >>>> + .force_thumb >>> >>> I believe that line should be removed. >>> >>> The issue here is that when gcc emits Thumb code to call this function, >>> it actually emits: >>> >>>> 108af8: f000 f94a bl 108d90 <____gnu_thumb1_case_uqi_from_thumb> >>> >>> which is implemented as: >>> >>>> 00108d90 <____gnu_thumb1_case_uqi_from_thumb>: >>>> 108d90: 4778 bx pc >>>> 108d92: 46c0 nop ; (mov r8, r8) >>>> 108d94: eafffde1 b 108520 <__gnu_thumb1_case_uqi> >>> >>> i.e. it switches to ARM mode then jumps to that function. Hence, >>> __gnu_thumb1_case_uqi must be compiled as ARM, not as Thumb. >> >> The function is supposed to be thumb code, it's basically a thumb >> optimization of a switch statement, and in the ARM case the compiler >> just emits the code directly instead of calling into libgcc. So it >> doesn't really make sense for the function to be anything but thumb. >> >> I think the real problem is the linker incorrectly thought the code >> was ARM so it generated a thumb to ARM interworking veneer. >> >> Can you try adding a ".type __gnu_thumb1_case_uqi STT_FUNC" instead? >> I've seen cases where the assembler generates the wrong type of symbol >> without some explicit guidance which messes up the linker's >> interworking generator. This might be one of those. > > Yes, not having STT_FUNC or %function is troublesome. I have faced this > before. > > Maybe you should wrap the assembly functions with standard ENTRY, > END_PROC macros from linkage.h. This will take care of STT_FUNC Yes. In fact patch 7/9 of this series is doing exactly that for some other assembly function. So, the patch below fixes this problem; I see the function being called directly without any compiler-synthesized "from_thumb" stub/trampoline being used. > diff --git a/arch/arm/lib/_thumb1_case_uqi.S b/arch/arm/lib/_thumb1_case_uqi.S > index e4bb194..a4241f6 100644 > --- a/arch/arm/lib/_thumb1_case_uqi.S > +++ b/arch/arm/lib/_thumb1_case_uqi.S > @@ -25,10 +25,11 @@ along with this program; see the file COPYING. If not, write to > the Free Software Foundation, 51 Franklin Street, Fifth Floor, > Boston, MA 02110-1301, USA. */ > > +#include <linux/linkage.h> > + > .force_thumb > .syntax unified > - .globl __gnu_thumb1_case_uqi > -__gnu_thumb1_case_uqi: > +ENTRY(__gnu_thumb1_case_uqi) > push {r1} > mov r1, lr > lsrs r1, r1, #1 > @@ -38,4 +39,4 @@ __gnu_thumb1_case_uqi: > add lr, lr, r1 > pop {r1} > bx lr > - > +ENDPROC(__gnu_thumb1_case_uqi)
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index bd3b77f..a54f831 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -33,6 +33,7 @@ GLSOBJS += _lshrdi3.o GLSOBJS += _modsi3.o GLSOBJS += _udivsi3.o GLSOBJS += _umodsi3.o +GLSOBJS += _thumb1_case_uqi.o GLCOBJS += div0.o diff --git a/arch/arm/lib/_thumb1_case_uqi.S b/arch/arm/lib/_thumb1_case_uqi.S new file mode 100644 index 0000000..e4bb194 --- /dev/null +++ b/arch/arm/lib/_thumb1_case_uqi.S @@ -0,0 +1,41 @@ +/* Copyright 1995, 1996, 1998, 1999, 2000, 2003, 2004, 2005 + Free Software Foundation, Inc. + +This file is free software; you can redistribute it and/or modify it +under the terms of the GNU General Public License as published by the +Free Software Foundation; either version 2, or (at your option) any +later version. + +In addition to the permissions in the GNU General Public License, the +Free Software Foundation gives you unlimited permission to link the +compiled version of this file into combinations with other programs, +and to distribute those combinations without any restriction coming +from the use of this file. (The General Public License restrictions +do apply in other respects; for example, they cover modification of +the file, and distribution when not linked into a combine +executable.) + +This file is distributed in the hope that it will be useful, but +WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program; see the file COPYING. If not, write to +the Free Software Foundation, 51 Franklin Street, Fifth Floor, +Boston, MA 02110-1301, USA. */ + + .force_thumb + .syntax unified + .globl __gnu_thumb1_case_uqi +__gnu_thumb1_case_uqi: + push {r1} + mov r1, lr + lsrs r1, r1, #1 + lsls r1, r1, #1 + ldrb r1, [r1, r0] + lsls r1, r1, #1 + add lr, lr, r1 + pop {r1} + bx lr +
Add function required by some thumb switch statements Signed-off-by: Allen Martin <amartin@nvidia.com> --- arch/arm/lib/Makefile | 1 + arch/arm/lib/_thumb1_case_uqi.S | 41 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 arch/arm/lib/_thumb1_case_uqi.S