public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Pete Batard <pete@akeo.ie>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH v3 4/6] ArmPkg/Library/CompilerIntrinsicsLib: Enable VS2017/ARM builds
Date: Tue, 12 Dec 2017 19:59:49 +0000	[thread overview]
Message-ID: <CAKv+Gu9qu-JzffSXLOVVXmkmYR5HFQ6DKAo7MDkUxhnLp_asZg@mail.gmail.com> (raw)
In-Reply-To: <20171208140631.4252-5-pete@akeo.ie>

Hi Pete,

On 8 December 2017 at 14:06, Pete Batard <pete@akeo.ie> wrote:
> Introduce CRT assembly replacements for __rt_sdiv, __rt_udiv,
> __rt_udiv64, __rt_sdiv64, __rt_srsh, memcpy and memset.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtdiv.asm             | 255 ++++++++++++++++++++
>  ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtsrsh.asm            |  45 ++++
>  ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf |  13 +-
>  ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c               |  34 +++
>  ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c               |  33 +++
>  5 files changed, 378 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtdiv.asm b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtdiv.asm
> new file mode 100644
> index 000000000000..096dc6317318
> --- /dev/null
> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtdiv.asm
> @@ -0,0 +1,255 @@
> +///** @file
> +//
> +//  This code provides replacement for MSVC CRT division functions
> +//
> +//  Copyright (c) 2017, Pete Batard. All rights reserved.<BR>
> +//  Based on generated assembly of ReactOS' sdk/lib/crt/math/arm/__rt_###div.c,
> +//  Copyright (c) Timo Kreuzer. All rights reserved.<BR>
> +//
> +//  This program and the accompanying materials
> +//  are licensed and made available under the terms and conditions of the BSD License
> +//  which accompanies this distribution.  The full text of the license may be found at
> +//  http://opensource.org/licenses/bsd-license.php
> +//
> +//  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +//  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +//
> +//**/
> +
> +  EXPORT _fltused
> +  EXPORT __brkdiv0
> +

Why are these being exported? Are these part of the CRT ABI?

> +  EXPORT __rt_sdiv
> +  EXPORT __rt_udiv
> +  EXPORT __rt_udiv64
> +  EXPORT __rt_sdiv64
> +
> +  AREA  Math, CODE, READONLY
> +
> +_fltused
> +    dcd 0x9875
> +
> +__brkdiv0
> +    udf #249
> +

This will cause a crash when invoked, in a way that gives very little
to go on to figure out what happens. Perhaps branch to a C function
here that calls ASSERT() and CpuDeadLoop ?

> +//
> +// uint64_t __rt_udiv(uint32_t divisor, uint32_t dividend)
> +//

There are many different division implementations already in the same
directory. Do we really need a new one?

> +
> +__rt_udiv
> +  cmp         r0, #0
> +  beq         __brkdiv0
> +  push        {r3-r5,lr}
> +  mov         r5,r0
> +  mov         r4,r1
> +  cmp         r5,r4
> +  it          hi
> +  movhi       r0,#0
> +  bhi         __rt_udiv_label3
> +  clz         r2,r5
> +  clz         r3,r4
> +  subs        r3,r2,r3
> +  movs        r1,#1
> +  lsl         r2,r5,r3
> +  lsl         r3,r1,r3
> +  movs        r0,#0
> +__rt_udiv_label1
> +  cmp         r4,r2
> +  bcc         __rt_udiv_label2
> +  orrs        r0,r0,r3
> +  subs        r4,r4,r2
> +__rt_udiv_label2
> +  lsrs        r2,r2,#1
> +  lsrs        r3,r3,#1
> +  bne         __rt_udiv_label1
> +__rt_udiv_label3
> +  mov         r1,r4
> +  pop         {r3-r5,pc}
> +
> +//
> +// uint64_t __rt_sdiv(int32_t divisor, int32_t dividend)
> +//
> +

Does signed division return an unsigned result? And do we really need
yet another implementation?

> +__rt_sdiv
> +  cmp         r0, #0
> +  beq         __brkdiv0
> +  push        {r4-r6,lr}
> +  mov         r4,r1
> +  ands        r6,r0,#0x80000000
> +  it          ne
> +  rsbne       r4,r4,#0
> +  mov         r5,r0
> +  rsbs        r5,r5,#0
> +  cmp         r5,r4
> +  it          hi
> +  movhi       r0,#0
> +  bhi         __rt_sdiv_label3
> +  clz         r2,r5
> +  clz         r3,r4
> +  subs        r3,r2,r3
> +  movs        r1,#1
> +  lsl         r2,r5,r3
> +  lsl         r3,r1,r3
> +  movs        r0,#0
> +__rt_sdiv_label1
> +  cmp         r4,r2
> +  bcc         __rt_sdiv_label2
> +  orrs        r0,r0,r3
> +  subs        r4,r4,r2
> +__rt_sdiv_label2
> +  lsrs        r2,r2,#1
> +  lsrs        r3,r3,#1
> +  bne         __rt_sdiv_label1
> +__rt_sdiv_label3
> +  cbz         r6,__rt_sdiv_label4
> +  rsbs        r4,r4,#0
> +__rt_sdiv_label4
> +  mov         r1,r4
> +  pop         {r4-r6,pc}
> +
> +//
> +// typedef struct {
> +//   uint64_t quotient;
> +//   uint64_t modulus;
> +// } udiv64_result_t;
> +//
> +// void __rt_udiv64_internal(udiv64_result_t *result, uint64_t divisor, uint64_t dividend)
> +//
> +
> +__rt_udiv64_internal
> +  orrs        r1,r2,r3
> +  beq         __brkdiv0
> +  push        {r4-r8,lr}
> +  mov         r7,r3
> +  mov         r6,r2
> +  mov         r4,r0
> +  ldrd        r0,r5,[sp,#0x18]
> +  cmp         r7,r5
> +  bcc         __rt_udiv64_internal_label2
> +  bhi         __rt_udiv64_internal_label1
> +  cmp         r6,r0
> +  bls         __rt_udiv64_internal_label2
> +__rt_udiv64_internal_label1
> +  movs        r3,#0
> +  strd        r3,r3,[r4]
> +  b           __rt_udiv64_internal_label8
> +__rt_udiv64_internal_label2
> +  clz         r2,r7
> +  cmp         r2,#0x20
> +  bne         __rt_udiv64_internal_label3
> +  clz         r3,r6
> +  add         r2,r2,r3
> +__rt_udiv64_internal_label3
> +  clz         r1,r5 ;
> +  cmp         r1,#0x20
> +  bne         __rt_udiv64_internal_label4
> +  clz         r3,r0
> +  add         r1,r1,r3
> +__rt_udiv64_internal_label4
> +  subs        r1,r2,r1
> +  rsb         r3,r1,#0x20
> +  lsr         r3,r6,r3
> +  lsl         r2,r7,r1
> +  orrs        r2,r2,r3
> +  sub         r3,r1,#0x20
> +  lsl         r3,r6,r3
> +  orrs        r2,r2,r3
> +  lsl         r7,r6,r1
> +  sub         r3,r1,#0x20
> +  movs        r6,#1
> +  lsls        r6,r6,r3
> +  movs        r3,#1
> +  mov         lr,#0
> +  lsl         r1,r3,r1
> +  mov         r8,lr
> +__rt_udiv64_internal_label5
> +  cmp         r5,r2
> +  bcc         __rt_udiv64_internal_label7
> +  bhi         __rt_udiv64_internal_label6
> +  cmp         r0,r7
> +  bcc         __rt_udiv64_internal_label7
> +__rt_udiv64_internal_label6
> +  subs        r0,r0,r7
> +  sbcs        r5,r5,r2
> +  orr         lr,lr,r1
> +  orr         r8,r8,r6
> +__rt_udiv64_internal_label7
> +  lsls        r3,r2,#0x1F
> +  orr         r7,r3,r7,lsr #1
> +  lsls        r3,r6,#0x1F
> +  orr         r1,r3,r1,lsr #1
> +  lsrs        r6,r6,#1
> +  lsrs        r2,r2,#1
> +  orrs        r3,r1,r6
> +  bne         __rt_udiv64_internal_label5
> +  strd        lr,r8,[r4]
> +__rt_udiv64_internal_label8
> +  str         r5,[r4,#0xC]
> +  str         r0,[r4,#8]
> +  pop         {r4-r8,pc}
> +
> +//
> +// {int64_t, int64_t} __rt_sdiv64(int64_t divisor, int64_t dividend)
> +//
> +
> +__rt_sdiv64
> +  push        {r4-r6,lr}
> +  sub         sp,sp,#0x18
> +  and         r6,r1,#0x80000000
> +  movs        r4,r6
> +  mov         r5,r0
> +  beq         __rt_sdiv64_label1
> +  movs        r0,#0
> +  rsbs        r2,r2,#0
> +  sbc         r3,r0,r3
> +__rt_sdiv64_label1
> +  movs        r4,r6
> +  beq         __rt_sdiv64_label2
> +  movs        r0,#0
> +  rsbs        r5,r5,#0
> +  sbc         r1,r0,r1
> +__rt_sdiv64_label2
> +  str         r2,[sp]
> +  str         r3,[sp,#4]
> +  mov         r3,r1
> +  mov         r2,r5
> +  add         r0,sp,#8
> +  bl          __rt_udiv64_internal
> +  movs        r3,r6
> +  beq         __rt_sdiv64_label3
> +  ldrd        r3,r2,[sp,#0x10]
> +  movs        r1,#0
> +  rsbs        r3,r3,#0
> +  sbcs        r1,r1,r2
> +  b           __rt_sdiv64_label4
> +__rt_sdiv64_label3
> +  ldrd        r3,r1,[sp,#0x10]
> +__rt_sdiv64_label4
> +  mov         r2,r3
> +  ldr         r0,[sp,#8]
> +  mov         r3,r1
> +  ldr         r1,[sp,#0xC]
> +  add         sp,sp,#0x18
> +  pop         {r4-r6,pc}
> +
> +//
> +// {uint64_t, uint64_t} __rt_udiv64(uint64_t divisor, uint64_t dividend)
> +//
> +
> +__rt_udiv64
> +  push        {r4,r5,lr}
> +  sub         sp,sp,#0x1C
> +  mov         r4,r2
> +  mov         r2,r0
> +  mov         r5,r3
> +  add         r0,sp,#8
> +  mov         r3,r1
> +  str         r4,[sp]
> +  str         r5,[sp,#4]
> +  bl          __rt_udiv64_internal
> +  ldrd        r2,r3,[sp,#0x10]
> +  ldrd        r0,r1,[sp,#8]
> +  add         sp,sp,#0x1C
> +  pop         {r4,r5,pc}
> +
> +  END
> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtsrsh.asm b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtsrsh.asm
> new file mode 100644
> index 000000000000..2332dda823f2
> --- /dev/null
> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/rtsrsh.asm
> @@ -0,0 +1,45 @@
> +///** @file
> +//
> +//  This code provides replacement for MSVC CRT __rt_srsh
> +//
> +//  Copyright (c) Timo Kreuzer. All rights reserved.<BR>
> +//
> +//  This program and the accompanying materials
> +//  are licensed and made available under the terms and conditions of the BSD License
> +//  which accompanies this distribution.  The full text of the license may be found at
> +//  http://opensource.org/licenses/bsd-license.php
> +//
> +//  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +//  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +//
> +//**/
> +
> +  EXPORT  __rt_srsh
> +
> +  AREA  Math, CODE, READONLY
> +
> +//
> +//  int64_t __rt_srsh(int64_t  value, uint32_t shift);
> +//
> +

Same questions here, basically.

> +__rt_srsh
> +    rsbs r3, r2, #32
> +    bmi __rt_srsh_label1
> +    lsr r0, r0, r2
> +    lsl r3, r1, r3
> +    orr r0, r0, r3
> +    asr r1, r1, r2
> +    bx  lr
> +__rt_srsh_label1
> +    cmp r2, 64
> +    bhs __rt_srsh_label2
> +    sub r3, r2, #32
> +    asr r0, r1, r3
> +    asr r1, r1, #32
> +    bx  lr
> +__rt_srsh_label2
> +    asr r1, r1, #32
> +    mov r0, r1
> +    bx  lr
> +
> +  END
> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> index 44333141a70a..0dacc5e5117d 100644
> --- a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> @@ -23,8 +23,12 @@ [Defines]
>    LIBRARY_CLASS                  = CompilerIntrinsicsLib
>
>  [Sources]
> -  memcpy.c
> -  memset.c
> +  memcpy.c             | RVCT
> +  memcpy.c             | GCC
> +  memcpy_ms.c          | MSFT
> +  memset.c             | RVCT
> +  memset.c             | GCC
> +  memset_ms.c          | MSFT
>
>  [Sources.ARM]
>    Arm/mullu.asm        | RVCT
> @@ -94,6 +98,8 @@ [Sources.ARM]
>    Arm/llsr.S       | GCC
>    Arm/llsl.S       | GCC
>
> +  Arm/rtdiv.asm    | MSFT
> +  Arm/rtsrsh.asm   | MSFT
>
>  [Packages]
>    MdePkg/MdePkg.dec
> @@ -101,3 +107,6 @@ [Packages]
>
>  [LibraryClasses]
>
> +[BuildOptions]
> +  MSFT:*_*_ARM_CC_FLAGS = /GL-
> +  MSFT:*_*_AARCH64_CC_FLAGS = /GL-
> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c b/ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c
> new file mode 100644
> index 000000000000..90bbbb930d31
> --- /dev/null
> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c
> @@ -0,0 +1,34 @@
> +//------------------------------------------------------------------------------
> +//
> +// Copyright (c) 2017, Pete Batard. All rights reserved.<BR>
> +//
> +// This program and the accompanying materials are licensed and made
> +// available under the terms and conditions of the BSD License which
> +// accompanies this distribution.  The full text of the license may be
> +// found at http://opensource.org/licenses/bsd-license.php
> +//
> +// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +// IMPLIED.
> +//
> +//------------------------------------------------------------------------------
> +
> +#if defined(_M_ARM64)
> +typedef unsigned __int64  size_t;
> +#else
> +typedef unsigned __int32  size_t;
> +#endif
> +
> +void* memcpy(void *, const void *, size_t);
> +#pragma intrinsic(memcpy)
> +#pragma function(memcpy)
> +void* memcpy(void *dest, const void *src, size_t n)
> +{
> +  unsigned char *d = dest;
> +  unsigned char const *s = src;
> +
> +  while (n--)
> +    *d++ = *s++;
> +
> +  return dest;
> +}
> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c b/ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c
> new file mode 100644
> index 000000000000..64205e5d1012
> --- /dev/null
> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c
> @@ -0,0 +1,33 @@
> +//------------------------------------------------------------------------------
> +//
> +// Copyright (c) 2017, Pete Batard. All rights reserved.<BR>
> +//
> +// This program and the accompanying materials are licensed and made
> +// available under the terms and conditions of the BSD License which
> +// accompanies this distribution.  The full text of the license may be
> +// found at http://opensource.org/licenses/bsd-license.php
> +//
> +// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +// IMPLIED.
> +//
> +//------------------------------------------------------------------------------
> +
> +#if defined(_M_ARM64)
> +typedef unsigned __int64  size_t;
> +#else
> +typedef unsigned __int32  size_t;
> +#endif
> +
> +void* memset(void *, int, size_t);
> +#pragma intrinsic(memset)
> +#pragma function(memset)
> +void *memset(void *s, int c, size_t n)
> +{
> +  unsigned char *d = s;
> +
> +  while (n--)
> +    *d++ = (unsigned char)c;
> +
> +  return s;
> +}

These look fine to me.


  parent reply	other threads:[~2017-12-12 19:55 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-08 14:06 [PATCH v3 0/6] Add ARM support for VS2017 Pete Batard
2017-12-08 14:06 ` [PATCH v3 1/6] MdePkg: Disable some Level 4 warnings for VS2017/ARM Pete Batard
2017-12-08 14:06 ` [PATCH v3 2/6] MdePkg/Library/BaseStackCheckLib: Add Null handler " Pete Batard
2017-12-08 14:06 ` [PATCH v3 3/6] MdePkg/Library/BaseLib: Enable VS2017/ARM builds Pete Batard
2017-12-08 14:06 ` [PATCH v3 4/6] ArmPkg/Library/CompilerIntrinsicsLib: " Pete Batard
2017-12-10 13:30   ` Gao, Liming
2017-12-12 19:59   ` Ard Biesheuvel [this message]
2017-12-13 13:56     ` Pete Batard
2017-12-14 14:05       ` Ard Biesheuvel
2017-12-08 14:06 ` [PATCH v3 5/6] MdePkg/Include: Add VA list support for VS2017/ARM Pete Batard
2017-12-08 14:06 ` [PATCH v3 6/6] BaseTools/Conf: Add VS2017/ARM support Pete Batard
2017-12-08 14:13 ` [PATCH v3 0/6] Add ARM support for VS2017 Gao, Liming

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu9qu-JzffSXLOVVXmkmYR5HFQ6DKAo7MDkUxhnLp_asZg@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox