Jordy, Thanks for the review.
On Apr 10, 2012, at 8:12 PM, Jordy Rose wrote:
> Nice. There are two PRs about dynamic_cast: PR10380 and PR12366, though
> neither of them are so encompassing. Does this handle those cases?
>
This was done as part of addressing PR12366. I closed the other one as "Works
for me", not sure what was the complaint there.
Note, the cast still does not work when we 'new' an object. Not sure why, but I
am out of time working on this.
See: r154543.
>
> On Apr 10, 2012, at 16:59, Anna Zaks wrote:
>
>> + if (Failed) {
>> + // If the cast fails, conjure symbol constrained to 0.
>> + DefinedOrUnknownSVal NewSym =
>> svalBuilder.getConjuredSymbolVal(NULL,
>> + CastE, LCtx, resultType,
>> +
>> currentBuilderContext->getCurrentBlockCount());
>> + DefinedOrUnknownSVal Constraint = svalBuilder.evalEQ(state,
>> + NewSym,
>> svalBuilder.makeZeroVal(resultType));
>> + state = state->assume(Constraint, true);
>> + state = state->BindExpr(CastE, LCtx, NewSym);
>
> Why is this constrained to 0 instead of just a null? (SValBuilder::makeNull)
> We don't track anything about null pointers, so we don't need the symbol.
>
Using a symbolic value is more flexible, potentially, we could add extra
constraints on top of that. However, I could not come up with an example to
prove that, so changed in r154542.
>
>> + } else {
>> + // If we don't know if the cast succeeded, conjure a new symbol.
>> + if (val.isUnknown()) {
>> + DefinedOrUnknownSVal NewSym =
>> svalBuilder.getConjuredSymbolVal(NULL,
>> + CastE, LCtx, resultType,
>> +
>> currentBuilderContext->getCurrentBlockCount());
>> + state = state->BindExpr(CastE, LCtx, NewSym);
>
> Since dynamic_casts should /always/ be checked, I feel like we should
> probably just bifurcate the state here, so that the subexpr value gets
> propagated. Unfortunately, SValBuilder can't do that...
>
>
Bifurcating the state should not be done unless absolutely necessary; it has
huge impact on performance.
>> +// False negatives.
>> +
>> +// Symbolic regions are not typed, so we cannot deduce that the cast will
>> +// always fail in this case.
>> +int testDynCastFail1(class C *c) {
>> + B *b = 0;
>> + b = dynamic_cast<B*>(c);
>> + return b->m;
>> +}
>
> Yuck. A syntactic checker would be able to handle this. Why isn't (*c) a
> TypedRegion on top of the SymbolicRegion?
>
The comment in the Symbolic Region says that it could be typed. I am guessing
we just did not get there yet. However, as per Richard's comment, we'll have to
special case here to make sure we do not report failure when the type info
comes from a symbolic region. There might be more issues like this throughout
the codebase.
> Also, the class keyword is odd here.
In r154541
> Jordy
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
