aaronpuchert wrote:

> The problem is that, if lockFooWithEscapablePointer() doesn't take the 
> pointer-to-alias as const, we conservatively invalidate the alias, because 
> after that function call we don't know if F is still pointing to whatever it 
> pointed to before the call. That's technically WAI.

Hmm, this reminds me of what @delesley wrote on https://reviews.llvm.org/D52395:

>  When you pass an object to a function, either by pointer or by reference, no 
> actual load from memory has yet occurred. Thus, there is a real risk of false 
> positives; what you are saying is that the function *might* read or write 
> from protected memory, not that it actually will.

Also later:

> Adding to the difficulty is the fact that the correct use of "const" in 
> real-world C++ code is spotty at best. There is LOTS of code that arguably 
> should be using const, but doesn't for various reasons.

Which probably applies even more to C, given that it still doesn't allow 
conversions like `T**` → `const T* const*` that are allowed in C++.

The other aspect is that we're not the static analyzer, and we're not trying to 
track values and their changes over the course of a function. There are too 
many moving pieces. I think we should use alias analysis only as a fallback, 
expanding variables if there is no equality.

What I mean by this is the following: expressions should not be eagerly 
expanded but stay as written, with the context attached. In the commit message 
example, we're locking `*C` in the context { `C` = `0` } (a bit strange, as you 
pointed out, but let's just go with it), then accessing `x` with `*C` locked in 
the context { `C` = ? }, then unlocking `*C` in the same context.

Both the access and the unlocking should proceed with the usual AST walk, 
expanding variables with the context only if they're not equal. In this case, 
we have `*C` = `*C` for both comparisons, so we stop right there. Only when 
comparison fails should we attempt to expand one or both sides until we have a 
match.

So I agree with @ziqingluo-90 that this behavior is undesirable and should be 
fixed. We can also discuss the invalidation rules. But the example shouldn't 
depend on invalidation rules and always be accepted.

You could argue that we should at least track aliases that we're certain about, 
but that's limited to cases like taking addresses of different global or local 
variables. Anything that takes a pointer/reference from anywhere could alias 
anything else, and as I wrote above, we're going to get into deep trouble if 
we're trying to implement something in the direction of the static analyzer. 
This will end up in a messy patch work of heuristics, and the goal for the 
warning is to be predictable.

https://github.com/llvm/llvm-project/pull/183640
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to