[PATCH] D152246: [clang][ThreadSafety] Analyze known function pointer values

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D152246#4412333 <https://reviews.llvm.org/D152246#4412333>, @aaronpuchert wrote:

> That's a tough one. The change seems to be correct, and makes more patterns visible. Of course I wonder how it comes that we see calls of a function pointer with a uniquely determined value, as that would seem like obfuscation. Maybe you can show an example how this might look like in practice?
>
> Another issue (that of course you can't know about) is that I'm not sure about the `LocalVariableMap`. It's currently only used to find a try-acquire function return value from a branch, not in the core analysis and can apparently be quite expensive <https://github.com/llvm/llvm-project/issues/42656> for long functions with exception handling edges. I'd like to get rid of it, unless we decide to use it for resolving aliases more widely. But I'm wary of alias analysis, because we don't want to reimplement the static analyzer, and we also want to be more predictable, maybe even provide correctness guarantees. Alias analysis ends in undecidable territory quite early on, and using heuristics would sacrifice predictability.

That's good to know, thank you for bringing this up! I agree that we likely don't want to do alias analysis. The way `CallExpr` implements `getCalleeDecl()` is to check whether the callee expression (after ignoring implicit casts and dereference operators) references a declaration of some kind (`DeclRefExpr`, `MemberExpr`, `BlockExpr`) and if so, report back the declaration found. So it does not do any sort of tricky dataflow analysis and only handles relatively simple cases like: `void func() {} void (*fp)() = func; fp();`

> The relatively strict "dumb" analysis that we're doing today seems to work well enough for the purpose of tracking locks, though we can look through local references. (Their pointee doesn't change. Though it doesn't always work <https://github.com/llvm/llvm-project/issues/62497>.) My hope would be that we can leave it at that and don't build a fully-fledged alias analysis.
>
> I'd like if we could address the issue here by moving to type attributes. These would of course be "sugar", i.e. dropped on canonicalization, as we don't want them to affect overloading etc. But that would still allow more than declaration attributes, like in this case, warning if we drop the attribute on assignment. Of course that's big project though, and it's not clear if it'll work out. The paper <https://static.googleusercontent.com/media/research.google.com/en/us/pubs/archive/42958.pdf> by @delesley and @aaron.ballman briefly says that non-sugared attributes would probably not work, and there is a talk <https://www.youtube.com/watch?v=5Xx0zktqSs4> where @delesley says something along the lines of "we'd really like to integrate this into the type system, but it would be quite difficult".

Yeah, I seem to recall this adding enough overhead to the type system for it to be problematic. I think we were lamenting that Clang doesn't have a pluggable type system and we'd love to see one added, but that's a big, experimental undertaking.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152246/new/

https://reviews.llvm.org/D152246