[ARM][AsmParser] handles offset expression in parentheses

[jcai19]

Summary

Integrated assembler does not accept offset expressions surrounded by
parenthesis. Handle this case for GAS compability.
https://bugs.llvm.org/show_bug.cgi?id=43631

[jcai19]

Remove typos.

[MaskRay]

acceet offset

typo

llvm/test/MC/ARM/gas-compl.s

1 ↗

(On Diff #224263)

Change to a better test name?

[jcai19]

Fix bugs.

[jcai19]

Fix a typo.

[jcai19]

In D68764#1703095, @MaskRay wrote:

acceet offset

typo

Done. Thanks for catching it.

[nickdesaulniers]

Great test cases. Thanks for the patch!

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

5742–5745

Prefer:

if (foo && bar)
  baz();

to:

if (foo)
  if (bar)
    baz();

5743–5745

Looks like Parser.getTok().isNot is more readable that !Parser.getTok().is().

llvm/test/MC/ARM/gas-compl.s

4 ↗

(On Diff #224430)

An assembler directive should be set once. Resetting the syntax to unified has no effect. You can set it once in this test, then never again.

[manojgupta]

llvm/test/MC/ARM/gas-compl.s

1 ↗

(On Diff #224263)

+1 for a better test name.

[nickdesaulniers]

llvm/test/MC/ARM/gas-compl.s

17 ↗

(On Diff #224430)

Does gas support multiple parens, ie. ((15+5))? Do we?

[jcai19]

Expand test cases and rename test file.

[jcai19]

llvm/test/MC/ARM/gas-compl.s

17 ↗

(On Diff #224430)

Good question! Just verified GAS does, and we do too thanks to getParser().parseExpression(Offset) taking care of parentheses.

$ cat sample.s
.syntax unified

ldr r12, [sp, $(((15+5)*5))]

$ armv7a-cros-linux-gnueabihf-as sample.s -o sample.o; armv7a-cros-linux-gnueabihf-objdump -d sample.o

sample.o: file format elf32-littlearm

Disassembly of section .text:

00000000 <.text>:

0:	e59dc064 	ldr	ip, [sp, #100]	; 0x64

[nickdesaulniers]

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

5735–5736

Should this comment also mention '$'?

5743–5745

This line length looks a little long. Did you remember to run git-clang-format HEAD~ and amend that?

5743–5745

Based on the body of the if statement, would the condition Parser.getTok().is(AsmToken::Hash) || Parser.getTok().is(AsmToken::Dollar) work? If so, I think it would make more sense to use that, rather than check it's not the other cases.

llvm/test/MC/ARM/gas-compl-mem-offset-paren.s

2

Since this is a GAS compliance test, let's use a GAS triple, like -triple=arm-linux-gnueabi.

4

If you remove this assembler directive outright, does the test still pass? If so, let's remove it. Also, it seems that you partially removed the other occurrences, but not all of them. It should occur once, or not at all (unless you wanted to test changing back and forth between them, but that's not what we're testing here).

[jcai19]

Update based on comments.

[jcai19]

llvm/test/MC/ARM/gas-compl-mem-offset-paren.s

4

Oops! Thanks for the catch.

[nickdesaulniers]

llvm/test/MC/ARM/gas-compl-mem-offset-paren.s

2

missing the i on the end.

[jcai19]

Fix a typo

[jcai19]

llvm/test/MC/ARM/gas-compl-mem-offset-paren.s

2

Thanks for the catch. Don't know why llvm-mc let it slide when I ran llvm-lit.

[nickdesaulniers]

Thanks for the patch and for following up on code review!

[jcai19]

In D68764#1708513, @nickdesaulniers wrote:

Thanks for the patch and for following up on code review!

No problem! Thanks for all the comments.

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

if (Parser.getTok().is(AsmToken::Colon)) {
     // operand.
     if (Parser.getTok().is(AsmToken::Exclaim)) {
       Operands.push_back(ARMOperand::CreateToken("!",Parser.getTok().getLoc()));
       Parser.Lex(); // Eat the '!'.
     }
 
     return false;
   }
 
-  // If we have a '#', it's an immediate offset, else assume it's a register
-  // offset. Be friendly and also accept a plain integer (without a leading
-  // hash) for gas compatibility.
+  // If we have a '#' or '$', it's an immediate offset, else assume it's a
+  // register offset. Be friendly and also accept a plain integer or expression
+  // (without a leading hash) for gas compatibility.
   if (Parser.getTok().is(AsmToken::Hash) ||
       Parser.getTok().is(AsmToken::Dollar) ||
+      Parser.getTok().is(AsmToken::LParen) ||
       Parser.getTok().is(AsmToken::Integer)) {
-    if (Parser.getTok().isNot(AsmToken::Integer))
-      Parser.Lex(); // Eat '#' or '$'.
+    if (Parser.getTok().is(AsmToken::Hash) ||
+        Parser.getTok().is(AsmToken::Dollar))
+      Parser.Lex(); // Eat '#' or '$'
     E = Parser.getTok().getLoc();
 
     bool isNegative = getParser().getTok().is(AsmToken::Minus);
     const MCExpr *Offset;
     if (getParser().parseExpression(Offset))
      return true;
 
     // The expression has to be a constant. Memory references with relocations

llvm/test/MC/ARM/gas-compl-mem-offset-paren.s

+@ RUN: llvm-mc -triple=arm-linux-gnueabi < %s | FileCheck %s
+
+@ CHECK: ldr  r12, [sp, #15]
+ldr r12, [sp, (15)]
+
+@ CHECK: ldr  r12, [sp, #15]
+ldr r12, [sp, #(15)]
+
+@ CHECK: ldr  r12, [sp, #15]
+ldr r12, [sp, $(15)]
+
+@ CHECK: ldr  r12, [sp, #100]
+ldr r12, [sp, (((15+5)*5))]
+
+@ CHECK: ldr  r12, [sp, #100]
+ldr r12, [sp, #(((15+5)*5))]
+
+
+@ CHECK: ldr  r12, [sp, #100]
+ldr r12, [sp, $(((15+5)*5))]