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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
