Skip to content

c10/util/BFloat16-math.h has undefined behavior #144495

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

Open
swolchok opened this issue Jan 9, 2025 · 1 comment
Open

c10/util/BFloat16-math.h has undefined behavior #144495

swolchok opened this issue Jan 9, 2025 · 1 comment
Labels
module: build Build system issues module: core aten Related to change to the Core ATen opset triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@swolchok
Copy link
Contributor

swolchok commented Jan 9, 2025

🐛 Describe the bug

Per https://en.cppreference.com/w/cpp/language/extending_std:

It is undefined behavior to add declarations or definitions to namespace std or to any namespace nested within std, with a few exceptions noted below.

The "exceptions noted below" do not seem to include what we're doing in BFloat16-math.h, and specifically don't include adding overloads of functions that take program-defined types.

This problem is currently "theoretical" in that I am not aware of practical issues resulting from this header at this time.

To fix this, we would need to at least put the functions in BFloat16-math.h into a namespace other than std (either c10 or a new one, like say c10_math). Then, we could either:

  • have callers do using std::pow and all the other cmath functions, and rely on ADL to select the c10/c10_math version for half/BFloat16
  • using all the std:: functions into our namespace (which IMO argues toward that namespace being a new one like c10_math).

Versions

N/A

cc @malfet @seemethere @manuelcandales @SherlockNoMad @angelayi

swolchok added a commit that referenced this issue Jan 9, 2025
Partial fix for #144495. Avoiding BC-break using existing practice of removing only if FBCODE_CAFFE2 and C10_NODEPRECATED are not defined.

Differential Revision: [D67992342](https://our.internmc.facebook.com/intern/diff/D67992342/)

[ghstack-poisoned]
swolchok added a commit that referenced this issue Jan 9, 2025
Partial fix for #144495. Avoiding BC-break using existing practice of removing only if FBCODE_CAFFE2 and C10_NODEPRECATED are not defined.

Differential Revision: [D67992342](https://our.internmc.facebook.com/intern/diff/D67992342/)

ghstack-source-id: 260810447
Pull Request resolved: #144502
@malfet malfet added module: build Build system issues triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module module: core aten Related to change to the Core ATen opset labels Jan 9, 2025
@malfet
Copy link
Contributor

malfet commented Jan 9, 2025

like say c10_math

I would prefer c10::math, where math is inline namespace

pytorchmergebot pushed a commit that referenced this issue Jan 10, 2025
Partial fix for #144495. Avoiding BC-break using existing practice of removing only if FBCODE_CAFFE2 and C10_NODEPRECATED are not defined.

Differential Revision: [D67992342](https://our.internmc.facebook.com/intern/diff/D67992342/)

Pull Request resolved: #144502
Approved by: https://github.com/malfet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: build Build system issues module: core aten Related to change to the Core ATen opset triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

2 participants