NoQ added a comment.
Ok screw it, my static example is still broken. There's technically still a
leak in it because there's no time for the other code to kick in between `p =
(int *)malloc(sizeof(int));` and `p = 0;` to read the value from `p`.
But in practice there could be a lot of things going on in between, that with
your patch might not trigger enough escapes. Let me disconnect from the
original test case and try to build a few examples more carefully.
void foo() {
static int *p;
escape(&p);
p = malloc(sizeof(int));
free_whatever_escaped();
p = 0; // no-leak
}
This isn't a leak, regardless of whether the variable is static or not.
Currently we display a leak when the variable is non-static (bad) but don't
display a leak when the variable is static (good). IIUC your patch might break
this. The important part is that in this case there's no direct connection
between the tracked pointer `p` and the escaped region `&p` because `p` isn't
stored in `&p` until the next line. So if you remove invalidation of `p` on
`free_whatever_escaped();`, it'll cause a new false positive in the static case.
void foo(cond) {
static int *p;
if (cond) {
p = malloc(sizeof(int));
free_whatever_escaped();
p = 0; // no-leak
}
escape(&p);
}
In this case there's no leak as long as the first invocation of the function
always has `cond` set to false. In this case, again, there's no direct
connection between `&p` and the return value of `malloc()` when direct escape
of `&p` happens. On top of that, the direct escape of `&p` isn't observed at
all until much later in the analysis. But just because the local is static, and
the function can be called multiple times, the escape at the end of the
analysis may "affect" our decision-making at the beginning of the analysis. I
suspect your patch breaks this example as well.
So basically in order to do the right thing here, you need to make sure there
are no direct escapes of that static variable *anywhere* in the function. But
as long as there's no direct escapes, you're probably good to go. With
non-static locals you only need to observe escapes *before* the leak (but
possibly before the allocation as well), which we currently don't do a good job
at anyway.
Then, again, it's possible that your patch improves FP rates than what we
currently have, just by being a different trade-off, given how artificial my
examples are, but we'll need some data to figure it out.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139534/new/
https://reviews.llvm.org/D139534
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
