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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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? It would be worthwhile to at least compile a few programs to check if they break.


Even if we say that it's undefined behavior, we don't have to start converting "ret undef" to "unreachable".  I mean, it's something we could consider doing, but we don't have to immediately start doing it just because the attribute exists.  I expect that just hooking up the attribute to isGuaranteedNotToBeUndefOrPoison() will have almost no immediate effect.

> Also, what's the plan to detect these cases in ubsan?

I don't think this has any practical impact on our goals with sanitizers.  We should detect undefined behavior before it gets to the point of actually passing or returning an undef or poison value.

> 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.

A pure function can have undefined behavior, in general.

I guess the interaction between "speculatable" and "frozen" is a little weird.  We don't have any optimizations that infer either "speculatable" or "frozen", though, so I'm not sure there's any practical impact here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678