================
@@ -1854,28 +1945,27 @@ static ProgramStateRef
MallocUpdateRefState(CheckerContext &C, const Expr *E,
// Get the return value.
if (!RetVal)
- RetVal = C.getSVal(E);
+ RetVal = State->getSVal(E, C.getLocationContext());
// We expect the malloc functions to return a pointer.
if (!RetVal->getAs<Loc>())
return nullptr;
SymbolRef Sym = RetVal->getAsLocSymbol();
- // This is a return value of a function that was not inlined, such as
malloc()
- // or new(). We've checked that in the caller. Therefore, it must be a
symbol.
- assert(Sym);
- // FIXME: In theory this assertion should fail for `alloca()` calls (because
- // `AllocaRegion`s are not symbolic); but in practice this does not happen.
+ // FIXME: Following if fails for `alloca()` calls (because
+ // `AllocaRegion`s are not symbolic);
// As the current code appears to work correctly, I'm not touching this issue
// now, but it would be good to investigate and clarify this.
// Also note that perhaps the special `AllocaRegion` should be replaced by
// `SymbolicRegion` (or turned into a subclass of `SymbolicRegion`) to enable
// proper tracking of memory allocated by `alloca()` -- and after that change
// this assertion would become valid again.
----------------
NagyDonat wrote:
```suggestion
// NOTE: If this was an `alloca()` call, then `RetVal` holds an
// `AllocaRegion`, so `Sym` will be a nullpointer because `AllocaRegion`s do
// not have an associated symbol. However, this distinct region type means
// that we don't need to store anything about them in `RegionState`.
```
I'm happy to see that this FIXME comment (which was added by one of my commits
a few months ago) can be finally eliminated :relieved:. At that time I was
confused to see that `assert(Sym);` was did not crash after `alloca()` calls,
but your refactoring experimentally proves that this was caused by the
invariant-breaking logic that used `BindExpr()` from the `postCall` callback.
My suggestion rewords and mostly removes the leftover parts of my old comment
block, because I'm satisfied with their resolution and no "FIXME" remains after
your change.
I thought about keeping the remark that "perhaps the special `AllocaRegion`
should be replaced by `SymbolicRegion` (or turned into a subclass of
`SymbolicRegion`)", but I decided to remove it, because it would be completely
off-topic here. (It would have provided a workaround to dodge the "why is this
assert not triggered?" mystery, but now there is no motivation to mention it.)
I still have an unrelated motivation for replacing `AllocaRegion`s with regular
`SymbolicRegion`s (it would improve diagnostics of some
`alpha.core.ArrayBoundV2` reports, because I could place a note tag on the
`alloca()` call), but there is no reason to mention that (low priority) plan
here.
https://github.com/llvm/llvm-project/pull/106081
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
