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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
