[ARM] Fix dropped dollar sign from symbols in branch targets

[pratlucas]

Summary

ARMAsmParser was incorrectly dropping a leading dollar sign character
from symbol names in targets of branch instructions. This was caused by
an incorrect assumption that the contents following the dollar sign
token should be handled as a constant immediate, similarly to the #
token.

This patch avoids the operand parsing from consuming the dollar sign
token when it is followed by an identifier, making sure it is properly
parsed as part of the expression.

[efriedma]

If the goal here is to match the GNU assembler, I don't think you've achieved that; gas treats "b $4" as a branch to the symbol "$4", not an absolute offset.

[chill]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6123

What if we have here $ foo, i.e. whitespace after the dollar? We should not paste together two separate tokens $ and <whatever> to form an identifier, $foo, $12 are identifiers, but $ 12 is not.

[pratlucas]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6123

From what I've checked, parseExpression(...) takes care of this scenario:

<stdin>:36:11: error: invalid token in expression
        b $ foo
          ^

The change is not binding the two actual tokens together, but only refraining from removing the $ token from the expression.
Please let me know if you believe a more active handling of this scenario is necessary.

[pratlucas]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6123

Checking the behavior without the changes from the patch, expressions like b $ foo are currently accepted by the parser with no errors, as the $ token is dropped from the expression.

[pratlucas]

In D73176#1835170, @efriedma wrote:

If the goal here is to match the GNU assembler, I don't think you've achieved that; gas treats "b $4" as a branch to the symbol "$4", not an absolute offset.

Good point @efriedma , I took a look at gas and it indeed handles $4 as a symbol on branch instructions.

I've checked the patch that originated the existing behavior (ef70e9b704f7f482faac0696c7288743061ed652) and it appears to have introduced this intentionally, though. The following part of the commit message got me concerned that we might break the handling of legacy code if matching gas' behavior:

Backwards compatibility with 'gas'. #imm is the preferered and documented syntax, but lots of existing code uses the '$' prefix, so we should support it if we can.

Do you have any thoughts on that? I couldn't find any example of the mentioned "existing code" on the regression tests.

[chill]

There's obviously lexer ambiguity in interpreting, e.g. the character sequence $4 as a single <identifier> token, or as the two tokens $ and a <constant>.
Looks like the tokenisation needs to be context dependent: in certain directives (.global $4) and instructions (b $4) the sequence needs to be interpreted a single identifier, and
in other contexts (ldr r0, [r0, $4]) as two separate tokens.

[chill]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6123

Well, if we have two separate tokens (AsmToken::Dollar and AsmToken::Idenrifier), but somehow end up with symbol name $foo, something must be combining those tokens into one.

That happens in AsmParser::parseIdentifier (https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCParser/AsmParser.cpp#L2844), but (unfortunately?) this function does not handle
a non-identifier immediately following the $.

[chill]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6123

(Indeed it check tokens are adjacent).

[pratlucas]

[ARM] Updating handling of $-prefixed operands for branch instructions

Updating handling of the dollar sign token to check for the Mnemonic being
evaluated in order to decide wether to try to parse it as a $-prefixed
symbol or to consider it the start of an immediate expression. The token
should be considered part of a symbol when handling branch instructions.

[pratlucas]

Ping.

[chill]

Could you, please, add tests for the following cases (some of them will fail, and need to be fixed):

1. b $4

$4 needs to be interpreted as a symbol name; It's GCC behaviour as well.

2. b $foo + 4

It's an error now; needs to be like b foo + 4, but with symbol $foo.

3. b $ 4, b $ foo + 4 and b $ foo

These work as expected.

Rationale: GCC does not support $ as introducing an expression, but does
allow an identifier to start with a $. IMHO, our approach here should be:
when there's ambiguity, resolve it in favour of the GCC's interpretation, e.g. b $4 is parsed
as a branch to symbol.

[pratlucas]

Regarding the b $4 case, I found that the limitation currently lies in the expression/identifier parsing at the core AsmParser, which only detects $-prefixed Identifier or String tokens at the moment.
As this part of the code has no knowledge of the branch instruction context, I'm afraid updating it to accept Integer tokens as well might cause some inconsistencies in the instructions for which we drop the leading Dollar token. For example:

# Branch instructions consistently treat $<integere> as identifier
b   $4                  @ b ($4)         - Symbol
b   $4 + $4             @ b #($4)+($4)   - Symbol
# Other instructions treat those as identifiers only when they're
# on the begining of the operand
movw  r0, $4            @ movw r0, #4            - Immediate constant
movw  r0, :lower16:$4   @ movw r0, :lower16:($4) - Symbol

From what I checked on the gas behavior, it seems to only treat those as constant immediates when actual constants are explicitly expected by the instruction. Otherwise it assumes those are identifiers.
Would you say the behavior above is the expected one? Or should we go for the alignment with gas' behavior?

[chill]

IMHO, we should resolve ambiguity between $ starting an expression, and $ being part of an identifier in favour of the latter, reason
being that using $ instead of # is a "non-standard" extension, whereas dollars in identifiers must be fully supported.

[pratlucas]

I agree with resolving the ambiguity in favor of identifiers regardless of the mnemonic for the general-case operand parsing, avoiding inconsistencies. Specific cases where constant immediates are explicitly expected are currently handled by their own ParseMethod implementation.
If there's no objection, I'll update the patch to reflect this behavior.

[pratlucas]

Removed restriction of branch instruction mnemonic for allowing $-prefixed
symbol names in operands, resolving ambriguity in favor of symbol names over
expression start.

There is currently a limitation on AsmParser's core causing $-prefixed symbol
names composed by numeric sequences not to be accepted. This will be handled in
a separate patch.
The test case for b $4 will be added to the separate patch accordingly.

[pratlucas]

Ping.

[efriedma]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6135

AsmToken::String? I don't see any testcase for that.

[pratlucas]

Addressing review comment.

[pratlucas]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6135

I removed AsmToken::String from the identifier handling in D75111 but forgot to update this condition. Sorry about that.

[pratlucas]

Addressing review.

[pratlucas]

Formatting patch according to clang-format and clang-tidy.

[efriedma]

LGTM

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

case AsmToken::Dot: {   // . as a branch target
     Operands.push_back(ARMOperand::CreateImm(IdVal, S, E));
     return false;
   }
   case AsmToken::LBrac:
     return parseMemory(Operands);
   case AsmToken::LCurly:
     return parseRegisterList(Operands, !Mnemonic.startswith("clr"));
   case AsmToken::Dollar:
-  case AsmToken::Hash:
-    // #42 -> immediate.
+  case AsmToken::Hash: {
+      // #42 -> immediate
+      // $ 42 -> immediate
+      // $foo -> symbol name
+      // $42 -> symbol name
       S = Parser.getTok().getLoc();
+
+      // Favor the interpretation of $-prefixed operands as symbol names.
+      // Cases where immediates are explicitly expected are handled by their
+      // specific ParseMethod implementations.
+      auto AdjacentToken = getLexer().peekTok(/*ShouldSkipSpace=*/false);
+      bool ExpectIdentifier = Parser.getTok().is(AsmToken::Dollar) &&
+          (AdjacentToken.is(AsmToken::Identifier) ||
+           AdjacentToken.is(AsmToken::String) ||
+           AdjacentToken.is(AsmToken::Integer));
+      if (!ExpectIdentifier) {
+        // Token is not part of identifier. Drop leading $ or # before parsing expression.
         Parser.Lex();
+      }
 
       if (Parser.getTok().isNot(AsmToken::Colon)) {
           bool isNegative = Parser.getTok().is(AsmToken::Minus);
           const MCExpr *ImmVal;
           if (getParser().parseExpression(ImmVal))
               return true;
           const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(ImmVal);
           if (CE) {
               int32_t Val = CE->getValue();
               if (isNegative && Val == 0)
                   ImmVal = MCConstantExpr::create(std::numeric_limits<int32_t>::min(),
                                                   getContext());
           }
           E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
           Operands.push_back(ARMOperand::CreateImm(ImmVal, S, E));
 
           // There can be a trailing '!' on operands that we want as a separate
           // '!' Token operand. Handle that here. For example, the compatibility
           // alias for 'srsdb sp!, #imm' is 'srsdb #imm!'.
           if (Parser.getTok().is(AsmToken::Exclaim)) {
               Operands.push_back(ARMOperand::CreateToken(Parser.getTok().getString(),
                                                          Parser.getTok().getLoc()));
               Parser.Lex(); // Eat exclaim token
           }
           return false;
       }
       // w/ a ':' after the '#', it's just like a plain ':'.
       LLVM_FALLTHROUGH;
-
+  }
   case AsmToken::Colon: {
     S = Parser.getTok().getLoc();
     // ":lower16:" and ":upper16:" expression prefixes
     // FIXME: Check it's an expression prefix,
     // e.g. (FOO - :lower16:BAR) isn't legal.
     ARMMCExpr::VariantKind RefKind;
     if (parsePrefix(RefKind))
       return true;

llvm/test/MC/ARM/arm-branches.s

 @ RUN: llvm-mc -triple=armv7-apple-darwin -show-encoding < %s | FileCheck %s
 
 @------------------------------------------------------------------------------
 @ Branch targets destined for ARM mode must == 0 (mod 4), otherwise (mod 2).
 @------------------------------------------------------------------------------
 
         b #4
         bl #4
         beq #4
         blx #2
 
 @ CHECK: b  #4                      @ encoding: [0x01,0x00,0x00,0xea]
 @ CHECK: bl  #4                      @ encoding: [0x01,0x00,0x00,0xeb]
 @ CHECK: beq  #4                      @ encoding: [0x01,0x00,0x00,0x0a]
 @ CHECK: blx  #2                      @ encoding: [0x00,0x00,0x00,0xfb]
+
+@------------------------------------------------------------------------------
+@ Leading '$' on branch targets must not be dropped if part of symbol names
+@------------------------------------------------------------------------------
+
+        .global $foo
+        b $foo
+        bl $foo
+        beq $foo
+        blx $foo
+        b $foo + 4
+
+@ CHECK: b      ($foo)                      @ encoding: [A,A,A,0xea]
+@ CHECK: bl     ($foo)                      @ encoding: [A,A,A,0xeb]
+@ CHECK: beq    ($foo)                      @ encoding: [A,A,A,0x0a]
+@ CHECK: blx    ($foo)                      @ encoding: [A,A,A,0xfa]
+@ CHECK: b      #($foo)+4                   @ encoding: [A,A,A,0xea]
+
+@------------------------------------------------------------------------------
+@ Leading '$' should be allowed to introduce an expression
+@------------------------------------------------------------------------------
+
+        .global bar
+        b $ 4
+        bl $ bar + 4
+        blx $ bar
+@ CHECK: b      #4                        @ encoding: [0x01,0x00,0x00,0xea]
+@ CHECK: bl     #bar+4                    @ encoding: [A,A,A,0xeb]
+@ CHECK: blx    bar                       @ encoding: [A,A,A,0xfa]