Bug 236920

Summary: Bad asm constraints in arm64 byte swap functions
Product: Base System Reporter: John F. Carr <jfc>
Component: armAssignee: Mitchell Horne <mhorne>
Status: Closed FIXED    
Severity: Affects Only Me CC: Andrew, emaste, mhorne
Priority: --- Keywords: patch
Version: CURRENT   
Hardware: arm64   
OS: Any   
Attachments:
Description Flags
Fix inline assembly none

Description John F. Carr 2019-04-01 00:04:10 UTC
Created attachment 203277 [details]
Fix inline assembly

The bswap inline functions in sys/arm64/include/endian.h look wrong.

The ARM byte swap instructions are ordinary RISC register to register instructions and should have "=r" as the constraint on the output operand and "r" as the constraint on the input operand.

A message to the freebsd-arm list mentioned an error message "invalid operand in inline asm" associated with one of the inline assembly statements in endian.h.  This could have been caused by the strange constraints on the inline assembly.

I've tested that buildworld works with the attached patch.  I have only run a couple network utilities as a smoke test.
Comment 1 John F. Carr 2019-04-04 12:37:47 UTC
The chromium port problem mentioned on the mailing list turns out to be unrelated.  The port was compiling for ARM v7 which doesn't have rev instructions.  I still think the constraints are wrong.
Comment 2 commit-hook freebsd_committer 2021-03-26 22:02:39 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=720dc6bcb5a8c4283802576e2ef54f42b33fa8d4

commit 720dc6bcb5a8c4283802576e2ef54f42b33fa8d4
Author:     Mitchell Horne <mhorne@FreeBSD.org>
AuthorDate: 2021-03-01 15:07:54 +0000
Commit:     Mitchell Horne <mhorne@FreeBSD.org>
CommitDate: 2021-03-26 22:00:22 +0000

    Consolidate machine/endian.h definitions

    This change serves two purposes.

    First, we take advantage of the compiler provided endian definitions to
    eliminate some long-standing duplication between the different versions
    of this header. __BYTE_ORDER__ has been defined since GCC 4.6, so there
    is no need to rely on platform defaults or e.g. __MIPSEB__ to determine
    endianness. A new common sub-header is added, but there should be no
    changes to the visibility of these definitions.

    Second, this eliminates the hand-rolled __bswapNN() routines, again in
    favor of the compiler builtins. This was done already for x86 in
    e6ff6154d203. The benefit here is that we no longer have to maintain our
    own implementations on each arch, and can instead rely on the compiler
    to emit appropriate instructions or libcalls, as available. This should
    result in equivalent or better code generation. Notably 32-bit arm will
    start using the `rev` instruction for these routines, which is available
    on armv6+.

    PR:             236920
    Reviewed by:    arichardson, imp
    Tested by:      bdragon (BE powerpc)
    MFC after:      3 weeks
    Differential Revision:  https://reviews.freebsd.org/D29012

 sys/arm/include/endian.h     | 109 ++-----------------------------------------
 sys/arm64/include/endian.h   |  85 +--------------------------------
 sys/mips/include/endian.h    | 106 +----------------------------------------
 sys/powerpc/include/endian.h | 102 +---------------------------------------
 sys/riscv/include/endian.h   |  87 +---------------------------------
 sys/sys/_endian.h (new)      |  92 ++++++++++++++++++++++++++++++++++++
 sys/x86/include/endian.h     |  38 +--------------
 7 files changed, 101 insertions(+), 518 deletions(-)
Comment 3 Mitchell Horne freebsd_committer 2021-03-27 14:33:55 UTC
(In reply to John F. Carr from comment #0)

I agree with your analysis, and I found that your patch saved one instruction for each call to these routines.

The committed fix looks quite different, because I chose to eliminate FreeBSD's many implementations of these functions in favor of the compiler's built-in versions. Still, I only discovered the potential for this cleanup because of your patch, so thank you.
Comment 4 commit-hook freebsd_committer 2021-06-24 23:44:36 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9d61599983dbc3e43a4eb5b0e87276afbc1be257

commit 9d61599983dbc3e43a4eb5b0e87276afbc1be257
Author:     Mitchell Horne <mhorne@FreeBSD.org>
AuthorDate: 2021-03-01 15:07:54 +0000
Commit:     Mitchell Horne <mhorne@FreeBSD.org>
CommitDate: 2021-06-24 23:42:56 +0000

    Consolidate machine/endian.h definitions

    This change serves two purposes.

    First, we take advantage of the compiler provided endian definitions to
    eliminate some long-standing duplication between the different versions
    of this header. __BYTE_ORDER__ has been defined since GCC 4.6, so there
    is no need to rely on platform defaults or e.g. __MIPSEB__ to determine
    endianness. A new common sub-header is added, but there should be no
    changes to the visibility of these definitions.

    Second, this eliminates the hand-rolled __bswapNN() routines, again in
    favor of the compiler builtins. This was done already for x86 in
    e6ff6154d203. The benefit here is that we no longer have to maintain our
    own implementations on each arch, and can instead rely on the compiler
    to emit appropriate instructions or libcalls, as available. This should
    result in equivalent or better code generation. Notably 32-bit arm will
    start using the `rev` instruction for these routines, which is available
    on armv6+.

    PR:             236920
    Reviewed by:    arichardson, imp
    Tested by:      bdragon (BE powerpc)
    MFC after:      3 weeks
    Differential Revision:  https://reviews.freebsd.org/D29012

    (cherry picked from commit 720dc6bcb5a8c4283802576e2ef54f42b33fa8d4)

 sys/arm/include/endian.h     | 109 ++-----------------------------------------
 sys/arm64/include/endian.h   |  85 +--------------------------------
 sys/mips/include/endian.h    | 106 +----------------------------------------
 sys/powerpc/include/endian.h | 102 +---------------------------------------
 sys/riscv/include/endian.h   |  87 +---------------------------------
 sys/sys/_endian.h (new)      |  92 ++++++++++++++++++++++++++++++++++++
 sys/x86/include/endian.h     |  38 +--------------
 7 files changed, 101 insertions(+), 518 deletions(-)