[shoumikhin]

Title: Add validate_tensor_layout to check a tensor layout without aborting, and document the raw-bytes overloads

What is wrong today

A bad layout aborts. make_tensor_ptr aborts when the strides do not match the sizes:

auto t = make_tensor_ptr({2, 2}, data.data(), {}, {8, 2});

No ExecuTorch function lets an app reject a bad layout and continue.

Some strides overflow. Portable strides are 32-bit, so for sizes like {0, 65536, 65536} computing them is undefined behavior (signed overflow).

The raw-bytes overloads are easy to misread. With sizes, a std::vector<uint8_t> is raw memory read as the given type, like a void* in from_blob. The docs never said the type defaults to Float.

Tensor.fromArray with extra data aborts the WebAssembly module.

What this change does

Commit 1 is docs only: raw bytes, the Float default, ScalarType::Byte.

Commit 2 adds validate_tensor_layout(sizes, dim_order, strides). It returns Error::InvalidArgument for every layout that the make_tensor_ptr overloads taking sizes and data abort on. It also rejects negative sizes, sizes whose product overflows, and dense strides that do not fit StridesType. It does not check the data.

if (validate_tensor_layout(sizes, {}, strides) != Error::Ok) {
  return Error::InvalidArgument;
}

make_tensor_ptr uses the same internal check. It accepts and rejects the same layouts as before, with the same strides; only its abort message changes. It still accepts overflowing strides, which the program loader builds too, but computes them with c10::mul_overflows instead of a plain signed multiply.

The WebAssembly Tensor.fromArray and Tensor.fromIter now use this check, so overflowing strides throw too, and every layout error says "Invalid tensor layout." rather than a specific message.

Commit 3 makes both throw when they get more data than the sizes need.

Case	PyTorch	Before	After
Strides that do not match	accepted	abort	abort, or InvalidArgument
Strides that overflow	throws on 64-bit overflow	wrapped stride	same stride, or InvalidArgument

What was tested

• extension_tensor_test passes in portable and ATen mode, and fails if any check breaks.
• On 150,000 random layouts, portable and ATen, make_tensor_ptr matches main, and a signed-overflow sanitizer on tensor_ptr.cpp flags none (main: thousands).
• The WebAssembly unit tests pass; the new ones fail without their fix.
• A Linux CMake build with LLM, XNNPACK and devtools passes all its tests; lintrunner is clean.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 1 New Failure

As of commit d6e6f75 with merge base e0015e1 ():

NEW FAILURE - The following job has failed:

• Build Presets / apple (llm) / build (gh)
  ##[error]fatal: Fetched in submodule path 'extension/llm/tokenizers/third-party/json', but it did not contain e6cafa573aac6ed9227f752a5371c0b3f436307d. Direct fetching of that commit failed.

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 19m 38s —— View job

---

Request changes: removing the Float default breaks existing out-of-tree callers at run time without the deprecation step that the API policy requires, and the wasm bindings' hand-copied layout check now lets JS input reach the new abort; the other findings are minor contract, docs and test gaps.

Detailed review

PR Review: #24772

Summary

The PR makes a std::vector<uint8_t> without a scalar type a Byte tensor. It also adds validate_tensor_layout, a layout check that returns an error instead of aborting and shares its logic with make_tensor_ptr.

Two issues block:

• The default is removed without going through Deprecated first.
• One in-tree caller, the wasm bindings, keeps its own copy of the check and was not switched to the new function.

The other findings are minor. All four changed files were covered. The findings come from reading the code; nothing was built or run.

Code Quality

• The overflow guard covers construction only, and it repeats the stride computation.
  • tensor_ptr.cpp:102-116 repeats the stride product that dim_order_to_stride_nocheck computes.
  • The resize path still uses the unchecked runtime::dim_order_to_stride (dim_order_util.h:152-170): resize_tensor_ptr calls TensorImpl::internal_resize_contiguous (tensor_impl.cpp:150-168).
  • Example in portable mode: make_tensor_ptr({0, 1, 1}, std::vector<float>{}), then resize_tensor_ptr(t, {0, 65536, 65536}). The resize passes the numel bound (0 ≤ 0) and wraps strides[0] in int32. That is the state this PR now refuses to create.
  • A checked multiply inside dim_order_to_stride, which already returns Error, would cover both paths. make_tensor_ptr could keep calling it as it did before the PR, and the wasm copy below would get the check too.
  • This touches runtime/core, so it may need discussion. It is not a blocker.

Testing

• tensor_ptr_test.cpp:1097-1118 only tests validate_tensor_layout. Missing:

  • An ET_EXPECT_DEATH showing that make_tensor_ptr itself now aborts on an overflowing layout, e.g. {0, 1 << 30, 1 << 30, 1 << 30}. This is the make_tensor_ptr behavior change the description claims, and nothing tests that the two functions agree.
  • A case where the element count overflows, which is the safe_numel path.
  • A stride overflow with no zero-size dim, e.g. {2, 65536, 65536}. Its numel (2^33) fits in ssize_t, but its largest stride (2^32) does not fit in int32. It should give InvalidArgument in portable mode and Ok under USE_ATEN_LIB. The only overflow case now has a zero-size dim.

  Fix this →

API Design

• validate_tensor_layout throws away the element count that the caller needs next.
  • tensor_ptr.cpp:77-78 runs safe_numel and keeps only the error.
  • The vector overloads still abort unless the data size matches exactly (tensor_ptr.h:233-235, tensor_ptr.cpp:218-222).
  • The raw-bytes overload also aborts when numel * elementSize(type) overflows (tensor_ptr.cpp:209-217). This function cannot check that, because it does not take the type.
  • So the description's pattern, validate and then make_tensor_ptr(sizes, data, {}, strides), still aborts on a payload of the wrong length unless the caller repeats safe_numel and the byte-size overflow check.
  • Consider returning the count (Result<size_t>), or taking the ScalarType and returning the byte size.
  • The size wording also describes a lower bound, while the vector overloads need an exact match. This affects "holds as many elements" in the new header text (tensor_ptr.h:638-639) and "must be large enough" on the edited docs line (extension-tensor.md:123). Fix wording →
• The documented contract leaves out the 16-dim limit.
  • tensor_ptr.h:632-636 lists the conditions. runtime::validate_dim_order also rejects dim > kTensorDimensionLimit (dim_order_util.h:30-32), even when dim_order was empty and built internally.
  • validate_tensor_layout(std::vector<SizesType>(17, 1), {}, {}) returns InvalidArgument and logs "values must be a permutation of [0, 17)" (tensor_ptr.cpp:97-101). The message points the caller at a dim order they never passed.
  • make_tensor_ptr already had this limit through dim_order_to_stride, so the gap is in the contract, not in behavior.
  • Document the limit, and ideally check it explicitly with its own message. Fix this →
• docs/source/extension-tensor.md does not mention the new API. That page documents the dim_order and strides parameters of make_tensor_ptr, and CONTRIBUTING.md:113-114 asks for doc updates on API changes. A short example that validates and then creates the tensor would cover it. Fix this →

Backward Compatibility

• Removing the Float default (tensor_ptr.h:436, :455) breaks a Stable API at run time, with no deprecation step.
  • make_tensor_ptr is not marked experimental. docs/source/api-life-cycle.md:22-31 says such APIs "will always transition to Deprecated before being broken".
  • Existing 2-arg and 4-arg calls still compile without a warning. They now resolve to the std::vector<T> template (tensor_ptr.h:216-235) with T = uint8_t, and abort at :233-235 after an SDK upgrade. Example: a 602112-byte buffer for {1, 3, 224, 224} that used to become a Float tensor.
  • The description argues that such callers fail loudly. But they fail at run time instead of compile time, and that is the outcome the deprecation step exists to prevent. The risk is to out-of-tree code.
  • Suggested path: for one release, add ET_DEPRECATED (runtime/platform/compiler.h:120) non-template overloads (sizes, std::vector<uint8_t>) and (sizes, std::vector<uint8_t>, dim_order, strides) that forward ScalarType::Float. As exact non-template matches they win over the template, so old callers get a compile warning. Remove them afterwards to switch to Byte. Fix this →
• The wasm bindings keep a hand-copied version of the old check, so JS input can now abort the module.
  • According to its comment, assert_dim_order_and_strides_valid (extension/wasm/wasm_bindings.cpp:139-182) exists so that make_tensor_ptr failures become JS errors instead of aborts.
  • It calls the unchecked runtime::dim_order_to_stride (:163-165), so it accepts layouts that the new check at tensor_ptr.cpp:104-116 rejects.
  • Example: JsTensor::from_array (:300-325) with sizes [0, 65536, 65536] and empty data. It passes the wasm check, assert_valid_numel (numel 0) and the template's size check, then dies in ET_CHECK_MSG at tensor_ptr.cpp:151. Before this PR it returned a tensor.
  • This is the use case validate_tensor_layout was added for. Replace the helper's body with THROW_IF_ERROR(validate_tensor_layout(...)). That also removes the third copy of the logic and makes "always agree" hold for this caller. Fix this →

Recommendation

Request Changes

Two things block:

• Under the API life-cycle policy, the Float default has to be deprecated before it is removed.
• The wasm bindings should use validate_tensor_layout, so that this PR does not add an abort path for JS input.

The remaining items are minor.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 19m 55s —— View job

---

Request changes: validate_tensor_layout returns Ok for layouts whose strides overflow StridesType, such as {0, 65536, 65536} in portable mode, and untrusted input like this is what it was added to reject; the other findings are minor.

Detailed review

PR Review: #24772

Summary

The PR documents that the std::vector<uint8_t> overloads read raw bytes as type (default Float). It also adds validate_tensor_layout, which shares one layout check with make_tensor_ptr and replaces the wasm bindings' copy.

One issue should be fixed before merge: the new check accepts layouts whose strides overflow, and it overflows a signed integer while computing them. The other three findings are minor: the docs, the wasm error messages and a boundary test. All five changed files were covered. The findings come from reading the code; nothing was built or run.

Code Quality

• validate_tensor_layout accepts layouts whose strides overflow StridesType.

  • The only overflow check is safe_numel (tensor_ptr.cpp:330-331). It checks the product of all sizes in ssize_t.
  • The strides are then computed by dim_order_to_stride_nocheck (tensor_ptr.cpp:108-109, dim_order_util.h:146-147) without a check. They are int32_t in portable mode (portable_type/tensor_impl.h:90) and int64_t in ATen mode.
  • A size-0 dimension makes the element count 0. It only stops the stride product when it is the inner factor (dim_order_util.h:143).
  • Examples:
    • Portable mode, any target including wasm32: validate_tensor_layout({0, 65536, 65536}, {}, {}) computes strides[0] = 65536 * 65536 in int32_t and returns Ok. make_tensor_ptr({0, 65536, 65536}, std::vector<float>{}) passes its data-size check and builds a tensor with the overflowed stride.
    • Portable mode, 64-bit host: {2, 65536, 65536}. Its count, 2^33, fits ssize_t, but strides[0], 2^32, does not fit int32_t.
    • ATen mode: {0, 1LL << 32, 1LL << 32}.
  • So the function meant for layouts from a file or the network runs signed-overflow UB on such input, then reports it as valid.
  • Fix: compute the strides with c10::mul_overflows, which this file already uses (:201), and return InvalidArgument on overflow, at least on the validate_tensor_layout path.
    • Lines 102-109 already copy the body of runtime::dim_order_to_stride (dim_order_util.h:162-168), message included, so the checked loop can replace the dim_order_to_stride_nocheck call there.
    • Doing it in resolve_layout also turns the silent wrap in make_tensor_ptr into an abort.
    • Add the examples above as tests.

  Fix this →

