[cfe-commits] r138372 - in /cfe/trunk: lib/Analysis/CFG.cpp lib/StaticAnalyzer/Core/ExprEngineC.cpp test/Analysis/outofbound-notwork.c test/Analysis/outofbound.c test/Sema/uninit-variables.c

[Eli Friedman]

On Tue, Aug 23, 2011 at 1:30 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Tue Aug 23 15:30:50 2011
> New Revision: 138372
>
> URL: http://llvm.org/viewvc/llvm-project?rev=138372&view=rev
> Log:
> Fix regression in -Wuninitialized involving VLAs.  It turns out that we were modeling sizeof(VLAs)
> incorrectly in the CFG, and also the static analyzer.  This patch regresses the analyzer a bit, but
> that needs to be followed up with a better solution.
>
> Fixes <rdar://problem/10008112>.
>
> Added:
>    cfe/trunk/test/Analysis/outofbound-notwork.c
> Modified:
>    cfe/trunk/lib/Analysis/CFG.cpp
>    cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
>    cfe/trunk/test/Analysis/outofbound.c
>    cfe/trunk/test/Sema/uninit-variables.c
>
> Modified: cfe/trunk/lib/Analysis/CFG.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=138372&r1=138371&r2=138372&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
> +++ cfe/trunk/lib/Analysis/CFG.cpp Tue Aug 23 15:30:50 2011
> @@ -2203,16 +2203,7 @@
>     for (const VariableArrayType *VA =FindVA(E->getArgumentType().getTypePtr());
>          VA != 0; VA = FindVA(VA->getElementType().getTypePtr()))
>       lastBlock = addStmt(VA->getSizeExpr());
> -  } else {
> -    // For sizeof(x), where 'x' is a VLA, we should include the computation
> -    // of the lvalue of 'x'.
> -    Expr *subEx = E->getArgumentExpr();
> -    if (subEx->getType()->isVariableArrayType()) {
> -      assert(subEx->isLValue());
> -      lastBlock = addStmt(subEx);
> -    }
>   }
> -
>   return lastBlock;
>  }

A bit late, but I think this commit is wrong.  The subexpression of a
sizeof() expression is in fact evaluated per C99 6.5.3.4p2.  So
strictly speaking, this code has undefined behavior.  The fact that
we're getting this wrong is leading to a crash in a patch I'm working
on to model the evaluated-ness of sizeof() correctly in Sema.

(That said, we can use the following reasoning to suppress the warning
for the given testcase: in "sizeof(*memory)", the code doesn't
actually use the loaded value, so it doesn't matter that it's an
uninitialized load.)

-Eli