Page MenuHomePhabricator

[ARM][AsmParser] handles offset expression in parentheses
ClosedPublic

Authored by jcai19 on Oct 9 2019, 11:50 PM.

Details

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

Diff Detail

Event Timeline

jcai19 created this revision.Oct 9 2019, 11:50 PM
jcai19 added subscribers: manojgupta, llozano.
jcai19 updated this revision to Diff 224263.Oct 9 2019, 11:54 PM

Remove typos.

acceet offset

typo

llvm/test/MC/ARM/gas-compl.s
1 ↗(On Diff #224263)

Change to a better test name?

jcai19 updated this revision to Diff 224270.Oct 10 2019, 12:29 AM

Fix bugs.

jcai19 updated this revision to Diff 224430.Oct 10 2019, 11:44 AM

Fix a typo.

acceet offset

typo

Done. Thanks for catching it.

jcai19 edited the summary of this revision. (Show Details)Oct 10 2019, 11:45 AM

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 added inline comments.Oct 10 2019, 1:13 PM
llvm/test/MC/ARM/gas-compl.s
1 ↗(On Diff #224263)

+1 for a better test name.

llvm/test/MC/ARM/gas-compl.s
17 ↗(On Diff #224430)

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

jcai19 updated this revision to Diff 224503.Oct 10 2019, 5:00 PM

Expand test cases and rename test file.

jcai19 marked 6 inline comments as done.Oct 10 2019, 5:03 PM
jcai19 added inline comments.
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
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 updated this revision to Diff 224673.Oct 11 2019, 1:37 PM
jcai19 marked 4 inline comments as done.

Update based on comments.

jcai19 marked 3 inline comments as done.Oct 11 2019, 1:38 PM
jcai19 added inline comments.
llvm/test/MC/ARM/gas-compl-mem-offset-paren.s
4

Oops! Thanks for the catch.

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

missing the i on the end.

jcai19 updated this revision to Diff 224901.Oct 14 2019, 1:55 PM

Fix a typo

jcai19 marked an inline comment as done.Oct 14 2019, 1:56 PM
jcai19 added inline comments.
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 accepted this revision.Oct 14 2019, 2:14 PM

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

This revision is now accepted and ready to land.Oct 14 2019, 2:14 PM

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

No problem! Thanks for all the comments.

This revision was automatically updated to reflect the committed changes.