Patchwork [U-Boot,v2,4/9] arm: add _thumb1_case_uqi to libgcc

login
register
mail settings
Submitter Allen Martin
Date Aug. 1, 2012, 8:32 p.m.
Message ID <1343853146-15498-5-git-send-email-amartin@nvidia.com>
Download mbox | patch
Permalink /patch/174594/
State Superseded
Headers show

Comments

Allen Martin - Aug. 1, 2012, 8:32 p.m.
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
Stephen Warren - Aug. 13, 2012, 11:44 p.m.
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).
Allen Martin - Aug. 14, 2012, 12:36 a.m.
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
Aneesh V - Aug. 14, 2012, 2:39 a.m.
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
Stephen Warren - Aug. 14, 2012, 4:02 p.m.
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)

Patch

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
+