[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

[Ted Kremenek]

Please file a PR.  I have no time to look at this right now, and we shouldn't forget about fixing this.

On Jan 19, 2012, at 5:04 PM, Eli Friedman wrote:

> 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