• JS callers of the wasm bindings no longer see why a layout was rejected.

  • THROW_JS_ERROR used to put the reason into the JS Error with snprintf (wasm_bindings.cpp:26-31), for example "invalid strides for dim 0: 8!= 2 while its size is 2 != 1". Every layout failure now throws "Invalid tensor layout." (:144-146).
  • The commit message says the reason goes to the log. But the wasm builds compile logging out:
    • The documented CMake builds are Release (extension/wasm/README.md:33, scripts/build_wasm_tests.sh:41), and EXECUTORCH_ENABLE_LOGGING is on by default only for Debug builds (tools/cmake/preset/default.cmake:19-22, CMakeLists.txt:613-615).
    • The Buck test build passes -DET_LOG_ENABLED=0 (extension/wasm/test/xplat_build.sh:47).
  • Negative or overflowing sizes are now caught here first too, because the layout check (:279, :331) runs before assert_valid_numel (:284, :337). So they also get the generic message instead of "Invalid tensor sizes: numel computation failed" (:108-109).
  • If JS users should still get a reason, put the sizes, dim order and strides in the thrown message. Otherwise, correct the commit message.

Testing

• No test accepts a 16-dim layout.
  • The new dim <= kTensorDimensionLimit check (tensor_ptr.cpp:78-83) now runs before every make_tensor_ptr.
  • The tests reject 17 dims (tensor_ptr_test.cpp:1106-1109). No test, for either function, accepts 16.
  • An off-by-one in that check would stop make_tensor_ptr from creating 16-dim tensors, and no test would fail. Fix this →

API Design

• The docs promise more than the check covers.

  • The example still aborts on a bad payload.
    • The example at extension-tensor.md:167-172 says "reject the input instead of aborting", then calls the vector overload.
    • That overload still aborts when the payload has the wrong length (tensor_ptr.h:233-235, tensor_ptr.cpp:208-212).
    • The note after the example (:174) says so, but the example leaves the check out. Copied as-is, it aborts on a truncated file.
  • Raw bytes can abort on a byte count that overflows.
    • The raw-bytes overload aborts when numel * elementSize(type) overflows (tensor_ptr.cpp:200-207), before it compares sizes. validate_tensor_layout does not take the type, so it cannot catch this.
    • On 32-bit targets such as wasm32, {1 << 30} with Float passes validation and then aborts there, whatever the payload.
    • A caller who computes the expected byte count in plain size_t gets 0, so an empty payload also passes their own check.
    • The docs do not say that this count needs an overflow check.
  • The aliasing overload is not covered.
    • tensor_ptr.h:630-632 promises an error for "every layout that make_tensor_ptr aborts on".
    • The aliasing overload (tensor_ptr.h:485-533) treats empty sizes as the source's sizes (:491-493). It also aborts when the view has more elements than the source (:505-509).
    • Example: with a 2×2 source, make_tensor_ptr(src, {3, 3}) aborts, but validate_tensor_layout({3, 3}, {}, {}) returns Ok.
  • Fix:
    • Show the data-size check in the example.
    • Say that the byte count needs an overflow check.
    • Limit the header sentence to the overloads that take sizes and data.

  Fix this →

Recommendation

Request Changes

The stride overflow should be fixed before merge. validate_tensor_layout exists to vet untrusted layouts, and right now it overflows a signed integer on some of them and reports them as valid. The other items are minor.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

Thanks for the careful review. Changes in this push:

• Stride overflow. The shared layout check now computes strides with an overflow check. validate_tensor_layout returns InvalidArgument for {0, 65536, 65536} and {2, 65536, 65536} in portable mode, and make_tensor_ptr aborts on them. These layouts ran signed overflow before. So did {1, 16384, 131072}, which now aborts too; the description names it. PyTorch also throws "Stride calculation overflowed" when its own strides overflow. New tests cover both functions in portable and ATen mode, and the size-0 stride rule.
• 16 dims. A new test checks that validate_tensor_layout accepts 16 dims and make_tensor_ptr builds a 16-dim tensor.
• Docs. The guide example now checks the data size. The guide says the raw-bytes byte count needs an overflow check. The header claim now covers only the overloads that take sizes and data.
• Wasm messages. The commit message now says that fromArray and fromIter throw "Invalid tensor layout." for every layout error and that the reason is logged only with logging enabled. I kept the generic text on purpose: the bindings report other ExecuTorch errors the same way, and the exact reason would need the copied checks back or a new API.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 32m 39s —— View job

---

PR Review: #24772

Request changes: Android Tensor.fromBlob and wasm Tensor.zeros/ones/full still pass unchecked shapes to make_tensor_ptr, which now aborts on stride overflow, and the new overflow check is a copy that resize_tensor_ptr and the runtime don't use.

Detailed review

Summary

The PR adds validate_tensor_layout, which shares a new resolve_layout with make_tensor_ptr (which now also rejects stride overflow and more than 16 dims), uses it in the wasm bindings, and documents that the std::vector<uint8_t> overloads take raw bytes.

• Main problems: two binding paths still hand unchecked shapes to the factory and now abort on them, and the overflow check lives in a copy of the stride code that other paths skip. The rest is minor.
• Coverage: all 5 changed files were reviewed. The findings come from reading the code; nothing was built or run.

Backward Compatibility

• Android Tensor.fromBlob and wasm Tensor.zeros/ones/full now abort on shapes they used to accept.

  • Cause: the new stride-overflow abort (tensor_ptr.cpp:114-119, :152) is reached from bindings that never call validate_tensor_layout.
  • Android: Tensor.fromBlob(FloatArray(0), longArrayOf(0, 65536, 65536)) passes the Kotlin checks (Tensor.kt:569-572, default dim order at :66) and the JNI checks (jni_layer.cpp:159-212; numel is 0). It then aborts the app process in for_blob(...).make_tensor_ptr() (jni_layer.cpp:213-218). Before this PR the call returned a tensor whose stride came from a signed overflow.
  • wasm: et.Tensor.zeros([0, 65536, 65536]) aborts the module through extension::zeros → empty_strided → make_tensor_ptr. full and ones do the same (wasm_bindings.cpp:199-235).
  • This is the same kind of abort the PR fixes for fromArray/fromIter.
  • Fix:
    • JNI: call validate_tensor_layout(shape_vec, dim_order_vec, {}) before for_blob and throw ExecutorchException on error.
    • wasm: call assert_dim_order_and_strides_valid(sizes_vec, {}, {}) in the three factories.

  Fix this →

API Design

• Inferring the dim order from strides rejects strides that make_tensor_ptr itself builds (tensor_ptr.h:637-638, tensor_ptr.cpp:97-99).

  • Cause: a dim of size 0 or 1 has the same stride as the next-outer dim. Neither the new doc nor std::sort says how such ties are ordered.
  • Example:
    • make_tensor_ptr({0, 2, 3}, ptr, {1, 0, 2}) builds strides {3, 3, 1}.
    • validate_tensor_layout({0, 2, 3}, {}, {3, 3, 1}) infers {0, 1, 2} when ties keep index order (as libstdc++ does here) and returns InvalidArgument.
    • make_tensor_ptr aborts on the same input.
  • The inference predates the PR, but the PR now documents it as the contract.
  • Fix: on equal strides, put the dim of size > 1 first, then order by index, and document the rule. In a run of equal dense strides only the outermost dim can be larger than 1, so this always finds a matching order.

  Fix this →

