Repository navigation
Fix -Werror diagnostics in vendored runtime and kernel sources - #24702
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24702
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 95cef92 with merge base 4411b8d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
f7f85f7 to
df8386b
Compare
This PR needs a
|
JakeStevens
left a comment
There was a problem hiding this comment.
Thanks for cleaning these up! A few things I'd like changed before this lands.
Use ET_PRI_SIZES_AND_STRIDES for sizes/strides instead of per-site casts
There's already a format specifier for SizesType/StridesType elements. It's documented as "must be used instead of using d directly to support ATen mode", but in lean mode it's still "d" (runtime/core/portable_type/tensor_impl.h:337). That makes it wrong on exactly the toolchains this PR targets. Instead of fixing it, the PR casts at each call site, two different ways in the same header:
ET_CHECK_CONTIGUOUSandop_fft_r2c.cpp:static_cast<int64_t>+PRId64ET_CHECK_SAME_STRIDES2/3:(int32_t)+PRId32. In ATen mode this truncatesint64_tstrides, while the macro just above prints the same type at full width.
The macro's existing users are still broken. At this PR's head, extension/tensor/tensor_ptr.cpp:106-108 fails -Werror=format with 3 errors under ESP-IDF GCC 15.2 (xtensa-esp32s3-elf-g++ and riscv32-esp-elf-g++). extension/wasm/wasm_bindings.cpp:171-173 uses the same pattern. extension/tensor isn't vendored into the IDF component, but your out-of-tree argument for tensor_util.h applies to it too.
Suggested fix:
// runtime/core/portable_type/tensor_impl.h
#include <cinttypes>
...
#define ET_PRI_SIZES_AND_STRIDES PRId32Then use "%" ET_PRI_SIZES_AND_STRIDES with no casts at the four sites. strides[i] * sizes[i] has the element type in both modes, so it fits too. Where int32_t is int, PRId32 is "d", so host builds don't change. With this change, tensor_ptr.cpp and the rewritten macro messages compile cleanly under -Werror=format on both IDF toolchains and on host clang.
Smaller things
-
tensor_util.h:286-290, 319-325: theSTRIDES2/3loop index now prints withET_PRIsize_t, buticomes fromc10::irange(a__.dim()), so it'sssize_tin lean mode andint64_tin ATen mode. The first check in each macro (:276,:306) also still passesdim()to%zu. In ATen mode whereverint64_tislong long(e.g. macOS), plain-Wformatflags both. To fix, loop withsize_tasET_CHECK_CONTIGUOUSdoes, and printdim()as asize_tasET_CHECK_SAME_SHAPE3does. -
The
%zu→ET_PRIsize_tedits don't affect-Werror, since%zuwith asize_tis already clean on all the GCC targets named. They do leave the touched functions mixed:flat_tensor_data_map.cpp:359printsflat_tensor_data->size()with%zu, 14 lines above:373, which prints the same value withET_PRIsize_t.tensor_parser_exec_aten.cpp:115sits between the two edited checks.ET_CHECK_SAME_SHAPE3's message (tensor_util.h:108) still uses%zu.
I'd either convert the touched functions fully or leave the Cadence
%zumigration to its own change. -
op_prod.cpp:105:prod_int_outstill computes the bool product asacc * outv, and onlyprod_outusesmultiply(). It avoids the warning only because the conversion happens insidestd::tuple's constructor. Usingmultiply(acc, outv)there would keep one spelling in the file. -
op_im2row.cpp:13-17/op_transposed_im2row.cpp:16-20: the patched localALWAYS_INLINEis now the same asET_INLINE(runtime/platform/compiler.h:91-94, which also covers MSVC). Nothing definesDISABLE_ALWAYS_INLINE, so both blocks could be replaced withET_INLINE.
df8386b to
e8c8420
Compare
|
Thanks Jake — all addressed in e8c8420:
|
e8c8420 to
9642c76
Compare
On toolchains where int32_t and uint32_t are long (the Xtensa and RISC-V ESP-IDF toolchains, and arm-none-eabi-gcc), log and check messages that pass them to %d or %u trip -Wformat. ESP-IDF builds with -Werror, so its component currently compiles the vendored ExecuTorch sources with a blanket -Wno-error; these fixes let the next change narrow that. The runtime, flat_tensor and portable kernel sources now use PRId32/PRIu32, or cast to int where the rest of a message already does. ET_PRI_SIZES_AND_STRIDES, the format for elements of sizes() and strides(), was "d" in lean mode, which is wrong for int32_t on these toolchains, so its users such as extension/tensor's tensor_ptr.cpp failed -Werror=format there. It is now PRId32, which is "d" wherever int32_t is int. The size and stride messages in op_fft_r2c and in the ET_CHECK_CONTIGUOUS and ET_CHECK_SAME_STRIDES macros of tensor_util.h use it, so the STRIDES macros no longer truncate ATen-mode strides to int32_t, and those macros loop over and print dimensions as size_t, which kernels outside this tree also get. Other diagnostics that ESP-IDF turns into errors are fixed in the same sources: an always-true `index >= 0` on a uint32_t in flat_tensor, a cast to a const-qualified type in native_dropout, prod's `*=` on bool outputs (-Wint-in-bool-context), missing parentheses in ET_CHECK_SAME_SHAPE3, and always_inline on non-inline templates in the Cadence generic im2row operators, which now use ET_INLINE. prod's bool product, in both prod_out and prod_int_out, is now `&&` through a template helper, because `if constexpr` only discards a branch inside a template; results are unchanged for every dtype. Verified by compiling every vendored runtime and kernel source (281) as the generated component builds them for esp32s3 and esp32c6, under ESP-IDF 6.1's -Wall -Werror, at both -Os and -Og; by compiling extension/tensor/tensor_ptr.cpp and the tensor_util.h check macros with both ESP-IDF toolchains under -Werror=format and on host in ATen mode; and by running the component's emulator tests. On host, portable_kernels_test and the exec_aten util tests pass. Authored with Claude Code.
9642c76 to
95cef92
Compare
On toolchains where int32_t and uint32_t are long (the Xtensa and RISC-V
ESP-IDF toolchains, and arm-none-eabi-gcc), log and check messages that pass
them to %d or %u trip -Wformat. ESP-IDF builds with -Werror, so its component
currently compiles the vendored ExecuTorch sources with a blanket -Wno-error;
these fixes let the next change narrow that.
The runtime, flat_tensor and portable kernel sources now use PRId32/PRIu32, or
cast to int where the rest of a message already does. ET_PRI_SIZES_AND_STRIDES,
the format for elements of sizes() and strides(), was "d" in lean mode, which is
wrong for int32_t on these toolchains, so its users such as extension/tensor's
tensor_ptr.cpp failed -Werror=format there. It is now PRId32, which is "d"
wherever int32_t is int. The size and stride messages in op_fft_r2c and in the
ET_CHECK_CONTIGUOUS and ET_CHECK_SAME_STRIDES macros of tensor_util.h use it,
so the STRIDES macros no longer truncate ATen-mode strides to int32_t, and those
macros loop over and print dimensions as size_t, which kernels outside this
tree also get.
Other diagnostics that ESP-IDF turns into errors are fixed in the same sources:
an always-true
index >= 0on a uint32_t in flat_tensor, a cast to aconst-qualified type in native_dropout, prod's
*=on bool outputs(-Wint-in-bool-context), missing parentheses in ET_CHECK_SAME_SHAPE3, and
always_inline on non-inline templates in the Cadence generic im2row operators,
which now use ET_INLINE. prod's bool product, in both prod_out and prod_int_out,
is now
&&through a template helper, becauseif constexpronly discards abranch inside a template; results are unchanged for every dtype.
Verified by compiling every vendored runtime and kernel source (281) as the
generated component builds them for esp32s3 and esp32c6, under ESP-IDF 6.1's
-Wall -Werror, at both -Os and -Og; by compiling extension/tensor/tensor_ptr.cpp
and the tensor_util.h check macros with both ESP-IDF toolchains under
-Werror=format and on host in ATen mode; and by running the component's
emulator tests. On host, portable_kernels_test and the exec_aten util tests
pass.
Authored with Claude Code.