Page MenuHomePhabricator

ARM: Tighten up disassembling of MSR mask for M-class
ClosedPublic

Authored by petpav01 on Jul 28 2014, 2:33 AM.

Details

Reviewers
jmolloy
petpav01
Summary

Allow only disassembling of M-class MSR masks that the assembler knows how to assemble back.

Note: The current code in DecodeMSRMask() rejects the unpredictable A/R MSR mask '0000' with Fail. The code in the patch follows this style and rejects unpredictable M-class MSR masks also with Fail (instead of SoftFail). If SoftFail is preferred in this case then additional changes to ARMInstPrinter (to print non-symbolic masks) and ARMAsmParser (to parse non-symbolic masks) will be needed.

Diff Detail

Event Timeline

petpav01 updated this revision to Diff 11940.Jul 28 2014, 2:33 AM
petpav01 retitled this revision from to ARM: Tighten up disassembling of MSR mask for M-class.
petpav01 updated this object.
petpav01 edited the test plan for this revision. (Show Details)
petpav01 added a project: deleted.
petpav01 added a subscriber: Unknown Object (MLST).

Hi Petr,

Thanks for working on this. As far as the softfail stuff goes, we're not too strict on that and there are bigger problems to solve before UNPREDICTABLE all works correctly, so let's not worry about that for the moment.

I do have a few specific comments, below.

Cheers,

James

lib/Target/ARM/Disassembler/ARMDisassembler.cpp
3999

Don't need the braces here.

4009

MSR only appears to have one encoding. What are you trying to achieve by checking the opcode here?

4010

Mask is two bits, so there should be a & 3 here.

4011

Several points here:

  • Please add more documentation as to what this condition is supposed to check. I think I've reverse engineered it from the ARMARM but I shouldn't have to.
  • The mask values 0b01 and 0b11 are predicated on the DSP extension in the ARMARM. I *believe* we do model this extension in LLVM, so you should check it here.
petpav01 updated this revision to Diff 11942.Jul 28 2014, 5:33 AM

Hi James,

Thank you for reviewing this. I am attaching a new patch to address your comments.

================
Comment at: lib/Target/ARM/Disassembler/ARMDisassembler.cpp:4009
@@ +4008,3 @@
+ // Validate the 2-bit mask value in MSR for v7m.
+ if ((FeatureBits & ARM::HasV7Ops) && Inst.getOpcode() == ARM::t2MSR_M) {
+ unsigned Mask = Val >> 10;


MSR only appears to have one encoding. What are you trying to achieve by checking the opcode here?

This code path (function DecodeMSRMask()) is also used for the MRS instruction.
In that case, the 2-bit mask value will be always zero (because the MRS
instruction has no mask) and no mask validation should be done.

Thanks,
Petr

petpav01 accepted this revision.Aug 1 2014, 7:38 AM
petpav01 added a reviewer: petpav01.

r214505

This revision is now accepted and ready to land.Aug 1 2014, 7:38 AM
petpav01 closed this revision.Aug 1 2014, 7:38 AM