Fix bug in MachineInstr::hasPropertyInBundle

[mikael]

Summary

The MCID::Flag enumeration now has more than 32 items, this means that
the hasPropertyBundle argument 'Mask' can overflow.

This patch changes the argument to be 64 bits instead.

[spatel]

I don't have much experience with this code, so adding more potential reviewers.
It should be possible to show the failure with a test if we're overflowing?

[mikael]

I'm not sure it can be reproduced in an in-tree target. But it is easy to give you an example when this happens:

You would need a target that uses bundle instructions and uses one of the flags 'Convergent', 'Trap' or 'Add'. If you call e.g MachineInstr.h:isConvergent on bundles the issue occurs.

Following the code path for isConvergent leads us to the following code snippet:

// MCFlag == 32 because Convergent == 32
return hasPropertyInBundle(1ULL << MCFlag, Type);

The resulting value of the shift does not fit in the 'Mask' argument.

[bjope]

It seems reasonable to match the type of llvm::MCInstrDesc::getFlags() which is uint64_t nowadays. The hasPropertyInBundle method is private to MachineInstr, and afaict only used from MachineInstr::hasProperty. This is kind of a no-brainer to me, and I think we do not need a regression test here.

Although, one idea is to add an assert that MCFlag<64 in MachineInstr::hasProperty.
Another idea could be to add an assert that "Mask != 0" in MachineInstr::hasPropertyInBundle.
And maybe we should have a static (compile time) assert somewhere that the highest value in MCID::Flag is less than 64.

So I think this patch looks good as-is. But feel free to for example add the assert on MCFlag<64 in MachineInstr::hasProperty. I assume that could have helped in detecting this fault?

A small NIT on the first line in the commit message is that it usually is written in "imperative mood" (see https://chris.beams.io/posts/git-commit/#imperative). I don't say that it is mentioned in the dev policy (https://llvm.org/docs/DeveloperPolicy.html#commit-messages) but I think it is common practice.

[mikael]

Updated the patch to include suggestions from @bjope

[bjope]

include/llvm/CodeGen/MachineInstr.h

587 ↗

(On Diff #163794)

Asserts should include an error message (see https://llvm.org/docs/CodingStandards.html#assert-liberally).

I think that something like this

`assert(MCFlag < 64 && "MCFlag out of range for bit mask in getFlags/hasPropertyInBundle.");`

also would explain why we require the value to be below 64.

[mikael]

Added the suggested assertion comment.

[mikael]

@bjope, are you happy with the last diff?

[bjope]

In D51596#1225577, @mikael wrote:

@bjope, are you happy with the last diff?

Yes, LGTM.

Do you have commit access, or do you need help with landing this?

[mikael]

I don't have commit access yet, I would like to have it at some point, but for now I'd be happy if someone could push it for me.

[svenvh]

Sure, I'll push it.

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

public:
   };
 
   /// Return true if the instruction (or in the case of a bundle,
   /// the instructions inside the bundle) has the specified property.
   /// The first argument is the property being queried.
   /// The second argument indicates whether the query should look inside
   /// instruction bundles.
   bool hasProperty(unsigned MCFlag, QueryType Type = AnyInBundle) const {
+    assert(MCFlag < 64 &&
+           "MCFlag out of range for bit mask in getFlags/hasPropertyInBundle.");
     // Inline the fast path for unbundled or bundle-internal instructions.
     if (Type == IgnoreBundle || !isBundled() || isBundledWithPred())
       return getDesc().getFlags() & (1ULL << MCFlag);
 
     // If this is the first instruction in a bundle, take the slow path.
     return hasPropertyInBundle(1ULL << MCFlag, Type);
   }
 
private:
   void RemoveRegOperandsFromUseLists(MachineRegisterInfo&);
 
   /// Add all of the register operands in this instruction from their
   /// respective use lists.  This requires that the operands not be on their
   /// use lists yet.
   void AddRegOperandsToUseLists(MachineRegisterInfo&);
 
   /// Slow path for hasProperty when we're dealing with a bundle.
-  bool hasPropertyInBundle(unsigned Mask, QueryType Type) const;
+  bool hasPropertyInBundle(uint64_t Mask, QueryType Type) const;
 
   /// Implements the logic of getRegClassConstraintEffectForVReg for the
   /// this MI and the given operand index \p OpIdx.
   /// If the related operand does not constrained Reg, this returns CurRC.
   const TargetRegisterClass *getRegClassConstraintEffectForVRegImpl(
       unsigned OpIdx, unsigned Reg, const TargetRegisterClass *CurRC,
       const TargetInstrInfo *TII, const TargetRegisterInfo *TRI) const;
 };

llvm/trunk/lib/CodeGen/MachineInstr.cpp

 }
 
 uint16_t MachineInstr::mergeFlagsWith(const MachineInstr &Other) const {
   // For now, the just return the union of the flags. If the flags get more
   // complicated over time, we might need more logic here.
   return getFlags() | Other.getFlags();
 }
 
-bool MachineInstr::hasPropertyInBundle(unsigned Mask, QueryType Type) const {
+bool MachineInstr::hasPropertyInBundle(uint64_t Mask, QueryType Type) const {
   assert(!isBundledWithPred() && "Must be called on bundle header");
   for (MachineBasicBlock::const_instr_iterator MII = getIterator();; ++MII) {
     if (MII->getDesc().getFlags() & Mask) {
       if (Type == AnyInBundle)
         return true;
     } else {
       if (Type == AllInBundle && !MII->isBundle())
         return false;