• The T* overload doc contradicts the new raw-bytes doc for uint8_t (tensor_ptr.h:109-113 vs :420-422).

  • The T* doc says the type is deduced "the same way the vector overload deduces it from the vector's element type". But std::vector<uint8_t> selects the raw-bytes overload, which defaults to Float.
  • Example: make_tensor_ptr({4}, bytes.data()) gives a 4-element Byte tensor. make_tensor_ptr({4}, std::move(bytes)) asks for Float and aborts (it has 4 bytes and needs 16).
  • Fix: say that uint8_t is the exception.

  Fix this →

Code Quality

• The overflow check is a copy of runtime::dim_order_to_stride, so other paths still compute the same strides unchecked (tensor_ptr.cpp:107-120). The original is a plain signed multiply at dim_order_util.h:146-147.

  • resize_tensor_ptr: it goes through internal_resize_contiguous → dim_order_to_stride (tensor_impl.cpp:163-168). In portable mode, take auto t = make_tensor_ptr({0, 1, 1}, std::vector<float>{}). Then resize_tensor_ptr(t, {0, 65536, 65536}) overflows int32_t (UB) and returns Ok, and a later clone of t aborts.
  • Program loading: the runtime builds strides the same way (tensor_parser_portable.cpp:144, method.cpp:795, :819). So clone_tensor_ptr or the aliasing make_tensor_ptr(tensor) can abort on a tensor the runtime accepted.
  • Fix:
    • Put the c10::mul_overflows check in dim_order_to_stride, which already returns Error.
    • Check every stride before writing any, so a failed resize leaves the tensor unchanged.
    • Call it from resolve_layout.
    • This changes runtime/core and may need a runtime owner. If that is out of scope here, check the new sizes with validate_tensor_layout in resize_tensor_ptr before resizing.

  Fix this →

• make_tensor_ptr skips the safe_numel check that validate_tensor_layout runs (tensor_ptr.cpp:341-342).

  • Cause: the factory goes from resolve_layout straight to the TensorImpl constructor, whose compute_numel multiplies without an overflow check (tensor_impl.cpp:42).
  • Example: on 32-bit targets (wasm32, armv7), make_tensor_ptr({2, 1 << 30}, ptr) passes resolve_layout and overflows ssize_t, while validate_tensor_layout rejects the same sizes.
  • Fix: move the check into resolve_layout. The header can then drop the separate case at tensor_ptr.h:632-633.

  Fix this →

• wasm fromArray/fromIter still abort when given too much data (wasm_bindings.cpp:109-110; the issue predates this PR).

  • assert_valid_numel accepts data.size() >= numel, but the vector make_tensor_ptr aborts unless the two are equal (tensor_ptr.h:233-235). So et.Tensor.fromArray([2], [1, 2, 3]) kills the module.
  • Fix: use ==.

  Fix this →

• With EXECUTORCH_LOG_LEVEL=Fatal, the abort no longer says what is wrong (tensor_ptr.cpp:152).

  • The specific messages moved from ET_CHECK_MSG (logged at Fatal) to ET_CHECK_OR_RETURN_ERROR (logged at Error). Those builds now print only "Invalid tensor layout.".
  • The commit message's "reason logged on the line before it" only holds at Error level or lower.

Testing

• Missing cases (tensor_ptr_test.cpp:1104, unittests.js:84-96):

  • No C++ test uses a duplicate dim order such as validate_tensor_layout({3, 4}, {1, 1}, {}); the existing invalid dim orders are all out of range.
  • No wasm test checks that et.Tensor.fromArray([0, 65536, 65536], []) throws, even though that is the new abort the wasm change guards against.

  Fix this →

Recommendation

Request Changes

• Blocking: the Android and wasm-factory aborts are new crashes on binding input, the same kind the PR fixes for fromArray/fromIter. Each one needs a single validate_tensor_layout call.
• Strongly recommended: moving the overflow check into runtime::dim_order_to_stride. It touches runtime/core, so it may need a runtime owner; the resize_tensor_ptr fallback is fine for this PR.
• Minor: everything else.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

Changes in this push:

