NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:133-135
+ static HandleState getWithoutError(HandleState S) {
+ return HandleState(S.K, nullptr);
+ }
----------------
It already makes me mildly uncomfortable that our data is not "normalized",
i.e. you can indicate the Schrödinger state by either adding an error symbol or
changing from `Allocated` to `MaybeAllocated`. Do i understand it correctly
that you're now adding a special state where the handle is still
`MaybeAllocated` but there's no error symbol? Please comment this up.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:415
+ if (CurItem.second.getErrorSym())
+ SymReaper.markLive(CurItem.first);
+ }
----------------
I think it should be possible to implement it without `checkLiveSymbols`,
because i don't have any reasons to believe that you can find the handle symbol
by looking at the error symbol, or vice versa, so i think they aren't supposed
to keep each other alive.
I.e., i think you can implement this by intentionally leaving zombie handles in
your state until the respective error symbol dies. After all, you don't rely on
any other metadata attached to your handles (eg., range constraints), you only
need your map, right?
I'm open to discussion here. I understand that intentional zombies sound scary,
but they also look slightly more correct to me. //"The handle is definitely
dead by now, but the successfulness of its birth is still a 'known unknown', so
let's delay our warning and keep the corpse frozen until we find out if it was
ever born to begin with"// - sounds about right :/ It's probably not a big deal
in any case, but i'm curious WDYT.
================
Comment at: clang/test/Analysis/fuchsia_handle.cpp:69
+ zx_status_t status = zx_channel_create(0, &sa, &sb);
+ if (status < 0)
+ return;
----------------
I'd like to see how do notes look when the warning *is* emitted (i.e., same
test but replace `status < 0` with `status >= 0`).
We don't have a note for the collapse point of the Schrödinger state, right? (i
think it was an unfortunate inevitable use case for a bug visitor because you
can't have tags in `evalAssume`)
That is, how comfortable would it be for the user to see that the leak warning
is emitted significantly later than the handle is dead? We already aren't great
at positioning our leak warnings, but it makes it even more unobvious. I guess
it's probably fine, but i want to see it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73151/new/
https://reviews.llvm.org/D73151
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
