Skip to content

Conversation

@aaron-ang
Copy link
Contributor

@aaron-ang aaron-ang commented Jun 10, 2025

Fixes #11370

Summary

We import safe_numerics.h from PyTorch and extend mul_overflows for size_t. Then, we apply it in calculate_nbytes to perform faster overflow checks.

Test plan

Use existing tests since we are doing a drop-in replacement for overflow checks.

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 10, 2025

🔗 Helpful Links

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

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

❌ 17 New Failures

As of commit 73aacee with merge base 18e9149 (image):

NEW FAILURES - The following jobs have failed:

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 10, 2025
@aaron-ang
Copy link
Contributor Author

@pytorchbot label "release notes: runtime"

@pytorch-bot pytorch-bot bot added the release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs label Jun 10, 2025
@aaron-ang aaron-ang marked this pull request as ready for review June 10, 2025 20:48
namespace executor {

#ifdef __clang__
#pragma clang diagnostic ignore "-Wvla-extension"
Copy link
Contributor

Choose a reason for hiding this comment

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

whys this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I faced this error when compiling (./install_executorch.sh) with Clang on MacOS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aaron-ang can you paste the error?

Copy link
Contributor Author

@aaron-ang aaron-ang Jun 11, 2025

Choose a reason for hiding this comment

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

@jathu My bad. I faced this error not when installing Executorch, but when running tests locally using sh test/build_size_test.sh as mentioned in https://github.com/pytorch/executorch/blob/main/CONTRIBUTING.md#testing.

executorch/kernels/portable/cpu/util/normalization_ops_util.cpp:110:37: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
  110 |   executorch::aten::SizesType shape[ndim];
      |                                     ^~~~
executorch/kernels/portable/cpu/util/normalization_ops_util.cpp:110:37: note: read of non-const variable 'ndim' is not allowed in a constant expression
executorch/kernels/portable/cpu/util/normalization_ops_util.cpp:88:10: note: declared here
   88 |   size_t ndim = normalized_shape.size();
      |          ^
1 error generated.
gmake[2]: *** [kernels/portable/cpu/util/CMakeFiles/kernels_util_all_deps.dir/build.make:219: kernels/portable/cpu/util/CMakeFiles/kernels_util_all_deps.dir/normalization_ops_util.cpp.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
gmake[1]: *** [CMakeFiles/Makefile2:750: kernels/portable/cpu/util/CMakeFiles/kernels_util_all_deps.dir/all] Error 2
gmake: *** [Makefile:136: all] Error 2

My syntax is also wrong: it should be ignored instead of ignore, but surprisingly compiles fine.

@JacobSzwejbka
Copy link
Contributor

JacobSzwejbka commented Jun 11, 2025

Oh on

check-c10-sync / check-c10-sync (pull_request)

We track that anything under c10 mirrors exactly (this is a temporary thing iiuc until we can introduce a hard dep on pytorch/pytorch directly). If you want you could try submitting this to pytorch/pytorch instead and then mirror the file from there, or you can just move this to any other util spot that looks reasonable in ET to dodge this issue, and then we will deal with upstreaming it later.

Moving it will also probably help with the c10 namespace issues you were having.

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 3dda80e.

@aaron-ang
Copy link
Contributor Author

aaron-ang commented Jul 1, 2025

hi @huydhn why is this issue closed? I need to update the code from PyTorch upstream. It is different from this branch.
Edit: I have incorporated the changes. Could you please reopen the PR?

@huydhn
Copy link
Contributor

huydhn commented Jul 1, 2025

What! This makes no sense. I have no intention of closing this PR, as I didn't even know about this before you pointed it out. Something doesn't feel right

@huydhn
Copy link
Contributor

huydhn commented Jul 1, 2025

And I couldn't reopen the PR, the option is grayed out :(

Screenshot 2025-06-30 at 18 31 54

@huydhn
Copy link
Contributor

huydhn commented Jul 1, 2025

While I'm checking, maybe it's easier to recreate this in a different PR

@malfet
Copy link
Contributor

malfet commented Jul 1, 2025

What! This makes no sense. I have no intention of closing this PR, as I didn't even know about this before you pointed it out. Something doesn't feel right

huydhn/pytorch@3dda80e claimes to fix this issue, therefore github closed it (issues and pulls are the same entities in GitHub parlance)

swolchok pushed a commit that referenced this pull request Jul 18, 2025
### Summary
Reopening from #11537
Fixes #11370

### Test plan
Use existing tests since we are doing a drop-in replacement for overflow
checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import and extend c10/util/safe_numerics.h

6 participants