NoQ added a comment.
Much symbols!
================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+ State = State->set<RawPtrMap>(ObjRegion, NewSet);
----------------
rnkovacs wrote:
> xazax.hun wrote:
> > Is it possible here the set to be empty? We just added a new element to it
> > above. Maybe turn this into an assert or just omit this if it is impossible?
> I'm not sure whether `add()` can fail. I turned it into an assert now and
> will see if it ever fails.
It totally can't. We're just adding an object to a set. What could possibly go
wrong?
================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:32-40
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait<PtrSet>
+ : public ProgramStatePartialTrait<PtrSet> {
+ static void *GDMIndex() {
+ static int Index = 0;
+ return &Index;
----------------
Please add a comment on how this template is useful. This trick is used by some
checkers, but it's a veeeery unobvious trick. We should probably add a macro
for that, i.e. `REGISTER_FACTORY_WITH_PROGRAMSTATE` or something like that.
================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:113
- const SymbolRef *StrBufferPtr = State->get<RawPtrMap>(TypedR);
- // FIXME: What if Origin is null?
const Expr *Origin = Call.getOriginExpr();
----------------
Maybe turn the FIXME into a NOTE or something like that. It indeed seems fine,
but let's still make sure the reader realizes that there's a subtle potential
problem here.
================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:115
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
+ // Start tracking this raw pointer by adding it to the set of symbols
----------------
I'd much rather unwrap the symbol here, i.e. `if (SymbolRef Sym =
RawPtr.getAsSymbol())`. A lot of other cornercases may occur if the
implementation is accidentally inlined (`Undefined`, concrete regions). Also,
speculatively, `.getAsSymbol(/* search for symbolic base = */ true)` for the
same reason//*//.
If we didn't care about inlined implementations, the `Unknown` check would have
been redundant. So it should also be safe to straightforwardly ignore inlined
implementations by consulting `C.wasInlined`, then the presence of the symbol
can be asserted. But i'd rather speculatively care about inlined
implementations as long as it seems easy.
__
//*// In fact your code relies on a very wonky implementation detail of our
`SVal` hierarchy: namely, pointer-type return values of conservatively
evaluated functions are always expressed as `&SymRegion{conj_$N<pointer type>}`
and never as `&element{SymRegion{conj_$N<pointer type>}, 0 S32b, pointee
type}`. Currently nobody knows the rules under which zero element regions are
added in different parts of the analyzer, i.e. what is the "canonical"
representation of the symbolic pointer, though i made a few attempts to
speculate.
================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:119
+ PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
+ PtrSet NewSet = F.getEmptySet();
+ if (const PtrSet *OldSet = State->get<RawPtrMap>(ObjRegion)) {
----------------
My favorite idiom for such cases, looks a bit cleaner:
```
const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion)
PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet()
```
================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:122
+ NewSet = *OldSet;
+ if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
----------------
This `contains` check is very unlikely to yield true because if the
implementation is not inlined, the symbol is newly born. I'd much rather skip
the check; it doesn't sound like it'd make things any faster.
Even better, let's assert that: `assert(C.wasInlined || !Set.contains(Sym))`.
It'll probably help us catch some "reincarnated symbol" bugs in the core.
https://reviews.llvm.org/D49057
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
