This is an archive of the discontinued LLVM Phabricator instance.

Differential D99282

Use mcr for dmb instruction on armv6
ClosedPublic

Authored by raj.khem on Mar 24 2021, 10:49 AM.

Details

Reviewers
phosek
dmgreen
MaskRay
Commits
rGa11d1cc41c72: [builtins] Use mcr for dmb instruction on armv6
Summary

At present compiler-rt cross compiles for armv6 ( -march=armv6 ) but includes dmb instructions which are only available in armv7+ this causes SIGILL on clang+compiler-rt compiled components on rpi0w platforms.

Diff Detail

Event Timeline

raj.khem created this revision.Mar 24 2021, 10:49 AM
Herald added a subscriber: kristof.beyls. · View Herald TranscriptMar 24 2021, 10:49 AM
raj.khem requested review of this revision.Mar 24 2021, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 10:49 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
Harbormaster completed remote builds in B95531: Diff 333043.Mar 24 2021, 6:53 PM
raj.khem added a reviewer: phosek.Jun 12 2021, 8:56 AM

ping,

raj.khem mentioned this in D104183: [builtins] Do not force thumb mode directive in arm/sync-ops.h.Jun 12 2021, 9:09 AM
MaskRay added a reviewer: dmgreen.Jun 13 2021, 10:34 AM
MaskRay added a subscriber: MaskRay.Jun 13 2021, 10:36 AM
MaskRay added inline comments.
compiler-rt/lib/builtins/assembly.h
187

I see that musl src/thread/arm/atomics.s uses .arch armv6t2; mcr p15,0,r0,c7,c10,5

Is #elif __ARM_ARCH >= 6 accurate?

raj.khem added inline comments.Jun 14 2021, 12:39 AM
compiler-rt/lib/builtins/assembly.h
187

yes I think it is correct. Musl is using that directive for generating thumb2 code when -mthumb is used along with mcr barrier. mcr is available in both thumb/arm ISAs on ARMv6

raj.khem updated this revision to Diff 414563.Mar 10 2022, 7:23 PM

rebased

Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 7:23 PM
raj.khem added a comment.Mar 10 2022, 7:24 PM

@MaskRay can you help in moving this forward ?

MaskRay accepted this revision.Mar 10 2022, 7:25 PM
This revision is now accepted and ready to land.Mar 10 2022, 7:25 PM
This revision was landed with ongoing or failed builds.Mar 10 2022, 7:30 PM
Closed by commit rGa11d1cc41c72: [builtins] Use mcr for dmb instruction on armv6 (authored by raj.khem, committed by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.
MaskRay added a commit: rGa11d1cc41c72: [builtins] Use mcr for dmb instruction on armv6.
Harbormaster completed remote builds in B153704: Diff 414563.Mar 10 2022, 7:41 PM
MaskRay mentioned this in rG5017de8ba4b1: [builtins] Do not force thumb mode directive in arm/sync-ops.h.Mar 11 2022, 4:26 PM
pzheng mentioned this in D123105: [compiler-rt][builtins] Move DMB definition to syn-ops.h.Apr 4 2022, 6:37 PM
efriedma added subscribers: pzheng, efriedma.Apr 5 2022, 11:40 AM

I'm not sure this is right... even if dmb barriers do work, strex is only available in v7+ in Thumb mode, so I suspect this still causes a SIGILL on v6, whether or not Thumb2 is available.

Herald added a subscriber: StephenFan. · View Herald TranscriptApr 5 2022, 11:40 AM
pzheng mentioned this in rG1c9415806ba6: [compiler-rt][builtins] Move DMB definition to syn-ops.h.Apr 7 2022, 2:58 PM

Revision Contents

PathSize
compiler-rt/
lib/
builtins/
arm/
8 lines
8 lines

Diff 414563

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

Context not available.
.thumb; \ .thumb; \
.syntax unified; \ .syntax unified; \
DEFINE_COMPILERRT_THUMB_FUNCTION(__sync_fetch_and_##op) \ DEFINE_COMPILERRT_THUMB_FUNCTION(__sync_fetch_and_##op) \
dmb; \ DMB; \
mov r12, r0; \ mov r12, r0; \
LOCAL_LABEL(tryatomic_##op) : ldrex r0, [r12]; \ LOCAL_LABEL(tryatomic_##op) : ldrex r0, [r12]; \
op(r2, r0, r1); \ op(r2, r0, r1); \
strex r3, r2, [r12]; \ strex r3, r2, [r12]; \
cmp r3, #0; \ cmp r3, #0; \
bne LOCAL_LABEL(tryatomic_##op); \ bne LOCAL_LABEL(tryatomic_##op); \
dmb; \ DMB; \
bx lr bx lr
#define SYNC_OP_8(op) \ #define SYNC_OP_8(op) \
Context not available.
.syntax unified; \ .syntax unified; \
DEFINE_COMPILERRT_THUMB_FUNCTION(__sync_fetch_and_##op) \ DEFINE_COMPILERRT_THUMB_FUNCTION(__sync_fetch_and_##op) \
push {r4, r5, r6, lr}; \ push {r4, r5, r6, lr}; \
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-  push {r4, r5, r6, lr};                                                       \
+  push{r4, r5, r6, lr};                                                        \
Lint: Pre-merge checks: clang-format: please reformat the code ``` - push {r4, r5, r6, lr}…
dmb; \ DMB; \
mov r12, r0; \ mov r12, r0; \
LOCAL_LABEL(tryatomic_##op) : ldrexd r0, r1, [r12]; \ LOCAL_LABEL(tryatomic_##op) : ldrexd r0, r1, [r12]; \
op(r4, r5, r0, r1, r2, r3); \ op(r4, r5, r0, r1, r2, r3); \
strexd r6, r4, r5, [r12]; \ strexd r6, r4, r5, [r12]; \
cmp r6, #0; \ cmp r6, #0; \
bne LOCAL_LABEL(tryatomic_##op); \ bne LOCAL_LABEL(tryatomic_##op); \
dmb; \ DMB; \
pop { r4, r5, r6, pc } pop { r4, r5, r6, pc }
#define MINMAX_4(rD, rN, rM, cmp_kind) \ #define MINMAX_4(rD, rN, rM, cmp_kind) \
Context not available.

compiler-rt/lib/builtins/assembly.h

Context not available.
JMP(ip) JMP(ip)
#endif #endif
#if __ARM_ARCH >= 7
#define DMB dmb
#elif __ARM_ARCH >= 6
#define DMB mcr p15, #0, r0, c7, c10, #5
#else
#error Only use this with ARMv6+
#endif
#if defined(USE_THUMB_2) #if defined(USE_THUMB_2)
#define WIDE(op) op.w #define WIDE(op) op.w
#else #else
Context not available.