• New aborts in Android fromBlob and wasm zeros/ones/full. Fixed at the source. Making make_tensor_ptr abort on stride overflow was itself a breaking change: the program loader builds such tensors too (a {1, 65536, 32768} output gets a wrapped stride on its size-1 dim), and cloning or wrapping them aborted. make_tensor_ptr now accepts and rejects the same layouts as on main, with the same strides, computed with c10::mul_overflows instead of a plain signed multiply. Only validate_tensor_layout rejects stride overflow. On 150,000 random layouts the results match main.
• Check in runtime::dim_order_to_stride. Not here: it would make program loading and resize_tensor_ptr reject layouts they accept today. That needs its own runtime change.
• Minors. Fixed: the uint8_t exception in the typed-pointer doc, a duplicate dim order test, a JS test for overflowing strides, the log level wording in the commit message, and fromArray with extra data. Not changed: the reason stays at Error level, like other checks that return an Error; safe_numel in make_tensor_ptr (catchable ATen errors would become aborts); the tie rule (it changes the dim order of layouts accepted today). The header now says to pass a dim order for equal strides.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 25m 55s —— View job

---

PR Review: #24772

Approve: nothing blocks; the remaining findings are minor, chiefly that validate_tensor_layout accepts or rejects the same runtime-built tensor depending on whether its dim order is passed, and that the wasm zeros/ones/full still skip the new check.

Detailed review

Summary

The PR adds validate_tensor_layout, which shares one layout check with make_tensor_ptr (whose accepted layouts and strides stay the same), uses it in the wasm fromArray/fromIter, and documents that the std::vector<uint8_t> overloads take raw bytes.

• Verdict: no blockers. The findings are minor:
  • one rule in the new function;
  • gaps in wasm coverage, docs and tests;
  • signed-overflow UB that remains on Windows.
• Coverage: all six changed files were reviewed. The findings come from reading the code; nothing was built or run.

Code Quality

• The wasm zeros, ones and full still abort on bad sizes, and now disagree with fromArray (extension/wasm/wasm_bindings.cpp:197-236).

  • They take sizes from JS but don't call assert_layout_valid.
  • These aborts predate this PR:
    • et.Tensor.zeros([-1]) aborts the module in empty_strided (tensor_ptr_maker.cpp:116-120).
    • 17 sizes abort in make_tensor_ptr (tensor_ptr.cpp:161).
  • This disagreement is new: et.Tensor.zeros([0, 65536, 65536]) builds a tensor with a wrapped stride, while et.Tensor.fromArray([0, 65536, 65536], []) now throws (unittests.js:100-102).
  • Fix: call assert_layout_valid(sizes_vec, {}, {}) in the three factories.

  Fix this →

• resolve_layout is a second copy of the runtime's stride formula, switched by a private flag (tensor_ptr.cpp:109-123, flag at :77).

  • It repeats runtime::dim_order_to_stride_nocheck (dim_order_util.h:126-150), size-0 rule included. Only the comment at :109 keeps the two in step.
  • Risk: clone_tensor_ptr passes runtime-built strides back in (tensor_ptr.cpp:258-291), and :128 compares them against this copy. If the two ever diverge, for example in the runtime change the commit message defers, cloning a loaded tensor starts aborting.
  • Suggestion: a check in dim_order_to_stride was declined because it would reject layouts the loader accepts today. A variant next to it that computes with c10::mul_overflows and reports overflow, without rejecting, avoids that:
    • the loader and resize accept the same layouts as today, and lose the same UB;
    • reject_stride_overflow becomes a result that only validate_tensor_layout acts on.
  • This is fine as a follow-up.

  Fix this →

• On Windows the stride computation keeps its signed-overflow UB (tensor_ptr.cpp:116).

  • Cause: MSVC and clang-cl both define _MSC_VER, so C10_HAS_BUILTIN_OVERFLOW() is 0 (runtime/core/portable_type/c10/c10/util/safe_numerics.h:9-10). For signed T, mul_overflows then computes volatile T tmp = a * b (:58-65), which is the plain multiply this PR replaces.
  • The commit message limits the claim to GCC and Clang; the PR description does not.
  • Fix: qualify the description, or compute the product in std::make_unsigned_t<StridesType>, where wrapping is defined, and check for overflow separately.

  Fix this →

