steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, martong, baloghadamsoftware, ASDenysPetrov, xazax.hun, Szelethus. Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity. Herald added a project: clang. Harbormaster failed remote builds in B65978: Diff 281152! Harbormaster returned this revision to the author for changes because remote builds failed. steakhal requested review of this revision. steakhal added a comment.
Manually force **request review**, to start a discussion about this. Although I managed to implement this in a way to pass the tests, but the logic became somewhat messy, that's why I stick to this //dummy// patch. I'm gonna clean that up and update the diff, if you think that such handing of memregions are valid. Sorry for the `cfe-dev` spam, I did not know that I can manually //request review//. Currently, if the analyzer evaluates an expression like `to - from`, it only computes reasonable answer if both are representing ElementRegions. Formally, **for all** memory region `X` and **for all** SVal offset `Y`: `&Element{SymRegion{X},Y,char} - &SymRegion{X}` => `Y` The same for the symmetric version, but returning `-Y` instead. I'm curious why don't we handle the case, when only one of the operands is an `ElementRegion` and the other is a plain `SymbolicRegion`. IMO if the SuperMemoryRegion is the same, we can still return a reasonable answer. In this patch, I suppose an extension to resolve this. If this seems to be a good idea, I will: - implement the symmetric version of the check - add tests Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D84736 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -1017,6 +1017,21 @@ } } + // Try to get the Index as Nonloc, then cast to ArrayIndexTy. + // Otherwise return None. + const auto MaybeNonLocIndexOfElementRegion = + [this](const ElementRegion *ElemReg) -> Optional<NonLoc> { + SVal IndexVal = ElemReg->getIndex(); + Optional<NonLoc> Index = IndexVal.getAs<NonLoc>(); + if (!Index) + return llvm::None; + IndexVal = evalCastFromNonLoc(*Index, ArrayIndexTy); + Index = IndexVal.getAs<NonLoc>(); + if (!Index) + return llvm::None; + return Index.getValue(); + }; + // Handle special cases for when both regions are element regions. const ElementRegion *RightER = dyn_cast<ElementRegion>(RightMR); const ElementRegion *LeftER = dyn_cast<ElementRegion>(LeftMR); @@ -1027,31 +1042,32 @@ // give the correct answer. if (LeftER->getSuperRegion() == RightER->getSuperRegion() && LeftER->getElementType() == RightER->getElementType()) { - // Get the left index and cast it to the correct type. - // If the index is unknown or undefined, bail out here. - SVal LeftIndexVal = LeftER->getIndex(); - Optional<NonLoc> LeftIndex = LeftIndexVal.getAs<NonLoc>(); - if (!LeftIndex) - return UnknownVal(); - LeftIndexVal = evalCastFromNonLoc(*LeftIndex, ArrayIndexTy); - LeftIndex = LeftIndexVal.getAs<NonLoc>(); - if (!LeftIndex) - return UnknownVal(); - // Do the same for the right index. - SVal RightIndexVal = RightER->getIndex(); - Optional<NonLoc> RightIndex = RightIndexVal.getAs<NonLoc>(); - if (!RightIndex) - return UnknownVal(); - RightIndexVal = evalCastFromNonLoc(*RightIndex, ArrayIndexTy); - RightIndex = RightIndexVal.getAs<NonLoc>(); - if (!RightIndex) + Optional<NonLoc> LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER); + Optional<NonLoc> RightIndex = MaybeNonLocIndexOfElementRegion(RightER); + + if (!LeftIndex || !RightIndex) return UnknownVal(); // Actually perform the operation. // evalBinOpNN expects the two indexes to already be the right type. return evalBinOpNN(state, op, *LeftIndex, *RightIndex, resultTy); } + } else if (LeftER && isa<SymbolicRegion>(RightMR)) { + if (LeftER->getSuperRegion() != RightMR) + return UnknownVal(); + if (Optional<NonLoc> LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER)) + return LeftIndex.getValue(); + return UnknownVal(); + } else if (RightER && isa<SymbolicRegion>(LeftMR)) { + if (RightER->getSuperRegion() != LeftMR) + return UnknownVal(); + if (Optional<NonLoc> RightIndex = + MaybeNonLocIndexOfElementRegion(RightER)) + llvm_unreachable( + "TODO: return the negated value of RightIndex - figure out how :D\n" + "Maybe a unary minus operator would do the job."); + return UnknownVal(); } // Special handling of the FieldRegions, even with symbolic offsets.
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -1017,6 +1017,21 @@ } } + // Try to get the Index as Nonloc, then cast to ArrayIndexTy. + // Otherwise return None. + const auto MaybeNonLocIndexOfElementRegion = + [this](const ElementRegion *ElemReg) -> Optional<NonLoc> { + SVal IndexVal = ElemReg->getIndex(); + Optional<NonLoc> Index = IndexVal.getAs<NonLoc>(); + if (!Index) + return llvm::None; + IndexVal = evalCastFromNonLoc(*Index, ArrayIndexTy); + Index = IndexVal.getAs<NonLoc>(); + if (!Index) + return llvm::None; + return Index.getValue(); + }; + // Handle special cases for when both regions are element regions. const ElementRegion *RightER = dyn_cast<ElementRegion>(RightMR); const ElementRegion *LeftER = dyn_cast<ElementRegion>(LeftMR); @@ -1027,31 +1042,32 @@ // give the correct answer. if (LeftER->getSuperRegion() == RightER->getSuperRegion() && LeftER->getElementType() == RightER->getElementType()) { - // Get the left index and cast it to the correct type. - // If the index is unknown or undefined, bail out here. - SVal LeftIndexVal = LeftER->getIndex(); - Optional<NonLoc> LeftIndex = LeftIndexVal.getAs<NonLoc>(); - if (!LeftIndex) - return UnknownVal(); - LeftIndexVal = evalCastFromNonLoc(*LeftIndex, ArrayIndexTy); - LeftIndex = LeftIndexVal.getAs<NonLoc>(); - if (!LeftIndex) - return UnknownVal(); - // Do the same for the right index. - SVal RightIndexVal = RightER->getIndex(); - Optional<NonLoc> RightIndex = RightIndexVal.getAs<NonLoc>(); - if (!RightIndex) - return UnknownVal(); - RightIndexVal = evalCastFromNonLoc(*RightIndex, ArrayIndexTy); - RightIndex = RightIndexVal.getAs<NonLoc>(); - if (!RightIndex) + Optional<NonLoc> LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER); + Optional<NonLoc> RightIndex = MaybeNonLocIndexOfElementRegion(RightER); + + if (!LeftIndex || !RightIndex) return UnknownVal(); // Actually perform the operation. // evalBinOpNN expects the two indexes to already be the right type. return evalBinOpNN(state, op, *LeftIndex, *RightIndex, resultTy); } + } else if (LeftER && isa<SymbolicRegion>(RightMR)) { + if (LeftER->getSuperRegion() != RightMR) + return UnknownVal(); + if (Optional<NonLoc> LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER)) + return LeftIndex.getValue(); + return UnknownVal(); + } else if (RightER && isa<SymbolicRegion>(LeftMR)) { + if (RightER->getSuperRegion() != LeftMR) + return UnknownVal(); + if (Optional<NonLoc> RightIndex = + MaybeNonLocIndexOfElementRegion(RightER)) + llvm_unreachable( + "TODO: return the negated value of RightIndex - figure out how :D\n" + "Maybe a unary minus operator would do the job."); + return UnknownVal(); } // Special handling of the FieldRegions, even with symbolic offsets.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits