xazax.hun added a comment.

In https://reviews.llvm.org/D30489#689947, @zaks.anna wrote:

> Should we just remove ArrayBoundChecker.cpp or is there a value in keeping it 
> around?

I think once ArrayBoundCheckerV2 is in production we should only keep 
ArrayBoundChecker there as educational purpuses for now. However, in the 
future, as the constraint manager and the core analyzer gets smarter, the V2 
checker might get more and more similar to the original one. Once this patch is 
accepted, I might run the analyzer with V2 check enabled on some open source 
projects. It would be great to have it moved out of alpha soon.

>> If no, an alternative approach would be to properly set the constraints on 
>> the extent of the VLA's memory region.
> For information I tried to set the extent for the VLA.. by copy/pasting some 
> code from the other diff. Ideally I don't think it should be set in 
> checkPreStmt but this was an experiment...
>   void ArrayBoundCheckerV2::checkPreStmt(const ArraySubscriptExpr *A, 
> CheckerContext &C) const
>   {
>     ASTContext &Ctx = C.getASTContext();
>     QualType T = A->getBase()->IgnoreCasts()->getType();
>     const VariableArrayType *VLA = Ctx.getAsVariableArrayType(T);
>     if (!VLA)
>       return;
>     ProgramStateRef State = C.getState();
>     SVal ElementCount = State->getSVal(VLA->getSizeExpr(), 
> C.getLocationContext());
>     QualType ElementType = VLA->getElementType();
>     ASTContext &AstContext = C.getASTContext();
>     CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
>     if (Optional<DefinedOrUnknownSVal> DefinedSize =
>       ElementCount.getAs<DefinedOrUnknownSVal>()) {
>       SVal AV = State->getSVal(A->getBase(), C.getLocationContext());
>       const SubRegion *Region = AV.getAsRegion()->getAs<SubRegion>();
>       SValBuilder &svalBuilder = C.getSValBuilder();
>       DefinedOrUnknownSVal Extent = Region->getExtent(svalBuilder);
>       // size in Bytes = ElementCount*TypeSize
>       SVal SizeInBytes = svalBuilder.evalBinOpNN(
>         State, BO_Mul, ElementCount.castAs<NonLoc>(),
>         svalBuilder.makeArrayIndex(TypeSize.getQuantity()),
>         svalBuilder.getArrayIndexType());
>       DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
>         State, Extent, SizeInBytes.castAs<DefinedOrUnknownSVal>());
>       State = State->assume(extentMatchesSize, true);
>       C.addTransition(State);
>     }
>   }

You probably get an Unknown value for  `State->getSVal(VLA->getSizeExpr(), 
C.getLocationContext());`,  because the sizeExpr is not in the environment, it 
is already evaluated, its value has been used up, it is no longer needed. So in 
case the correct constraints are missing from the VLAs, you should add these 
assumptions at a point where this value is guaranteed to be available, probably 
right after the evaluation of the declaration.



cfe-commits mailing list
  • [PATCH] D30489: [analyze... Gábor Horváth via Phabricator via cfe-commits

Reply via email to