[NFC][CLANG] Fix Static Code Analysis Concerns

[Manna]

Summary

Reported by Static Analyzer Tool, Coverity:

Bad bit shift operation
The operation may have an undefined behavior or yield an unexpected result.

In <unnamed>::​SVEEmitter::​encodeFlag(unsigned long long, llvm::​StringRef): A bit shift operation has a shift amount which is too large or has a negative value.

// Returns the SVETypeFlags for a given value and mask.
  uint64_t encodeFlag(uint64_t V, StringRef MaskName) const {
    auto It = FlagTypes.find(MaskName);
   	//Condition It != llvm::StringMap<unsigned long long, llvm::MallocAllocator>::const_iterator const(this->FlagTypes.end()), taking true branch.
    if (It != FlagTypes.end()) {
      uint64_t Mask = It->getValue();
      //return_constant: Function call llvm::countr_zero(Mask) may return 64.
      //assignment: Assigning: Shift = llvm::countr_zero(Mask). The value of Shift is now 64.
      unsigned Shift = llvm::countr_zero(Mask);
   	
     //Bad bit shift operation (BAD_SHIFT)
     //large_shift: In expression V << Shift, left shifting by more than 63 bits has undefined behavior. The shift amount, Shift, is 64.
      return (V << Shift) & Mask;
    }
    llvm_unreachable("Unsupported flag");
  }

The only invalid value for Shift is 64 since a zero Mask is the only case where llvm::countr_zero will end up being 64.

This patch adds an assert that Mask != 0, since that would be an invalid mask.

[erichkeane]

clang/utils/TableGen/SveEmitter.cpp

302

Shouldn't this be: assert(Shift < 64 &&"...")?

expr.shift (https://eel.is/c++draft/expr.shift) says:

The operands shall be of integral or unscoped enumeration type and integral promotions are performed.
The type of the result is that of the promoted left operand.
The behavior is undefined if the right operand is negative, or greater than or equal to the width of the promoted left operand.

uint64 stays as an unsigned long, so it is still 64 bits, so the only invalid value for Shift is 64 (though >64 is 'nonsense', but only impossible because of llvm::countr_zero).

One thing to consider: I wonder if we should instead be changing the 'shift' to be:

(V << (Shift % 64)) && Mask ? It looks like arm_sve.td has the NoFlags value as zero, which I think will end up going through here possibly (or at least, inserted into FlagTypes.

So I suspect an assert might not be sufficient, since a 64 bit shift is possible in that case (since a zero 'Mask' is the only case where countr_zero will end up being 64).

[sdesmalen]

clang/utils/TableGen/SveEmitter.cpp

302

So I suspect an assert might not be sufficient, since a 64 bit shift is possible in that case (since a zero 'Mask' is the only case where countr_zero will end up being 64).

It should be fine to assert that Mask != 0, since that would be an invalid mask.

[erichkeane]

clang/utils/TableGen/SveEmitter.cpp

302

Thanks for the comment @sdesmalen! Is there something that prevents the NoFlags from being passed as the MaskName here?

[sdesmalen]

clang/utils/TableGen/SveEmitter.cpp

302

There's nothing that actively prevents it, but encodeFlag is a utility function that has no uses outside this file and has only 4 uses. Adding an assert should be sufficient.

[Manna]

Thank you for reviews and comments @erichkeane and @sdesmalen! I have updated patch.

[Manna]

clang/utils/TableGen/SveEmitter.cpp

302

Thank you for the explanation!

[tahonermann]

clang/utils/TableGen/SveEmitter.cpp

302

I'm not sure if asserting Mask != 0 will suffice to silence Coverity. While Coverity can specifically observe that countr_zero might return 0 (because TrailingZerosCounter<T, 8>::count() has a return 64 statement), I don't think Coverity can determine that the function can't return 65 or higher. I think Erich's initial intuition is correct; the concern that Coverity is reporting is that the shift might overflow, so that is what should be guarded.

assert(Shift < 64 && "Mask value produced an invalid shift value");

[Manna]

Fix Coverity complain about shift overflow issue

[Manna]

clang/utils/TableGen/SveEmitter.cpp

302

Thanks @tahonermann for comments. I have updated my patch to guard the shift overflow issue with Mask value.

[tahonermann]

Looks good to me!

[Manna]

In D150140#4333552, @tahonermann wrote:

Looks good to me!

Thank you @tahonermann for reviews!

[Manna]

Thank you everyone for comments and reviews!

clang/utils/TableGen/SveEmitter.cpp

public:
   }
 
   // Returns the SVETypeFlags for a given value and mask.
   uint64_t encodeFlag(uint64_t V, StringRef MaskName) const {
     auto It = FlagTypes.find(MaskName);
     if (It != FlagTypes.end()) {
       uint64_t Mask = It->getValue();
       unsigned Shift = llvm::countr_zero(Mask);
+      assert(Shift < 64 && "Mask value produced an invalid shift value");
       return (V << Shift) & Mask;
     }
     llvm_unreachable("Unsupported flag");
   }
 
   // Returns the SVETypeFlags for the given element type.
   uint64_t encodeEltType(StringRef EltName) {
     auto It = EltTypes.find(EltName);