Remove UB type punning from c10/util/floating_point_utils.h#140567
Remove UB type punning from c10/util/floating_point_utils.h#140567swolchok wants to merge 3 commits intogh/swolchok/694/basefrom
Conversation
Accessing the inactive member of a union is undefined behavior. Fortunately, we have c10::bit_cast. Differential Revision: [D65888680](https://our.internmc.facebook.com/intern/diff/D65888680/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/140567
Note: Links to docs will display an error until the docs builds have been completed. ❗ 2 Active SEVsThere are 2 currently active SEVs. If your PR is affected, please view them below:
✅ No FailuresAs of commit 656fca5 with merge base 891ba2e ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Accessing the inactive member of a union is undefined behavior. Fortunately, we have c10::bit_cast. Differential Revision: [D65888680](https://our.internmc.facebook.com/intern/diff/D65888680/) ghstack-source-id: 253354216 Pull Request resolved: #140567
|
This pull request was exported from Phabricator. Differential Revision: D65888680 |
Accessing the inactive member of a union is undefined behavior. Fortunately, we have c10::bit_cast. Differential Revision: [D65888680](https://our.internmc.facebook.com/intern/diff/D65888680/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D65888680 |
Accessing the inactive member of a union is undefined behavior. Fortunately, we have c10::bit_cast. Differential Revision: [D65888680](https://our.internmc.facebook.com/intern/diff/D65888680/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D65888680 |
malfet
left a comment
There was a problem hiding this comment.
Sure, but for my own education, why this is an undefined behavior? I though that union is a legit way of casting between types of the same size and https://stackoverflow.com/questions/6725809/trap-representation seems to confirm that
It might be legal C (and that stack overflow question is about C), but it is not legal C++. This is one of the ways in which C++ is not a superset of C; instead they are two sets with overlapping and non-overlapping parts. Compare https://en.cppreference.com/w/cpp/language/union (search "undefined") to https://en.cppreference.com/w/c/language/union (search "unspecified" and "type punning"). FWIW I wrote an article about fun things to do with unions at https://dev-discuss.pytorch.org/t/unionizing-for-profit-how-to-exploit-the-power-of-unions-in-c/444 |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This was added in pytorch/executorch#1789 . I'm working on sharing Half.h with ExecuTorch, and this is a missing feature. Differential Revision: [D65949409](https://our.internmc.facebook.com/intern/diff/D65949409/) Pull Request resolved: #140720 Approved by: https://github.com/malfet ghstack dependencies: #140564, #140565, #140566, #140567
|
This pull request was exported from Phabricator. Differential Revision: D65888680 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D65888680 |
It passed PyTorch CI, but internally we saw failures from this. Differential Revision: [D66137897](https://our.internmc.facebook.com/intern/diff/D66137897/) Pull Request resolved: #140994 Approved by: https://github.com/malfet ghstack dependencies: #140564, #140565, #140566, #140567, #140720
Make what we're doing as obvious as possible to the compiler. Differential Revision: [D66108811](https://our.internmc.facebook.com/intern/diff/D66108811/) Pull Request resolved: #141035 Approved by: https://github.com/Skylion007, https://github.com/ezyang, https://github.com/malfet ghstack dependencies: #140564, #140565, #140566, #140567, #140720, #140994
…140567) Accessing the inactive member of a union is undefined behavior. Fortunately, we have c10::bit_cast. Differential Revision: [D65888680](https://our.internmc.facebook.com/intern/diff/D65888680/) Pull Request resolved: pytorch#140567 Approved by: https://github.com/Skylion007, https://github.com/malfet ghstack dependencies: pytorch#140564, pytorch#140565, pytorch#140566
This was added in pytorch/executorch#1789 . I'm working on sharing Half.h with ExecuTorch, and this is a missing feature. Differential Revision: [D65949409](https://our.internmc.facebook.com/intern/diff/D65949409/) Pull Request resolved: pytorch#140720 Approved by: https://github.com/malfet ghstack dependencies: pytorch#140564, pytorch#140565, pytorch#140566, pytorch#140567
It passed PyTorch CI, but internally we saw failures from this. Differential Revision: [D66137897](https://our.internmc.facebook.com/intern/diff/D66137897/) Pull Request resolved: pytorch#140994 Approved by: https://github.com/malfet ghstack dependencies: pytorch#140564, pytorch#140565, pytorch#140566, pytorch#140567, pytorch#140720
Make what we're doing as obvious as possible to the compiler. Differential Revision: [D66108811](https://our.internmc.facebook.com/intern/diff/D66108811/) Pull Request resolved: pytorch#141035 Approved by: https://github.com/Skylion007, https://github.com/ezyang, https://github.com/malfet ghstack dependencies: pytorch#140564, pytorch#140565, pytorch#140566, pytorch#140567, pytorch#140720, pytorch#140994
Stack from ghstack (oldest at bottom):
Accessing the inactive member of a union is undefined behavior. Fortunately, we have c10::bit_cast.
Differential Revision: D65888680