Page MenuHomePhabricator

[ARM] Make coprocessor number restrictions consistent.
ClosedPublic

Authored by simon_tatham on Jun 27 2019, 2:49 AM.

Details

Summary

Different versions of the Arm architecture disallow the use of generic
coprocessor instructions like MCR and CDP on different sets of
coprocessors. This commit centralises the check of the coprocessor
number so that it's consistent between assembly and disassembly, and
also updates it for the new restrictions in Arm v8.1-M.

New tests added that check all the coprocessor numbers; old tests
updated, where they used a number that's now become illegal in the
context in question.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.Jun 27 2019, 2:49 AM
This revision is now accepted and ready to land.Jun 27 2019, 3:26 AM
This revision was automatically updated to reflect the committed changes.

This change made coprocessor numbers other than 14/15 invalid for ARMv8, and thus some instructions can't be assembled.

% cat a.s
ldc     p2, c4, [r12], #48
% llvm-mc -filetype=null -triple armv7-linux-musleabi a.s
# ok
% llvm-mc -filetype=null -triple armv8-linux-musleabi a.s
        .text
a.s:1:5: error: invalid operand for instruction
ldc p2, cr4, [ip], #48
    ^

Such instructions are used by musl guarded by an AT_HWCAP test.

// musl/src/setjmp/arm/longjmp.s
	tst r1,#0x20
	beq 2f
	ldc p2, cr4, [ip], #48
2:	tst r1,#0x40

.hidden __hwcap
.align 2
1:	.word __hwcap-1b

The consequence is that musl can't be built with clang-9 -target armv8-linux-musleabihf.

Hmmm. This surely can't be the first time a case like this has come up. What's the usual solution in other similar situations, when you want to include code for mutually incompatible architectures in the same object because you're going to test at run time which one to execute?

nsz added a subscriber: nsz.Jul 17 2019, 1:50 AM

Hmmm. This surely can't be the first time a case like this has come up. What's the usual solution in other similar situations, when you want to include code for mutually incompatible architectures in the same object because you're going to test at run time which one to execute?

separate use of .arch and .object_arch works sometimes (to allow use of instructions for a new arch but mark the object file to be compatible with an old one, however that does not work when there is no "new arch" which supports all the instructions in use)

or use .inst, but old binutils does not support that so .word is the only option which will be wrong on bigendian so conditional .word and then there is thumb vs arm mode to deal with too.

either way this kind of code is way more painful to write than it should be, ideally there should be a flag that tells the assembler to accept the union of all instructions supported on the architecture especially on targets like arm where you have to write this kind of runtime checks to be able to create portable binaries.

i suspect the musl longjmp asm is not the only thing this patch broke (multimedia codecs also tend to do this sort of runtime checks in asm)

So it sounds as if the most immediate problem is that there now _isn't_ an architecture you can specify on clang's command line that permits the union of all the instructions you plan to use?

Would some kind of architectural extension be a reasonable solution? -march=armv8+oldcoprocessors, or some such? That way -march=armv8 continues to diagnose things that actually are illegal in the architecture you asked for.

nsz added a comment.Jul 17 2019, 6:13 AM

Would some kind of architectural extension be a reasonable solution? -march=armv8+oldcoprocessors, or some such? That way -march=armv8 continues to diagnose things that actually are illegal in the architecture you asked for.

that's a possible approach, another is to have some temporary .arch setting for special instructions and restore the original setting after them, i'm not sure how such code is handled across the various assemblers.

if mixing .arch works then we can try to annotate the asm with different .arch settings (unfortunately there is no push/pop for the setting so restoring is only possible if we know the -march).