Re: [PATCH 1/1] ArmPkg/CompilerIntrinsicsLib: Add lasr assembly source for gcc/arm

["Pete Batard" <pete@akeo.ie>]

Hi Leif,

On 2019.05.07 17:52, Leif Lindholm wrote:
> On Tue, May 07, 2019 at 04:57:26PM +0100, Pete Batard wrote:
>> While compiling drivers or applications on Debian 9.8, using the native
>> gcc 6.3.0 ARM compiler, one may run into the following build failure:
>>
>>    "undefined reference to `__aeabi_lasr'"
>>
>> This patch adds the missing definition, through a port of the existing
>> RVCT lasr.asm source.
> 
> Ugh. Here's a module that could do with a bit of love.
> ArmPkg/Library/CompilerIntrinsicsLib/Arm/switch.asm,
> ArmPkg/Library/CompilerIntrinsicsLib/Arm/uread.asm and
> ArmPkg/Library/CompilerIntrinsicsLib/Arm/uwrite.asm
> also all miss .S versions.

Well, I would assume the switch.asm counterparts are provided in the 
various switch##.S.

On the other hand, since I'm half expecting to run into undefined refs 
with regards to __aeabi_uread# and __aeabi_uwrite# sooner rather than 
later, I will take a stab at uread/uwrite.

> And then there is an ominous comment in the .inf
> "# Move .c to .s to work around LLVM issues"
> and it appears all of the .c files are actually unused.

Okay. Unless someone objects, I'll remove these unused files as part of 
a v2 proposal.

> I'm not going to try to make you fix those too, but if you're feeling
> charitable...
> 
> One comment below.
> 
>> CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> CC: Leif Lindholm <leif.lindholm@linaro.org>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   ArmPkg/Library/CompilerIntrinsicsLib/Arm/lasr.S                | 22 ++++++++++++++++++++
>>   ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf |  1 +
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/lasr.S b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/lasr.S
>> new file mode 100644
>> index 000000000000..9ded045b8bc4
>> --- /dev/null
>> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/lasr.S
>> @@ -0,0 +1,22 @@
>> +#------------------------------------------------------------------------------
>> +#
>> +# Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +#------------------------------------------------------------------------------
>> +
>> +#include <AsmMacroIoLib.h>
>> +
> 
> Could you add a "prototype" here?
> The one in the original is nonsense, but
> something like

Will do that too.

Regards,

/Pete

> 
> #
> #INT64
> #EFIAPI
> #__aeabi_lasr (
> #  IN INT64 Value,
> #  IN INT32 Shift
> #  );
> #
> 
> ?
> 
> /
>      Leif
> 
>> +ASM_FUNC(__aeabi_lasr)
>> +  subs     r3,r2,#0x20
>> +  bpl      L_Test
>> +  rsb      r3,r2,#0x20
>> +  lsr      r0,r0,r2
>> +  orr      r0,r0,r1,LSL r3
>> +  asr      r1,r1,r2
>> +  bx       lr
>> +L_Test:
>> +  asr      r0,r1,r3
>> +  asr      r1,r1,#31
>> +  bx       lr
>> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> index 0d5634959993..7cb8f194f210 100644
>> --- a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> @@ -90,6 +90,7 @@ [Sources.ARM]
>>     Arm/uldiv.S      | GCC
>>     Arm/ldivmod.S    | GCC
>>   
>> +  Arm/lasr.S       | GCC
>>     Arm/llsr.S       | GCC
>>     Arm/llsl.S       | GCC
>>   
>> -- 
>> 2.21.0.windows.1
>>