[PATCH] D63973: [MachineVerifier] Improve checks of target instructions operands.

Tue Jul 2 06:13:41 PDT 2019

jonpa updated this revision to Diff 207533.
jonpa marked 3 inline comments as done.
jonpa added a comment.

Thank you for review.

> Do you really need this check? I would expect this to work naturally with generic instruction operands

OK, I removed that check and got some new errors:

  FAIL: LLVM :: CodeGen/MIR/X86/liveout-register-mask.mir
  *** Bad machine code: Expected a non-register operand. ***
  - function:    small_patchpoint_codegen
  - basic block: %bb.0 entry (0x5d637d8)
  - instruction: PATCHPOINT 5, 5, 0, 2, 0, $rdi, $rsi, <regmask $bh $bl $bp $bph $bpl $bx $ebp $ebx $hbp $hbx $rbp $rbx $r12 $r13 $r14 $r15 $r12b $r13b $r14b $r15b $r12bh $r13bh $r14bh $r15bh $r12d $r13d $r14d $r15d $r12w $r13w $r14w $r15w $r12wh and 3 more...>, liveout($esp, $rsp, $sp, $spl), implicit\
  -def dead early-clobber $r11, implicit-def $rsp, implicit-def dead $rax
  - operand 5:   $rdi
  
  FAIL: LLVM :: CodeGen/PowerPC/ppc64-anyregcc.ll
  *** Bad machine code: Expected a non-register operand. ***
  - function:    test
  - basic block: %bb.0 entry (0x5d8a0e8)
  - instruction: PATCHPOINT 0, 40, 0, 2, 13, killed %1:gprc, killed %0:gprc, 2, 3, <regmask $cr0 $cr1 $cr2 $cr3 $cr4 $cr5 $cr6 $cr7 $f0 $f1 $f2 $f3 $f4 $f5 $f6 $f7 $f8 $f9 $f10 $f11 $f12 $f13 $f14 $f15 $f16 $f17 $f18 $f19 $f20 $f21 $f22 $f23 $f24 and 93 more...>, implicit-def dead early-clobber $x12, i\
  mplicit-def dead early-clobber $lr8, implicit-def dead early-clobber $ctr8, implicit-def $r1, implicit $x2
  - operand 5:   killed %1:gprc
  LLVM ERROR: Found 1 machine code errors.
  
  ... and more PATCHPOINT fails...

PATCHPOINT has a 'variable_ops' operand, where %1:gprc is the first optional operand. So I added a guard against optional operands, and there were then just two additional test failures compared to before:

  FAIL: LLVM :: CodeGen/X86/xray-custom-log.ll
  *** Bad machine code: Expected a non-register operand. ***
  - function:    fn
  - basic block: %bb.0  (0x5da5768)
  - instruction: PATCHABLE_EVENT_CALL killed %1:gr64, killed %0:gr32
  - operand 1:   killed %0:gr32
  LLVM ERROR: Found 1 machine code errors.

PATCHABLE_EVENT_CALL is declared to have an immediate second source operand, where in this case a register operand has been added instead.

  FAIL: LLVM :: CodeGen/X86/xray-typed-event-log.ll
  *** Bad machine code: Expected a non-register operand. ***
  - function:    fn
  - basic block: %bb.0  (0x5da5988)
  - instruction: PATCHABLE_TYPED_EVENT_CALL killed %2:gr16, killed %1:gr64, killed %0:gr32
  - operand 0:   killed %2:gr16
  
  *** Bad machine code: Expected a non-register operand. ***
  - function:    fn
  - basic block: %bb.0  (0x5da5988)
  - instruction: PATCHABLE_TYPED_EVENT_CALL killed %2:gr16, killed %1:gr64, killed %0:gr32
  - operand 2:   killed %0:gr32

PATCHABLE_TYPED_EVENT_CALL is supposed to have as the first source operand an immedate per the declaration.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63973/new/

https://reviews.llvm.org/D63973