Testing

• The tests don't pin down what make_tensor_ptr is supposed to keep doing (tensor_ptr_test.cpp:1104-1155).

  • No death tests for the new reject cases. They only call validate_tensor_layout. Not covered:
    • the description's case {2, 2}, {}, {8, 2} (inferred dim order);
    • 17 dims.
  • The existing death tests (:1080, :1096, :1375-1376) pass an explicit dim order or wrong lengths. The 150,000-layout comparison behind "rejects every layout make_tensor_ptr aborts on" isn't in the repo.
  • TensorOverflowingStrides (:1146-1155) skips the round trip. It doesn't check strides()[0], which is the wrapped value (0 in both modes), and it doesn't clone the tensor.
    • make_tensor_ptr accepts overflow so that cloning or aliasing such tensors keeps working (tensor_ptr.cpp:153-154).
    • In portable mode, clone_tensor_ptr recomputes this stride and compares it at :128. No test covers that round trip.

  Fix this →

API Design

• validate_tensor_layout rejects an overflowing stride on a size-1 dim, so its answer for one tensor depends on whether the dim order is passed (tensor_ptr.cpp:116-122).

  • Cause: :128 accepts any stride on a size-1 dim, but the overflow check runs first and makes no such exception.
  • Example (portable mode, 64-bit host): the {1, 65536, 32768} output the commit message cites. Its runtime strides are {INT32_MIN, 32768, 1} after the wrap.
    • Empty dim order and empty strides: InvalidArgument.
    • Its dim order {0, 1, 2} and its strides: InvalidArgument.
    • Empty dim order and its strides: Ok. The inferred order {1, 2, 0} puts dim 0 innermost, so nothing overflows.
  • Every element offset fits in int32_t (the largest is 2^31 - 1), and make_tensor_ptr accepts all three.
  • Fix: once a stride overflows, treat every dim further out as overflowed too, and reject only when an overflowed stride falls on a dim whose size is not 1.
    • Skipping size-1 dims alone is not enough. For {2, 1, 65536, 32768}, the wrapped value passes through INT32_MIN * 1 without a new overflow, so dim 0 would get a wrapped stride.

  Fix this →

• The uint8_t exception is missing from the vector template doc and from the guide's example (tensor_ptr.h:197-219, extension-tensor.md:167-177).

  • Template doc: it still says the type is deduced from the element type, and that the data is cast when type differs. For std::vector<uint8_t>, the raw-bytes overload (:436) wins the tie because it is a non-template, so neither happens. The pointer doc (:111-117) sends readers to this doc for the rule.
    • Example: make_tensor_ptr({2}, std::vector<uint8_t>{1, 2}, {}, {}, ScalarType::Int) reads the 2 bytes as Int and aborts at tensor_ptr.cpp:228-232, because it needs 8. The same call with std::vector<int16_t> casts.
  • Guide example: it doesn't say what data is. A file or socket usually gives a std::vector<uint8_t>. The call at :177 then reads it as Float, so it passes the numel check at :174 and then aborts.

  Fix this →

• The guide's recipe for untrusted input leaves out the scalar type (extension-tensor.md:165, :180).

  • A type read from the same input, such as static_cast<ScalarType>(100), aborts in two places:
    • elementSize (scalar_type_util.h:411-419), which :180 tells the reader to call;
    • the TensorImpl constructor (tensor_impl.cpp:93-94).
  • Fix: say to check isValid(type) first (scalar_type_util.h:376).

  Fix this →

Recommendation

Approve

Nothing blocks. Every item is minor and can land here or as a follow-up. Loosening the size-1 rule later accepts strictly more layouts, so existing callers would not break.