[cfe-commits] r66892 - in /cfe/trunk: include/clang/Analysis/PathSensitive/BasicValueFactory.h lib/Analysis/RegionStore.cpp test/Analysis/ptr-arith.c

[Douglas Gregor]

On Mar 13, 2009, at 8:35 AM, Ted Kremenek wrote:

> Author: kremenek
> Date: Fri Mar 13 10:35:24 2009
> New Revision: 66892
>
> URL: http://llvm.org/viewvc/llvm-project?rev=66892&view=rev
> Log:
> Fix failure reported by Sebastian of test/Analysis/ptr-arith.c when  
> the target
> is 64-bit. I used his suggestion of doing a direct bitwidth/signedness
> conversion of the 'offset' instead of just changing the sign. For more
> information, see:

This kind of APSInt bitwidth/signedness problem has bitten us several  
times in several places, which makes me think that there are more of  
such problems lurking. Perhaps we should put a layer of abstraction  
over APSInt that automatically widens or sign-corrects when performing  
the various arithmetic and comparison operations?

	- Doug

> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2009-March/004587.html
>
> Modified:
>    cfe/trunk/include/clang/Analysis/PathSensitive/BasicValueFactory.h
>    cfe/trunk/lib/Analysis/RegionStore.cpp
>    cfe/trunk/test/Analysis/ptr-arith.c
>
> Modified: cfe/trunk/include/clang/Analysis/PathSensitive/ 
> BasicValueFactory.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/BasicValueFactory.h?rev=66892&r1=66891&r2=66892&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/include/clang/Analysis/PathSensitive/ 
> BasicValueFactory.h (original)
> +++ cfe/trunk/include/clang/Analysis/PathSensitive/ 
> BasicValueFactory.h Fri Mar 13 10:35:24 2009
> @@ -76,16 +76,18 @@
>   const llvm::APSInt& getValue(uint64_t X, unsigned BitWidth, bool  
> isUnsigned);
>   const llvm::APSInt& getValue(uint64_t X, QualType T);
>
> -  const llvm::APSInt& ConvertSignedness(const llvm::APSInt& To,
> -                                        const llvm::APSInt& From) {
> -    assert(To.getBitWidth() == From.getBitWidth());
> -
> -    // Same sign?  Just return.
> -    if (To.isUnsigned() == From.isUnsigned())
> +  /// Convert - Create a new persistent APSInt with the same value  
> as 'From'
> +  ///  but with the bitwidth and signeness of 'To'.
> +  const llvm::APSInt& Convert(const llvm::APSInt& To,
> +                              const llvm::APSInt& From) {
> +
> +    if (To.isUnsigned() == From.isUnsigned() &&
> +        To.getBitWidth() == From.getBitWidth())
>       return From;
>
> -    // Convert!
> -    return getValue(llvm::APSInt((llvm::APInt&) From, To.isUnsigned 
> ()));
> +    return getValue(From.getSExtValue(),
> +                    To.getBitWidth(),
> +                    To.isUnsigned());
>   }
>
>   const llvm::APSInt& getIntValue(uint64_t X, bool isUnsigned) {
>
> Modified: cfe/trunk/lib/Analysis/RegionStore.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/RegionStore.cpp?rev=66892&r1=66891&r2=66892&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/lib/Analysis/RegionStore.cpp (original)
> +++ cfe/trunk/lib/Analysis/RegionStore.cpp Fri Mar 13 10:35:24 2009
> @@ -642,12 +642,13 @@
>
>   // Only support concrete integer indexes for now.
>   if (Base && Offset) {
> -    // For now, convert the signedness of offset in case it doesn't  
> match.
> -    const llvm::APSInt &I =
> -      getBasicVals().ConvertSignedness(Base->getValue(), Offset- 
> >getValue());
> -    nonloc::ConcreteInt OffsetConverted(I);
> -
> -    SVal NewIdx = Base->EvalBinOp(getBasicVals(), Op,  
> OffsetConverted);
> +    // FIXME: For now, convert the signedness and bitwidth of  
> offset in case
> +    //  they don't match.  This can result from pointer  
> arithmetic.  In reality,
> +    //  we should figure out what are the proper semantics and  
> implement them.
> +    //
> +    nonloc::ConcreteInt OffConverted(getBasicVals().Convert(Base- 
> >getValue(),
> +                                                           Offset- 
> >getValue()));
> +    SVal NewIdx = Base->EvalBinOp(getBasicVals(), Op, OffConverted);
>     const MemRegion* NewER = MRMgr.getElementRegion(NewIdx,
>                                                     ER- 
> >getArrayRegion());
>     return Loc::MakeVal(NewER);
>
> Modified: cfe/trunk/test/Analysis/ptr-arith.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/ptr-arith.c?rev=66892&r1=66891&r2=66892&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/test/Analysis/ptr-arith.c (original)
> +++ cfe/trunk/test/Analysis/ptr-arith.c Fri Mar 13 10:35:24 2009
> @@ -1,4 +1,6 @@
> -// RUN: clang -analyze -checker-simple -analyzer-store=region - 
> verify %s
> +// RUN: clang -analyze -checker-simple -analyzer-store=region - 
> verify %s &&
> +// RUN: clang -analyze -checker-cfref -analyzer-store=region - 
> verify -triple x86_64-apple-darwin9 %s &&
> +// RUN: clang -analyze -checker-cfref -analyzer-store=region - 
> verify -triple i686-apple-darwin9 %s
>
> void f1() {
>   int a[10];
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits