[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D81678#2109059 <https://reviews.llvm.org/D81678#2109059>, @nlopes wrote:

> I'm a bit concerned with this patch as it increases the amount of UB that LLVM exploits without any study of the impact.
>  For example, right now it's ok do this with clang (not with constants; make it less trivial so clang doesn't fold it right away):
>
>   int f() { return INT_MAX + 1; }
>
> While technically this is UB in C, when lowered to LLVM IR, this function returns poison.
>  When the frozen attribute is attached, the function will now trigger UB in LLVM IR as well.
>  Is this what we want?


I think this is at least a mode we want to support, yes. I would try to make this default if we have proper tooling in place.

> It would be worthwhile to at least compile a few programs to check if they break. Also, what's the plan to detect these cases in ubsan? We shouldn't be exploiting new UB without having good warnings/checks in place IMO.

+1, good points, especially the ubsan one.

> Note that pure function calls with 'frozen' arguments become harder to hoist from loops, for example. Since now calling this pure function can trigger UB if one of the arguments is poison. You would need to introduce frozen beforehand. I don't see this issue addressed in this patch.

TBH, I don't think this is the case. Pure is not sufficient to hoist if it was not executed at all, only `speculatable` is. For a `speculatable` call you should be able to remove `frozen` from the attributes if you can't prove it. Actually, it might not be a good idea to have `speculatable` and `frozen` on the same call, given that they kinda contradict each other. Back to pure, aka `readnone` (I would assume is what you meant): If it is pure you cannot hoist it as it could cause UB  (`return 1/arg;` or `return 1 / 0;` for all we know.). Generally, I guess the return value attribute `frozen` can be dropped though.

> For msan & friends, this patch makes sense, as they operate in a rely-guarantee way. Initialization of memory is checked, so it can be relied upon by the compiler later on.

At least the `nopoison` part is something we need in the IR so we can make violated value attributes cause poison and not UB, e.g., if you pass `NULL` to a `nonnull` argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678