[Verifier] Move some atomicrmw/cmpxchg checks to instruction creation

[aeubanks]

Summary

These checks already exist as asserts when creating the corresponding
instruction. Anybody creating these instructions already need to take
care to not break these checks.

Move the checks for success/failure ordering in cmpxchg from the
verifier to the LLParser and BitcodeReader plus an assert.

Add some tests for cmpxchg ordering. The .bc files are created from the
.ll files with an llvm-as with these checks disabled.

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5146–5149

Does this have test coverage?

[aeubanks]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5146–5149

no, it's the thing mentioned in the description

adding test coverage would require a bitcode file that was generated by a modified LLVM that allows this to bypass the LLParser/verifier

so this probably isn't an issue in practice since it's always been invalid, so there shouldn't be any bitcode out there that hits this
I guess I could just remove this check

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5146–5149

We should still handle invalid bitcode/shouldn't be able to create undefined situations through invalid bitcode.

So I'd say this needs a test - I assume there are other invalid bitcode tests somewhere around ?

Happy to help create one if you like.

[aeubanks]

factor out cmpxchg ordering checks
add more bc tests

[dblaikie]

Looks good, thanks!

llvm/include/llvm/IR/Instructions.h

public:
   /// Return true if this cmpxchg may spuriously fail.
   bool isWeak() const { return getSubclassData<WeakField>(); }
 
   void setWeak(bool IsWeak) { setSubclassData<WeakField>(IsWeak); }
 
   /// Transparently provide more efficient getOperand methods.
   DECLARE_TRANSPARENT_OPERAND_ACCESSORS(Value);
 
+  static bool isValidSuccessOrdering(AtomicOrdering Ordering) {
+    return Ordering != AtomicOrdering::NotAtomic &&
+           Ordering != AtomicOrdering::Unordered;
+  }
+
+  static bool isValidFailureOrdering(AtomicOrdering Ordering) {
+    return Ordering != AtomicOrdering::NotAtomic &&
+           Ordering != AtomicOrdering::Unordered &&
+           Ordering != AtomicOrdering::AcquireRelease &&
+           Ordering != AtomicOrdering::Release;
+  }
+
   /// Returns the success ordering constraint of this cmpxchg instruction.
   AtomicOrdering getSuccessOrdering() const {
     return getSubclassData<SuccessOrderingField>();
   }
 
   /// Sets the success ordering constraint of this cmpxchg instruction.
   void setSuccessOrdering(AtomicOrdering Ordering) {
-    assert(Ordering != AtomicOrdering::NotAtomic &&
-           "CmpXchg instructions can only be atomic.");
+    assert(isValidSuccessOrdering(Ordering) &&
+           "invalid CmpXchg success ordering");
     setSubclassData<SuccessOrderingField>(Ordering);
   }
 
   /// Returns the failure ordering constraint of this cmpxchg instruction.
   AtomicOrdering getFailureOrdering() const {
     return getSubclassData<FailureOrderingField>();
   }
 
   /// Sets the failure ordering constraint of this cmpxchg instruction.
   void setFailureOrdering(AtomicOrdering Ordering) {
-    assert(Ordering != AtomicOrdering::NotAtomic &&
-           "CmpXchg instructions can only be atomic.");
+    assert(isValidFailureOrdering(Ordering) &&
+           "invalid CmpXchg failure ordering");
     setSubclassData<FailureOrderingField>(Ordering);
   }
 
   /// Returns the synchronization scope ID of this cmpxchg instruction.
   SyncScope::ID getSyncScopeID() const {
     return SSID;
   }
 

llvm/lib/AsmParser/LLParser.cpp

 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalIFunc.h"
 #include "llvm/IR/GlobalObject.h"
 #include "llvm/IR/InlineAsm.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/ValueSymbolTable.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
