NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+ if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+ CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+ // If a variable is reinterpreted as a type that doesn't fit into a larger
+ // type evenly, round it down.
+ // This is a signed value, since it's used in arithmetic with signed
+ // indices.
----------------
Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > And then remove the manual division.
> > > Hmpf.
> > >
> > > ```
> > > Failing Tests (7):
> > > Clang :: Analysis/misc-ps-region-store.m
> > > Clang :: Analysis/mpichecker.cpp
> > > Clang :: Analysis/outofbound.c
> > > Clang :: Analysis/rdar-6541136-region.c
> > > Clang :: Analysis/return-ptr-range.cpp
> > > Clang :: Analysis/track-control-dependency-conditions.cpp
> > > Clang :: Analysis/uninit-vals.c
> > > ```
> > >
> > > I would pick that solution because it may be a tiny-bit faster, and then
> > > later on investigate this issue when we model more about dynamic sizes.
> > Soooooo what does it tell us about the correctness of the new
> > `evalBinOp`-based solution?
> So, when I tried to inject an `APSInt` it converted to `0` so division by
> zero made that. I felt that the implicit conversion is wonky, but dividing by
> 0, ugh.
Yay, great job figuring this out!
Also the conversion wasn't implicit; you explicitly specified
`llvm::APSInt(...)`. I agree that this constructor is evil, though.
================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:44-47
+ if (auto DV = DivisionV.getAs<DefinedOrUnknownSVal>())
+ return *DV;
+
+ return UnknownVal();
----------------
I'd rather do a `castAs` here. Allocating a region of garbage size should be an
immediate warning; supplying a zero-size `ElementTy` should be an immediate
crash; in all other cases the result of division must be defined.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69599/new/
https://reviews.llvm.org/D69599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
