[compiler-rt][builtins] Move DMB definition to syn-ops.h

[pzheng]

Summary

Compiler-rt cross-compile for ARMv5 fails because D99282 made it an error if DMB
is used for any pre-ARMv6 targets. More specifically, the "#error only supported
on ARMv6+" added in D99282 will cause compilation to fail when any source file
which includes assembly.h are compiled for pre-ARMv6 targets. Since the only
place where DMB is used is syn-ops.h (which is only included by
arm/sync_fetch_and_* and these files are excluded from being built for older
targets), this patch moves the definition there to avoid the issues described
above.

[peter.smith]

I'm not sure that this is right. From the Armv5/v6 ARM ARM https://developer.arm.com/documentation/ddi0100/i/ I think DMB was introduced in Armv6 (see B2.6.1 DataMemoryBarrier (DMB) CP15 register 7) . Armv5 did have an operation called Drain Write Buffer DWB that was renamed in Armv6 to DSB (see DataSynchronizationBarrier (DSB) CP15 register 7). From B6.6.5 Register 7: cache management functions this is opcode 4 but I don't think DWB is exactly the same as DSB is on Armv6.

With some searching I found https://bugs.linaro.org/show_bug.cgi?id=355 which describes what the linux kernel uses in inline assembly, which is essentially #define dmb(x) __asm__ __volatile__ ("" : : : "memory")

Would you be able to provide us with a reference for how this MCR is expected to behave on Arm v5? It will compile, but I've no idea what will happen if executed, possible it will do nothing. If it is the case that it does nothing then it would be better to replace DMB with nothing on v5+. It may be that the old DWB is more appropriate on v5.

Would also be useful to know why you want to make the change? Is this just to make the code compile, or does the code have to be behave like a DMB would be executed on an Arm v5?

[efriedma]

Even if this did work, the only current use is in compiler-rt/lib/builtins/arm/sync-ops.h, and those routines definitely won't work on ARMv5. (For baremetal targets, there's simply no way to provide those routines for v5. On Linux, see https://www.kernel.org/doc/Documentation/arm/kernel_user_helpers.txt .)

[pzheng]

Thanks for digging out the references, @peter.smith. You are right, DMB is only available for ARMv6+ and what is available for ARMv5 is DWB. I overlooked some details.

The reason why I created the change was to resolve an error when compiler-rt was built for ARMv5. The build error I saw was due to D99282 making it always an error for any pre-ARMv6 target. Like what you have pointed out, DWB may not be exactly the same as DMB, so defining DMB as nothing and warn about this may be enough to unblock the build.

[pzheng]

I agree, @efriedma, the routines in sync-ops.h probably won't work as expected on ARMv5 and I doubt any of our baremetal customers actually use any of these. For now, to unblock the ARMv5 build, does it make sense to define DMB as nothing for any target pre-ARMv6? Any other suggestions are welcome too.

[pzheng]

Addressing some comments

[efriedma]

I think I'd prefer to just hack the CMake files to exclude arm/sync_fetch_and_* from any ARM target less than v7, given the routines don't actually work.

[pzheng]

Thanks for the suggestions, @efriedma. I checked the cmake files and the arm/sync_fetch_and_* files seem to have been properly excluded from unsupported archs. The build errors I saw are actually because the assembly.h header being included in many other assembly files too (e.g., aeabi_dcmp.S). After D99282, compiling any file which includes assembly.h for any pre-ARVv6 target will fail due to the "#error only supported on ARMv6+."

[pzheng]

update

[efriedma]

In that case, I see two options:

• Just skip defining "DMB" at all. And maybe rename it so "DMB" isn't accidentally interpreted as an instruction.
• Move the define into sync-ops.h.

[pzheng]

Thanks, @efriedma. I am updating the patch to move the DMB defintion to sync-ops.h.

[efriedma]

LGTM

compiler-rt/lib/builtins/arm/sync-ops.h

 //===-- sync-ops.h - --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements outline macros for the __sync_fetch_and_*
 // operations. Different instantiations will generate appropriate assembly for
 // ARM and Thumb-2 versions of the functions.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../assembly.h"
 
+#if __ARM_ARCH >= 7
+#define DMB dmb
+#elif __ARM_ARCH >= 6
+#define DMB mcr p15, #0, r0, c7, c10, #5
+#else
+#error DMB is only supported on ARMv6+
+#endif
+
 #define SYNC_OP_4(op)                                                          \
   .p2align 2;                                                                  \
   .syntax unified;                                                             \
   DEFINE_COMPILERRT_FUNCTION(__sync_fetch_and_##op)                            \
   DMB;                                                                         \
   mov r12, r0;                                                                 \
   LOCAL_LABEL(tryatomic_##op) : ldrex r0, [r12];                               \
   op(r2, r0, r1);                                                              \

compiler-rt/lib/builtins/assembly.h

 #if __ARM_ARCH >= 5
 #define POP_PC() pop {pc}
 #else
 #define POP_PC()                                                               \
   pop {ip};                                                                    \
   JMP(ip)
 #endif
 
-#if __ARM_ARCH >= 7
-#define DMB dmb
-#elif __ARM_ARCH >= 6
-#define DMB mcr p15, #0, r0, c7, c10, #5
-#else
-#error only supported on ARMv6+
-#endif
-
 #if defined(USE_THUMB_2)
 #define WIDE(op) op.w
 #else
 #define WIDE(op) op
 #endif
 #else // !defined(__arm)
 #define DECLARE_FUNC_ENCODING
 #define DEFINE_CODE_STATE