Files:
  lib/CodeGen/MachineVerifier.cpp
  test/MachineVerifier/verify-regops.mir


Index: test/MachineVerifier/verify-regops.mir
===================================================================

--- /dev/null
+++ test/MachineVerifier/verify-regops.mir
@@ -0,0 +1,37 @@
+# RUN: not llc -march=x86 -o - %s -run-pass=none -verify-machineinstrs \
+# RUN:   2>&1 | FileCheck %s
+# REQUIRES: x86-registered-target
+#
+# Check that MachineVerifier catches corrupt operands where MO->isReg()
+# returns true, but the descriptor says it should be an OPERAND_IMMEDIATE or
+# OPERAND_PCREL. Conversely, if MO->isReg() (and MO->isFI()) returns false,
+# check that not an OPERAND_REGISTER is expected.
+
+# CHECK-LABEL: fun
+
+# CHECK: *** Bad machine code: Expected a register operand. ***
+# CHECK: - instruction: %1:gr32 = XOR32rm -1, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
+# CHECK: - operand 1:   -1
+
+# CHECK: *** Bad machine code: Expected a non-register operand. ***
+# CHECK: - instruction: %2:gr32 = OR32ri %1:gr32(tied-def 0), %0:gr32, implicit-def dead $eflags
+# CHECK: - operand 2:   %0:gr32
+
+
+name:            fun
+tracksRegLiveness: true
+fixedStack:
+  - { id: 1, offset: 8, size: 4, alignment: 8, isImmutable: true }
+  - { id: 3, size: 4, alignment: 16, isImmutable: true }
+body:             |
+  bb.0:
+    %0:gr32 = MOV32rm %fixed-stack.3, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.3, align 16)
+    ; Was: %1:gr32 = XOR32rm %0, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
+    %1:gr32 = XOR32rm -1, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
+    ; Was: %2:gr32 = OR32ri %1, -256, implicit-def dead $eflags
+    %2:gr32 = OR32ri %1, %0, implicit-def dead $eflags
+    %3:gr32 = MOV32ri -1
+    $eax = COPY %2
+    $edx = COPY %3
+    RET 0, $eax, $edx
+...
Index: lib/CodeGen/MachineVerifier.cpp
===================================================================
--- lib/CodeGen/MachineVerifier.cpp
+++ lib/CodeGen/MachineVerifier.cpp
@@ -1499,14 +1499,24 @@
     const MCOperandInfo &MCOI = MCID.OpInfo[MONum];
     // Don't check if it's the last operand in a variadic instruction. See,
     // e.g., LDM_RET in the arm back end.
-    if (MO->isReg() &&
-        !(MI->isVariadic() && MONum == MCID.getNumOperands()-1)) {
+    bool IsOptional = (MI->isVariadic() && MONum == MCID.getNumOperands() - 1);
+    if (MO->isReg() && !IsOptional) {
       if (MO->isDef() && !MCOI.isOptionalDef())
         report("Explicit operand marked as def", MO, MONum);
       if (MO->isImplicit())
         report("Explicit operand marked as implicit", MO, MONum);
     }
 
+    // Check that an instruction has register operands only as expected.
+    if (!IsOptional) {
+      if (MCOI.OperandType == MCOI::OPERAND_REGISTER &&
+          (!MO->isReg() && !MO->isFI()))
+        report("Expected a register operand.", MO, MONum);
+      if ((MCOI.OperandType == MCOI::OPERAND_IMMEDIATE ||
+           MCOI.OperandType == MCOI::OPERAND_PCREL) && MO->isReg())
+        report("Expected a non-register operand.", MO, MONum);
+    }
+
     int TiedTo = MCID.getOperandConstraint(MONum, MCOI::TIED_TO);
     if (TiedTo != -1) {
       if (!MO->isReg())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D63973.207533.patch
Type: text/x-patch
Size: 3267 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190702/951c17f5/attachment-0001.bin>
