Also has a pair of fixes for armv4t. Let me know if you'd like those in a separate patch.
Diff Detail
Event Timeline
src/Unwind/UnwindRegistersSave.S | ||
---|---|---|
332 | Doesn't this change the behavior for __ARM_ARCH >= 7? | |
349 | Doesn't this change the behavior for __ARM_ARCH >= 7? | |
364 | This also doesn't work for ARMv7. Consider ARM VFPv3-d16 (e.g. Tegra2) ... vstmia r0, {d16-d31} refers to a non-existent register set. | |
388 | Do all CPUs have the wmmxt register set? It might be more prudent to move this out into a separate translation unit that is linked in if the target supports iwmmxt. | |
415–416 | Oh joy ... this is scary. Do we have some guarantee that they won't accidentally use long rather than int? | |
416 | Similar to above. |
src/Unwind/UnwindRegistersRestore.S | ||
---|---|---|
340 | I think we should use the same trick we used in other ARM files, to define a macro that is set by the arch for returning. | |
351 | Do you want to export the symbols (unwindVFP) but have it do nothing? Is it even possible to try to unwind VFP on v6M? Shouldn't this be a compilation error? | |
377 | This macro is in a different order than the one above, and will generate vldmia on v6M which will fail. | |
src/Unwind/UnwindRegistersSave.S | ||
285 | Maybe we should also have a macro that will use thumb on thumb-only platforms like v6M and v6M variants. That would have to be accompanied by changes to other areas, making them Thumb/2 friendly (like the ones you add here). | |
364 | And this is exactly the opposite as the other, which would just get "bx lr" on ARMv6M |
Sorry about the hideously bad merge....
src/Unwind/UnwindRegistersRestore.S | ||
---|---|---|
340 | Sounds like a good idea to me. | |
351 | I think that this will never get called on v6m because it doesn't have vfp registers, so the compiler will never issue the unwind opcodes that trigger this being called. Maybe that warrants having the body be: assert(false) ? This could be completely ifdef'd out on v6m, but that would mean ifdefing out the callers too, which would be a bit ugly. It does save a few bytes though.... | |
377 | Oops, bad merge. Yeah, they should be transposed. | |
394 | Another bad merge. Transpose this with the line above it. | |
436–437 | Another bad merge / patch cut. I didn't intend to include the s/MMXControlEPj/MMXControlEPm/ change. | |
src/Unwind/UnwindRegistersSave.S | ||
285 | like a LIBUNWIND_THUMB_ONLY flag? | |
332 | Another bad merge. This line, and the one above it should be transposed. | |
349 | Another bad merge. This line, and the one above it should be transposed. | |
364 | Conditions on this one should be flipped.... oops. | |
377–378 | Maybe this should use that RETURN macro @rengolin suggests? | |
388 | I think the idea here (before my change) was that if you were to build this library with -march=armv5t then you could either link it with code for a PXA270 and get something that would work on that arch, or link it with normal v5t code and get something that would work on a vanilla v5t board. With my change I am trying to be conservative... I don't know whether there are any v6m's with iwmmx support, but if there are, then this should DTRT. | |
415–416 | Oops, another bad patch cut. The s/saveiWMMXControlEPj/saveiWMMXControlEPm/ change is part of a different patch where I change the type of the argument to void*. |
Move return instructions into ARM_RETURN macro.
Add ARM_THUMB1_ONLY guard.
Add ARM_HAS_NO_VFP guard.
Please drop ARM_RETURN and use JMP(lr). That is duplicating an existing macro which is already in use.
Im not sure I understand why you are creating a new macro for the thumb check. __ARM_ARCH_ISA_THUMB == 1 is sufficient to identify if you only have Thumb-1. However, do you really want to use thumb instructions if you only have Thumb-1?
I dont understand the need for the ARM_HAS_NO_VFP. Isn't __VFP_FP__ usable for your check there? I don't think we need yet another macro, particularly if there is one that is defined by the compiler.
Finally, is __ARM_WMMX supposed to be defined for ARMv6M/s?
Sure, I'll copy over the ARM part of compiler_rt's assembly.h
Im not sure I understand why you are creating a new macro for the thumb check. __ARM_ARCH_ISA_THUMB == 1 is sufficient to identify if you only have Thumb-1. However, do you really want to use thumb instructions if you only have Thumb-1?
Sorry, bad name. What I meant here is to have a macro that means "doesn't have the ARM instruction set" (i.e. v6m, v6sm, v7m). I think this is what I actually mean:
#if !defined(__ARM_ARCH_ISA_ARM)
I dont understand the need for the ARM_HAS_NO_VFP. Isn't __VFP_FP__ usable for your check there? I don't think we need yet another macro, particularly if there is one that is defined by the compiler.
No, __VFP_FP__ isn't the right one for this. The best reference I can find for what that one means is this: https://wiki.debian.org/ArmEabiPort "Note that __VFP_FP__ does not mean that VFP code generation has been selected. It only speaks of the floating point data format in use and is normally set when soft-float has been selected."
After some digging around in http://infocenter.arm.com/help/topic/com.arm.doc.ihi0053c/IHI0053C_acle_2_0.pdf I think __ARM_FP is the right one to use, but it looks like clang doesn't ever define that one on 32bit arm. I should probably fix that first....
Finally, is __ARM_WMMX supposed to be defined for ARMv6M/s?
I don't know of any v6m/v6sm's with iWMMX instructions (and therefore __ARM_WMMX), but it might be possible that could exist, hence the:
#if (!defined(__ARM_ARCH_6M__) && !defined(__ARM_ARCH_6SM__)) || __ARM_WMMX
My assembler barfs on the coprocessor instructions in those blocks, so maybe it's not possible to have iWMMX instructions on a v6m part?
Address a few of @compnerd's comments. This is contingent on fixing __ARM_FP in clang.
assembly.h | ||
---|---|---|
65 ↗ | (On Diff #13753) | The whole block of __ARM_ARCH seems incorrect to me. What compiler are you trying to support? Both clang and gcc have supported this part of the ACLE for some time now. |
Dump the __ARM_ARCH ACLE shims that were copy-pasted from compiler_rt's assembly.h (and have been deleted there).
I think we should use the same trick we used in other ARM files, to define a macro that is set by the arch for returning.