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.
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.
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(-)
(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.
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(-)