[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

Nice catch.

I had a look at https://lists.llvm.org/pipermail/llvm-dev/2021-March/149216.html, and http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf.
Your change makes sense to me.

However, I'm a bit worried that other parts of the analyzer might suffer from the same phenomenon. When I grepped for `isIntegralOrEnumerationType()`, there were like 45 mentions.
I guess, you will worry about it more than I do, but I'd recommend you to have a look at them. You might be able to craft even more crashes.

PS: I was a bit shocked that we don't have any tests mentioning `_Accum` and `_Fract`.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:155-156
 
-    assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+    assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+           Loc::isLocType(T));
     return APSIntType(Ctx.getIntWidth(T),
----------------
I was thinking of using `isFixedPointOrIntegerType()`. However, that would not accept //scoped enums//.
```lang=c++
inline bool Type::isFixedPointOrIntegerType() const {
  return isFixedPointType() || isIntegerType();
}

inline bool Type::isIntegerType() const {
  if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))
    return BT->getKind() >= BuiltinType::Bool &&
           BT->getKind() <= BuiltinType::Int128;
  if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) {
    // Incomplete enum types are not treated as integer types.
    // FIXME: In C++, enum types are never integer types.
    return IsEnumDeclComplete(ET->getDecl()) &&
      !IsEnumDeclScoped(ET->getDecl());     // <------------ rejects scoped enums :(
  }
  return isBitIntType();
}

inline bool Type::isIntegralOrEnumerationType() const {
  if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))
    return BT->getKind() >= BuiltinType::Bool &&
           BT->getKind() <= BuiltinType::Int128;

  // Check for a complete enum type; incomplete enum types are not properly an
  // enumeration type in the sense required here.
  if (const auto *ET = dyn_cast<EnumType>(CanonicalType))
    return IsEnumDeclComplete(ET->getDecl());     // <-------- accepts scoped enums!

  return isBitIntType();
}
```

It looked so neat at first. Ah. I just had to share this.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+           Loc::isLocType(T));
     return APSIntType(Ctx.getIntWidth(T),
                       !T->isSignedIntegerOrEnumerationType());
   }
----------------
I don't think you are supposed to call `isSignedIntegerOrEnumerationType()` if you have a //fixed-point// type.
```lang=C++
inline bool Type::isSignedFixedPointType() const {
  if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) {
    return ((BT->getKind() >= BuiltinType::ShortAccum &&
             BT->getKind() <= BuiltinType::LongAccum) ||
            (BT->getKind() >= BuiltinType::ShortFract &&
             BT->getKind() <= BuiltinType::LongFract) ||
            (BT->getKind() >= BuiltinType::SatShortAccum &&
             BT->getKind() <= BuiltinType::SatLongAccum) ||
            (BT->getKind() >= BuiltinType::SatShortFract &&
             BT->getKind() <= BuiltinType::SatLongFract));
  }
  return false;
}
```
By looking at the implementation of this, I don't think you could substitute that with `isSignedIntegerOrEnumerationType()`.
Am I wrong about this?

Please demonstrate this by tests.


================
Comment at: clang/test/Analysis/fixed-point.c:6-9
+long a(int c) {                                                                       
+  (long _Accum) c >> 4;
+  return c;
+}  
----------------
It had a few extra spaces. And by casting it to void we could eliminate `-Wno-unused` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759