-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[LangRef] Specify that load of alloca outside lifetime is poison #157852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
We consider (in bounds) loads from allocas to always be speculatable, without taking lifetimes into account. This means that such loads cannot be immediate UB. Specify them as returning poison instead. Due to stack coloring, such a load may end up loading from a different alloca, but that's compatible with poison. Stores are still UB, but that's a much more narrow problem (I think the only transform violating that part is store scalar promotion in LICM).
@llvm/pr-subscribers-llvm-ir Author: Nikita Popov (nikic) ChangesWe consider (in bounds) loads from allocas to always be speculatable, without taking lifetimes into account. This means that such loads cannot be immediate UB. Specify them as returning poison instead. Due to stack coloring, such a load may end up loading from a different alloca, but that's compatible with poison. Stores are still UB, but that's a much more narrow problem (I think the only transform violating that part is store scalar promotion in LICM). Fixes #141892 (and probably a bunch of others...) Full diff: https://github.com/llvm/llvm-project/pull/157852.diff 1 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index a06be9dfb9403..3ba94d0d483bf 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -3388,6 +3388,10 @@ the object's lifetime. A stack object's lifetime can be explicitly specified
using :ref:`llvm.lifetime.start <int_lifestart>` and
:ref:`llvm.lifetime.end <int_lifeend>` intrinsic function calls.
+As an exception to the above, loading from a stack object outside its lifetime
+is not undefined behavior and returns a poison value instead. Storing to it is
+still undefined behavior.
+
.. _pointeraliasing:
Pointer Aliasing Rules
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG. I will update llubi later.
@dtcxzyw No, this store does unfortunately get executed, resulting in observable miscompiles when stack coloring is enabled. We still need a solution for that case. (Likely by actually checking whether we are within the alloca lifetime or not.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, in practice, optimizations already worked this way. Shrink-wrapping doesn't track alloca lifetimes, and without shrinkwrapping an alloca is always backed by memory anyway. And we don't have the infrastructure for IR optimizations to prove a load is outside the lifetime of an alloca.
I guess it doesn't hurt to explicitly state it, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
We consider (in bounds) loads from allocas to always be speculatable, without taking lifetimes into account. This means that such loads cannot be immediate UB. Specify them as returning poison instead.
Due to stack coloring, such a load may end up loading from a different alloca, but that's compatible with poison.
Stores are still UB, but that's a much more narrow problem (I think the only transform violating that part is store scalar promotion in LICM).
Fixes #141892 (and probably a bunch of others...)