Page MenuHomePhabricator

[ARM] Issue an eror when non-general-purpose registers used in address operands

Authored by chill on Nov 10 2017, 10:53 AM.



Currently the assembler will happily assemble, e.g. ldr r0, [s0, #12] and similar.
This patch add checks that only general-purpose registers are used in address operands, shifted registers, and shift amounts.
This is done in the asm parser, as it results in better error messages, compared do doing the check in the various AsmOperandClass predicates.

Diff Detail

Event Timeline

chill created this revision.Nov 10 2017, 10:53 AM

Less preferable (IMHO) alternative here:

olista01 edited edge metadata.Nov 13 2017, 3:19 AM

Hi Momchil,

I think your alternative patch is actually the right way to go here. The problem with doing this in ARMAsmParser::parseMemory and friends is that we can't reject SP and PC for instructions where they are not valid. Doing that here would require moving a lot more instruction-specific logic into these functions, which isn't going to scale well.

I have some WIP patches that do something similar to D39910, and also expand the diagnostics. These patches aren't complete (need to cover more operand types and more tests), but I'll send you a copy of them.

chill added a comment.Nov 14 2017, 7:22 AM

Correct me if I'm wrong ...

For me it seems that the approach of returning just true/false from the asm operand class predicates, while
good for weeding out invalid instructions, isn't that great for diagnostics. The operand may be invalid for
a variety of reasons, but we have only one diagnostics string, which can at best list all the possible constraints
and let the user figure out by themselves what's wrong.

For example, assembling:

vld1.16 {d0}, [s0:64]

with llvm-mc -triple=armv7a-eabi gives error: alignment must be 64 or omitted, and if we need to improve
this message, we'd have to list the constrain that the register must be GPR. And do that for all/lot-of the other
AsmOperandClassses, which would create a huge number of repetetetetitions of the constraint "X must be a GPR".
If we instead diagnose usage of non-GPR registers in the parser, and leave checking more specific constraints
(like "X must not be PC" or "X must be SP") to asm operand class predicates, some diagnostics will be more
precise and the rest will require less effort by the user, because that'd list a comparatively small number of
constraints to manually check.

chill added a comment.Nov 20 2017, 9:53 AM

So, this is now suspended in favor of