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.

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

Add overflow check for stride calculation #94900

Closed
wants to merge 3 commits into from
Closed

Add overflow check for stride calculation #94900

wants to merge 3 commits into from

Conversation

CaoE
Copy link
Collaborator

@CaoE CaoE commented Feb 15, 2023 •
edited

Fixes #94120 and #94128.

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 15, 2023 •
edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/94900

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit dca992e:
💚 Looks good so far! There are no failures yet. 💚

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

@CaoE CaoE mentioned this pull request Feb 16, 2023
Closed
@CaoE CaoE mentioned this pull request Apr 3, 2023
Closed
@ezyang ezyang self-requested a review April 3, 2023 19:16
ezyang
ezyang reviewed Apr 3, 2023
View reviewed changes
c10/core/TensorImpl.h Outdated
overflowed |= c10::mul_overflows(sizes_and_strides_.stride_at_unchecked(i + 1), std::max<int64_t>(
sizes_and_strides_.size_at_unchecked(i + 1), 1), std::addressof(sizes_and_strides_.stride_at_unchecked(i)));
TORCH_CHECK(!overflowed,
"Stride calculation overflowed with stride of dim", i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to test this only once outside of the loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Moved it outside the loop.

@CaoE CaoE force-pushed the ecao/fix_overlow branch 2 times, most recently from f52b910 to 93b6092 Compare April 6, 2023 09:12
@ezyang
Copy link
Contributor

ezyang commented Apr 6, 2023

waiting for you to un-draft this

@CaoE CaoE requested a review from jgong5 April 7, 2023 01:16
jgong5
jgong5 reviewed Apr 7, 2023
View reviewed changes
c10/util/safe_numerics.h
#if C10_HAS_BUILTIN_OVERFLOW()
return __builtin_mul_overflow(a, b, out);
#else
volatile int64_t tmp = a * b;
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I know why you make it volatile?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because compiler might optimize/omit it.

@CaoE CaoE marked this pull request as ready for review April 7, 2023 03:57
@CaoE CaoE added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 7, 2023
@ezyang
Copy link
Contributor

ezyang commented Apr 7, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@ezyang ezyang added release notes: composability release notes category topic: bug fixes topic category labels Apr 9, 2023
@ezyang
Copy link
Contributor

ezyang commented Apr 9, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

skotapati pushed a commit to kulinseth/pytorch that referenced this pull request Apr 10, 2023
@CaoE @skotapati
Add overflow check for stride calculation (pytorch#94900)
ef3a2a3 
Fixes pytorch#94120 and pytorch#94128.

Pull Request resolved: pytorch#94900
Approved by: https://github.com/ezyang, https://github.com/jgong5
ZainRizvi pushed a commit that referenced this pull request Apr 19, 2023
@CaoE @ZainRizvi
Add overflow check for stride calculation (#94900)
6a36f2a 
Fixes #94120 and #94128.

Pull Request resolved: #94900
Approved by: https://github.com/ezyang, https://github.com/jgong5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezyang ezyang ezyang approved these changes

@jgong5 jgong5 jgong5 approved these changes

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: composability release notes category topic: bug fixes topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A Segmentation Fault can be trigerred in torch.clone
5 participants
@CaoE @ezyang @pytorchmergebot @jgong5 @pytorchbot