This is an archive of the discontinued LLVM Phabricator instance.

[ARM][disassembler] Fix incorrect number of operands MCInst generated by the disassembler
ClosedPublic

Authored by myhsu on Apr 15 2021, 11:18 AM.

Details

Summary

Try to fix bug 49974.

This patch fixes two issues:

  1. BL does not use predicate (BL_pred is the predicate version of BL), so we shouldn't add related operands in DecodeBranchImmInstruction.
  2. Inside DecodeT2AddSubSPImm, we shouldn't add predicate operands into the MCInst because ARMDisassembler::AddThumbPredicate will do that for us. However, we should handle CC-out operand for t2SUBspImm and t2AddspImm.

I can't think of a way to test this patch because all the existing disassembler tests are based on disassembled assembly code.

Diff Detail

Event Timeline

myhsu created this revision.Apr 15 2021, 11:18 AM
myhsu requested review of this revision.Apr 15 2021, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 11:18 AM
myhsu added a comment.Apr 20 2021, 9:24 AM

ping.
Any comment on this fix?

dnsampaio resigned from this revision.Apr 20 2021, 11:14 PM

Sorry but I'm not the right person for this.

dmgreen added a subscriber: dmgreen.

Can you add a test using llvm-mc -show-inst ?

myhsu updated this revision to Diff 339328.Apr 21 2021, 11:22 AM
  • Add test cases
dmgreen accepted this revision.Apr 25 2021, 12:51 AM

Thanks, LGTM.

llvm/test/MC/Disassembler/ARM/bug49974.arm.txt
1

I would name these tests after the instruction they are testing, not the bug number.

llvm/test/MC/Disassembler/ARM/bug49974.thumb2.txt
15

May be worth adding quick tests for t2SUBspImm with and without S, to make sure they are still doing OK too. They seem fine but it can be good to have the test.

This revision is now accepted and ready to land.Apr 25 2021, 12:51 AM
myhsu updated this revision to Diff 340373.Apr 25 2021, 11:21 AM
myhsu marked 2 inline comments as done.

Update test cases

  • Use tested instructions as the file name.
  • Add tests for t2SUBspImm, which is also affected by this patch
This revision was landed with ongoing or failed builds.Apr 25 2021, 11:58 AM
This revision was automatically updated to reflect the committed changes.