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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked an inline comment as done.
vabridgers added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+           Loc::isLocType(T));
     return APSIntType(Ctx.getIntWidth(T),
                       !T->isSignedIntegerOrEnumerationType());
   }
----------------
steakhal wrote:
> vabridgers wrote:
> > steakhal wrote:
> > > 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.
> > I tried using isSignedIntegerOrEnumerationType() instead of (T->isIntegralOrEnumerationType() || T->isFixedPointType() ... ), but got the same assert :/  
> > 
> > I corrected the formatting and expanded the test cases. 
> Is hould have clarified, sorry.
> 
> My point is that for constructing the APSIntType, we calculate the bitwidth and the signedness.
> 
> My problem is that the calculation is wrong for the signedness in case we have a signed fixedpointtype.
> It is wrong because we reach `isSignedIntegerOrEnumerationType()` with a fixedpoint type. For that even though its signed, it will return false!
> 
> And in the end we will have an APSIntType with the wrong signednss.
> 
> So my point is that we should probably handle fixedpoint types separately to have a distict return statement for it.
> But im jumping to the solution, what I originally wanted to highlight was this.
> That was why I requested changes.
> And this is what I wanted to see some how intests, but I wont insist if its too difficult to craft.
In retrospect, your original comment was clear. I did not fully comprehend the comment. 

I'll explore a few options. I checked the unittests for a "reference", and it seems our static analysis unittest coverage is sparse. Not sure I need to go that far, but I'll explore the possibility of crafting a unittest for this case that demonstrates the problem and solution. 

Thanks again - best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759