if (parseTypeAndValue(Ptr, PtrLoc, PFS) ||
       parseTypeAndValue(Cmp, CmpLoc, PFS) ||
       parseToken(lltok::comma, "expected ',' after cmpxchg cmp operand") ||
       parseTypeAndValue(New, NewLoc, PFS) ||
       parseScopeAndOrdering(true /*Always atomic*/, SSID, SuccessOrdering) ||
       parseOrdering(FailureOrdering) ||
       parseOptionalCommaAlign(Alignment, AteExtraComma))
     return true;
 
-  if (SuccessOrdering == AtomicOrdering::Unordered ||
-      FailureOrdering == AtomicOrdering::Unordered)
-    return tokError("cmpxchg cannot be unordered");
-  if (FailureOrdering == AtomicOrdering::Release ||
-      FailureOrdering == AtomicOrdering::AcquireRelease)
-    return tokError(
-        "cmpxchg failure ordering cannot include release semantics");
+  if (!AtomicCmpXchgInst::isValidSuccessOrdering(SuccessOrdering))
+    return tokError("invalid cmpxchg success ordering");
+  if (!AtomicCmpXchgInst::isValidFailureOrdering(FailureOrdering))
+    return tokError("invalid cmpxchg failure ordering");
   if (!Ptr->getType()->isPointerTy())
     return error(PtrLoc, "cmpxchg operand must be a pointer");
   if (!cast<PointerType>(Ptr->getType())
            ->isOpaqueOrPointeeTypeMatches(Cmp->getType()))
     return error(CmpLoc, "compare value and pointer type do not match");
   if (!cast<PointerType>(Ptr->getType())
            ->isOpaqueOrPointeeTypeMatches(New->getType()))
     return error(NewLoc, "new value and pointer type do not match");

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::FUNC_CODE_INST_CMPXCHG_OLD: {
       if (Error Err = typeCheckLoadStoreInst(Cmp->getType(), Ptr->getType()))
         return Err;
 
       const AtomicOrdering FailureOrdering =
           NumRecords < 7
               ? AtomicCmpXchgInst::getStrongestFailureOrdering(SuccessOrdering)
               : getDecodedOrdering(Record[OpNum + 3]);
 
+      if (FailureOrdering == AtomicOrdering::NotAtomic ||
+          FailureOrdering == AtomicOrdering::Unordered)
+        return error("Invalid record");
+
       const Align Alignment(
           TheModule->getDataLayout().getTypeStoreSize(Cmp->getType()));
 
       I = new AtomicCmpXchgInst(Ptr, Cmp, New, Alignment, SuccessOrdering,
                                 FailureOrdering, SSID);
       cast<AtomicCmpXchgInst>(I)->setVolatile(Record[OpNum]);
       FullTy = StructType::get(Context, {FullTy, Type::getInt1Ty(Context)});
 
case bitc::FUNC_CODE_INST_CMPXCHG: {
 
       if (NumRecords < OpNum + 3 || NumRecords > OpNum + 6)
         return error("Invalid record");
 
       const bool IsVol = Record[OpNum];
 
       const AtomicOrdering SuccessOrdering =
           getDecodedOrdering(Record[OpNum + 1]);
-      if (SuccessOrdering == AtomicOrdering::NotAtomic ||
-          SuccessOrdering == AtomicOrdering::Unordered)
-        return error("Invalid record");
+      if (!AtomicCmpXchgInst::isValidSuccessOrdering(SuccessOrdering))
+        return error("Invalid cmpxchg success ordering");
 
       const SyncScope::ID SSID = getDecodedSyncScopeID(Record[OpNum + 2]);
 
       if (Error Err = typeCheckLoadStoreInst(Cmp->getType(), Ptr->getType()))
         return Err;
 
       const AtomicOrdering FailureOrdering =
           getDecodedOrdering(Record[OpNum + 3]);
+      if (!AtomicCmpXchgInst::isValidFailureOrdering(FailureOrdering))
+        return error("Invalid cmpxchg failure ordering");
 
       const bool IsWeak = Record[OpNum + 4];
 
       MaybeAlign Alignment;
 
       if (NumRecords == (OpNum + 6)) {
         if (Error Err = parseAlignmentValue(Record[OpNum + 5], Alignment))
           return Err;

llvm/lib/IR/Instructions.cpp

void AtomicCmpXchgInst::Init(Value *Ptr, Value *Cmp, Value *NewVal,
   assert(cast<PointerType>(getOperand(0)->getType())
              ->isOpaqueOrPointeeTypeMatches(getOperand(1)->getType()) &&
          "Ptr must be a pointer to Cmp type!");
   assert(cast<PointerType>(getOperand(0)->getType())
              ->isOpaqueOrPointeeTypeMatches(getOperand(2)->getType()) &&
          "Ptr must be a pointer to NewVal type!");
   assert(getOperand(1)->getType() == getOperand(2)->getType() &&
          "Cmp type and NewVal type must be same!");
-  assert(SuccessOrdering != AtomicOrdering::NotAtomic &&
-         "AtomicCmpXchg instructions must be atomic!");
-  assert(FailureOrdering != AtomicOrdering::NotAtomic &&
-         "AtomicCmpXchg instructions must be atomic!");
-  assert(FailureOrdering != AtomicOrdering::Release &&
-         FailureOrdering != AtomicOrdering::AcquireRelease &&
-         "AtomicCmpXchg failure ordering cannot include release semantics");
 }
 
 AtomicCmpXchgInst::AtomicCmpXchgInst(Value *Ptr, Value *Cmp, Value *NewVal,
                                      Align Alignment,
                                      AtomicOrdering SuccessOrdering,
                                      AtomicOrdering FailureOrdering,
                                      SyncScope::ID SSID,
                                      Instruction *InsertBefore)

llvm/lib/IR/Verifier.cpp

void Verifier::visitAllocaInst(AllocaInst &AI) {
   if (AI.isSwiftError()) {
     verifySwiftErrorValue(&AI);
   }
 
   visitInstruction(AI);
 }
 
 void Verifier::visitAtomicCmpXchgInst(AtomicCmpXchgInst &CXI) {
-
-  // FIXME: more conditions???
-  Assert(CXI.getSuccessOrdering() != AtomicOrdering::NotAtomic,
-         "cmpxchg instructions must be atomic.", &CXI);
-  Assert(CXI.getFailureOrdering() != AtomicOrdering::NotAtomic,
-         "cmpxchg instructions must be atomic.", &CXI);
-  Assert(CXI.getSuccessOrdering() != AtomicOrdering::Unordered,
-         "cmpxchg instructions cannot be unordered.", &CXI);
-  Assert(CXI.getFailureOrdering() != AtomicOrdering::Unordered,
-         "cmpxchg instructions cannot be unordered.", &CXI);
-  Assert(CXI.getFailureOrdering() != AtomicOrdering::Release &&
-             CXI.getFailureOrdering() != AtomicOrdering::AcquireRelease,
-         "cmpxchg failure ordering cannot include release semantics", &CXI);
-
-  PointerType *PTy = dyn_cast<PointerType>(CXI.getOperand(0)->getType());
-  Assert(PTy, "First cmpxchg operand must be a pointer.", &CXI);
   Type *ElTy = CXI.getOperand(1)->getType();
-  Assert(PTy->isOpaqueOrPointeeTypeMatches(ElTy),
-         "Expected value type does not match pointer operand type!", &CXI,
-         ElTy);
-  Assert(ElTy == CXI.getOperand(2)->getType(),
-         "Stored value type does not match expected value operand type!", &CXI,
-         ElTy);
   Assert(ElTy->isIntOrPtrTy(),
          "cmpxchg operand must have integer or pointer type", ElTy, &CXI);
   checkAtomicMemAccessSize(ElTy, &CXI);
   visitInstruction(CXI);
 }
 
 void Verifier::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
-  Assert(RMWI.getOrdering() != AtomicOrdering::NotAtomic,
-         "atomicrmw instructions must be atomic.", &RMWI);
   Assert(RMWI.getOrdering() != AtomicOrdering::Unordered,
          "atomicrmw instructions cannot be unordered.", &RMWI);
   auto Op = RMWI.getOperation();
-  PointerType *PTy = dyn_cast<PointerType>(RMWI.getOperand(0)->getType());
-  Assert(PTy, "First atomicrmw operand must be a pointer.", &RMWI);
   Type *ElTy = RMWI.getOperand(1)->getType();
   if (Op == AtomicRMWInst::Xchg) {
     Assert(ElTy->isIntegerTy() || ElTy->isFloatingPointTy(), "atomicrmw " +
            AtomicRMWInst::getOperationName(Op) +
            " operand must have integer or floating point type!",
            &RMWI, ElTy);
   } else if (AtomicRMWInst::isFPOperation(Op)) {
     Assert(ElTy->isFloatingPointTy(), "atomicrmw " +
            AtomicRMWInst::getOperationName(Op) +
            " operand must have floating point type!",
            &RMWI, ElTy);
   } else {
     Assert(ElTy->isIntegerTy(), "atomicrmw " +
            AtomicRMWInst::getOperationName(Op) +
            " operand must have integer type!",
            &RMWI, ElTy);
   }
   checkAtomicMemAccessSize(ElTy, &RMWI);
-  Assert(PTy->isOpaqueOrPointeeTypeMatches(ElTy),
-         "Argument value type does not match pointer operand type!", &RMWI,
-         ElTy);
   Assert(AtomicRMWInst::FIRST_BINOP <= Op && Op <= AtomicRMWInst::LAST_BINOP,
          "Invalid binary operation!", &RMWI);
   visitInstruction(RMWI);
 }
 
 void Verifier::visitFenceInst(FenceInst &FI) {
   const AtomicOrdering Ordering = FI.getOrdering();
   Assert(Ordering == AtomicOrdering::Acquire ||

llvm/test/Assembler/cmpxchg-ordering-2.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+define void @f(i32* %a, i32 %b, i32 %c) {
+; CHECK: invalid cmpxchg failure ordering
+  %x = cmpxchg i32* %a, i32 %b, i32 %c acq_rel unordered
+  ret void
+}

llvm/test/Assembler/cmpxchg-ordering-3.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+define void @f(i32* %a, i32 %b, i32 %c) {
+; CHECK: invalid cmpxchg failure ordering
+  %x = cmpxchg i32* %a, i32 %b, i32 %c acq_rel acq_rel
+  ret void
+}

llvm/test/Assembler/cmpxchg-ordering-4.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+define void @f(i32* %a, i32 %b, i32 %c) {
+; CHECK: invalid cmpxchg failure ordering
+  %x = cmpxchg i32* %a, i32 %b, i32 %c acq_rel release
+  ret void
+}

llvm/test/Assembler/cmpxchg-ordering.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+define void @f(i32* %a, i32 %b, i32 %c) {
+; CHECK: invalid cmpxchg success ordering
+  %x = cmpxchg i32* %a, i32 %b, i32 %c unordered monotonic
+  ret void
+}

llvm/test/Bitcode/invalid.test

 RUN:   FileCheck --check-prefix=NONPOINTER-ATOMICRMW %s
 
 NONPOINTER-ATOMICRMW: Invalid record
 
 RUN: not llvm-dis -disable-output %p/Inputs/invalid-fcmp-opnum.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=INVALID-FCMP-OPNUM %s
 
 INVALID-FCMP-OPNUM: Invalid record: operand number exceeded available operands
+
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-cmpxchg-ordering.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=CMPXCHG-ORDERING %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-cmpxchg-ordering-2.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=CMPXCHG-ORDERING %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-cmpxchg-ordering-3.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=CMPXCHG-ORDERING %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-cmpxchg-ordering-4.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=CMPXCHG-ORDERING %s
+
+CMPXCHG-ORDERING: Invalid cmpxchg {{failure|success}} ordering