Skip to content

Remove UB type punning from c10/util/floating_point_utils.h #140567

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.

Sign up for GitHub

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

Jump to bottom
Closed
swolchok wants to merge 3 commits into from
Closed

Remove UB type punning from c10/util/floating_point_utils.h #140567

swolchok wants to merge 3 commits into from

Conversation

@swolchok
Copy link
Contributor

@swolchok swolchok commented Nov 13, 2024 •
edited
Loading

@swolchok
Remove UB type punning from c10/util/floating_point_utils.h
f961510 
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]
@pytorch-bot
Copy link

pytorch-bot bot commented Nov 13, 2024 •
edited
Loading

🔗 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 SEVs

There are 2 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 656fca5 with merge base 891ba2e (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

This was referenced Nov 13, 2024
Closed
Closed
Closed
swolchok added a commit that referenced this pull request Nov 13, 2024
@swolchok
Remove UB type punning from c10/util/floating_point_utils.h
acf14bb 
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
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 13, 2024

This pull request was exported from Phabricator. Differential Revision: D65888680

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 14, 2024
@swolchok swolchok added the topic: improvements topic category label Nov 14, 2024
@swolchok
Update on "Remove UB type punning from c10/util/floating_point_utils.h"
b25f7e6 
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]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 14, 2024

This pull request was exported from Phabricator. Differential Revision: D65888680

@swolchok swolchok mentioned this pull request Nov 14, 2024
Closed
@swolchok swolchok added the release notes: cpp release notes category label Nov 14, 2024
@swolchok
Update on "Remove UB type punning from c10/util/floating_point_utils.h"
656fca5 
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]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 14, 2024

This pull request was exported from Phabricator. Differential Revision: D65888680

malfet
malfet approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@swolchok
Copy link
Contributor Author

swolchok commented Nov 18, 2024 •
edited
Loading

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

@swolchok
Copy link
Contributor Author

swolchok commented Nov 18, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Nov 18, 2024

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

pytorchmergebot pushed a commit that referenced this pull request Nov 18, 2024
@swolchok @pytorchmergebot
Port X86_F16 from executorch half to PyTorch half (#140720)
17bb78a 
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
@swolchok swolchok mentioned this pull request Nov 19, 2024
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 19, 2024

This pull request was exported from Phabricator. Differential Revision: D65888680

1 similar comment
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 19, 2024

This pull request was exported from Phabricator. Differential Revision: D65888680

@swolchok swolchok mentioned this pull request Nov 19, 2024
Closed
pytorchmergebot pushed a commit that referenced this pull request Nov 19, 2024
@swolchok @pytorchmergebot
Fix Half X86_F16 CUDA build failure (#140994)
efed02b 
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
pytorchmergebot pushed a commit that referenced this pull request Nov 19, 2024
@swolchok @pytorchmergebot
Use std::bit_cast as c10::bit_cast if available (#141035)
f30f43f 
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
cyyever pushed a commit to cyyever/pytorch that referenced this pull request Nov 20, 2024
@swolchok @cyyever
Remove UB type punning from c10/util/floating_point_utils.h (pytorch#…
2956aa3 
…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
cyyever pushed a commit to cyyever/pytorch that referenced this pull request Nov 20, 2024
@swolchok @cyyever
Port X86_F16 from executorch half to PyTorch half (pytorch#140720)
294048d 
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
cyyever pushed a commit to cyyever/pytorch that referenced this pull request Nov 20, 2024
@swolchok @cyyever
Fix Half X86_F16 CUDA build failure (pytorch#140994)
d542332 
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
cyyever pushed a commit to cyyever/pytorch that referenced this pull request Nov 20, 2024
@swolchok @cyyever
Use std::bit_cast as c10::bit_cast if available (pytorch#141035)
929d553 
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
Ryo-not-rio pushed a commit to Ryo-not-rio/pytorch that referenced this pull request Dec 2, 2024
@swolchok @Ryo-not-rio
Port X86_F16 from executorch half to PyTorch half (pytorch#140720)
444e1bf 
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
Ryo-not-rio pushed a commit to Ryo-not-rio/pytorch that referenced this pull request Dec 2, 2024
@swolchok @Ryo-not-rio
Fix Half X86_F16 CUDA build failure (pytorch#140994)
191589d 
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
Ryo-not-rio pushed a commit to Ryo-not-rio/pytorch that referenced this pull request Dec 2, 2024
@swolchok @Ryo-not-rio
Use std::bit_cast as c10::bit_cast if available (pytorch#141035)
8d533f4 
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
pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
@swolchok
Remove UB type punning from c10/util/floating_point_utils.h (pytorch#…
26cdc9d 
…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
pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
@swolchok
Port X86_F16 from executorch half to PyTorch half (pytorch#140720)
b614c64 
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
pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
@swolchok
Fix Half X86_F16 CUDA build failure (pytorch#140994)
b8eb551 
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
pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
@swolchok
Use std::bit_cast as c10::bit_cast if available (pytorch#141035)
b0a2889 
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
fmo-mt pushed a commit to fmo-mt/pytorch that referenced this pull request Dec 11, 2024
@swolchok @fmo-mt
Remove UB type punning from c10/util/floating_point_utils.h (pytorch#…
6bcb03a 
…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
fmo-mt pushed a commit to fmo-mt/pytorch that referenced this pull request Dec 11, 2024
@swolchok @fmo-mt
Port X86_F16 from executorch half to PyTorch half (pytorch#140720)
d008f51 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Skylion007 Skylion007 Skylion007 approved these changes

@malfet malfet malfet approved these changes

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged release notes: cpp release notes category topic: improvements topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@swolchok @facebook-github-bot @pytorchmergebot @Skylion007 @malfet