Skip to content
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

Fix nvcc-imposed UB in constexpr functions #17534

Merged

Conversation

vuule
Copy link
Contributor

@vuule vuule commented Dec 5, 2024

Description

nvcc does not support constexpr functions that are not well-defined to call from the device. This is UB even when the function is not called from the device.

Throwing an exception is one such operation. This PR cleans up error handling for functions that are called from device, and removes constexpr from the ones that are not actually used from the device, or in the constexpr context.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vuule vuule added bug Something isn't working non-breaking Non-breaking change labels Dec 5, 2024
@vuule vuule self-assigned this Dec 5, 2024
Copy link

copy-pr-bot bot commented Dec 5, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Dec 5, 2024
@vuule
Copy link
Contributor Author

vuule commented Dec 5, 2024

/ok to test

@vuule
Copy link
Contributor Author

vuule commented Dec 6, 2024

/ok to test

@vuule vuule marked this pull request as ready for review December 6, 2024 18:15
@vuule vuule requested a review from a team as a code owner December 6, 2024 18:15
@vuule vuule requested review from mythrocks and davidwendt December 6, 2024 18:15
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

Wow, that is some unexpected weirdness.

@davidwendt
Copy link
Contributor

Perhaps this is related to #7795

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I also suspect #7795 and mentioned that on Slack :)

@vuule
Copy link
Contributor Author

vuule commented Dec 6, 2024

There's definitely some overlap with #7795, but, AFAIK, that issue is related to device functions that include a throw. RLE_stream_size is only called on the host side. IMO it should behave according to the C++ standard.
Unless I misunderstand how code is generated and #7795 impacts both host and device code.

@vuule vuule changed the title Workaround for an error in ORC writer on some CUDA versions Fix nvcc-imposed UB in constexpr functions Dec 6, 2024
@vuule
Copy link
Contributor Author

vuule commented Dec 9, 2024

/merge

@rapids-bot rapids-bot bot merged commit ba3ed57 into rapidsai:branch-25.02 Dec 9, 2024
104 of 105 checks passed
@vuule vuule deleted the remove-constexpr-with-cudf_fail branch December 9, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants