[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());
   }
----------------
vabridgers wrote:
> 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!
Thanks for your insightful comments, in retrospect I should have dug into this more from the very beginning. Turns out there is an API get the fixed point sign I should be using, and creating a simple unittest had value in working this out (imagine that ! :) ). 

So, I updated with the correction and a simple unittest. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759