Make sure size_test CI uses -Wall -Werror by mergennachin · Pull Request #8016 · pytorch/executorch

mergennachin · 2025-01-28T21:33:06Z

I am not changing the default compilation option for all of ExecuTorch. I'm only changing the compile option for size_test and size_test_all_ops tests.

We don't run size_test for Windows, at least for today

Test Plan:

Run sh test/build_size_test.sh
Add op_fill.cpp with this patch at the end of the file.

int x;
printf("%d", x); // x is uninitialized

Make sure the compilation fails, https://gist.github.com/mergennachin/25b78c63069b9e9b2203e66f94b1a0be

Make sure the compilation succeeds without the "-Wall -Werror" flags.

pytorch-bot · 2025-01-28T21:33:11Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

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

mergennachin · 2025-01-28T21:36:56Z

Continuing where I left off: #7940

Not changing CMakeLists.txt this time.

swolchok

love it

swolchok · 2025-01-28T22:32:21Z

I don't see size_test in the CI jobs?

mergennachin · 2025-01-29T02:01:52Z

I don't see size_test in the CI jobs?

Just rebased (which contain CI fixes)

size_test for clang12 passed, but not for gcc9

https://github.com/pytorch/executorch/actions/runs/13022554836/job/36326025227?pr=8016

mergennachin · 2025-02-03T20:55:47Z

runtime/kernel/operator_registry.cpp

    return;
  }
-  strncpy(buf, "v1/", 3);
+  strcpy(buf, "v1/");


/pytorch/executorch/runtime/kernel/operator_registry.cpp:187:3: note: in expansion of macro 'ET_LOG_TENSOR_META' 187 | ET_LOG_TENSOR_META(meta_list); | ^~~~~~~~~~~~~~~~~~ In file included from /usr/include/string.h:535, from /usr/include/c++/9/cstring:42, from /pytorch/executorch/../executorch/runtime/kernel/operator_registry.h:11, from /pytorch/executorch/runtime/kernel/operator_registry.cpp:9: In function 'char* strncpy(char*, const char*, size_t)', inlined from 'void executorch::runtime::internal::make_kernel_key_string(executorch::runtime::Span<const executorch::runtime::TensorMeta>, char*)' at /pytorch/executorch/runtime/kernel/operator_registry.cpp:138:10: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:34: error: 'char* __builtin_strncpy(char*, const char*, long unsigned int)' output truncated before terminating nul copying 3 bytes from a string of the same length [-Werror=stringop-truncation]

the warning here was unhelpful IMO

Should this just be memcpy(buf, "v1/", 3);? Or are we relying on the NUL byte that strcpy adds?

I completely rewrote this function in #8327, so watch out for merge conflicts (if mine lands first)

yeah, thanks for the heads up, will be careful during rebase

mergennachin · 2025-02-03T20:59:31Z

Ignore 'sign-compare' but created a follow-up issue Compile with -Wsign-compare flag #8149
Fixed an error that was erroring with -Werror=stringop-truncation
Modified the CONTRIBUTION.md file to build the size_test information

facebook-github-bot · 2025-02-03T21:00:39Z

@mergennachin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-02-03T21:35:35Z

@mergennachin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mergennachin · 2025-02-03T21:59:52Z

kernels/portable/cpu/op_full.cpp

  ET_SWITCH_SCALAR_OBJ_TYPES(val_type, ctx, name, CTYPE_VAL, [&] {
    CTYPE_VAL val;
-    utils::extract_scalar(fill_value, &val);
+    ET_KERNEL_CHECK(


Had to adjust due to errors like this:

https://github.com/pytorch/executorch/actions/runs/13123283587/job/36614101657?pr=8016

I don't think ET_KERNEL_CHECK will do the right thing in a lambda -- it returns from the lambda, not the operator

any suggestions on how to fix?

i think it's fine, since it will return void from the lambda, but sets context to be fail, with error code InvalidArgument.

swolchok · 2025-02-04T00:09:32Z

kernels/portable/cpu/op_full.cpp

  ET_SWITCH_SCALAR_OBJ_TYPES(val_type, ctx, name, CTYPE_VAL, [&] {
    CTYPE_VAL val;
-    utils::extract_scalar(fill_value, &val);
+    ET_KERNEL_CHECK(


I don't think ET_KERNEL_CHECK will do the right thing in a lambda -- it returns from the lambda, not the operator

swolchok · 2025-02-04T00:09:39Z

runtime/kernel/operator_registry.cpp

    return;
  }
-  strncpy(buf, "v1/", 3);
+  strcpy(buf, "v1/");


the warning here was unhelpful IMO

swolchok · 2025-02-04T00:09:56Z

test/build_size_test.sh

 # shellcheck source=/dev/null
 source "$(dirname "${BASH_SOURCE[0]}")/../.ci/scripts/utils.sh"

+# Remove -Wno-sign-compare (https://github.com/pytorch/executorch/issues/8149)


did you mean to put a TODO here?

swolchok · 2025-02-08T00:53:44Z

@mergennachin do you need anything else from me on this one?

mergennachin · 2025-02-10T20:45:19Z

@mergennachin do you need anything else from me on this one?

@swolchok, @dbort

i don't think we have come to a resolution on errors on boolean errors:

https://github.com/pytorch/executorch/actions/runs/13123890485/job/36616081625?pr=8016

i can specialize for boolean cases, and use &&=

swolchok · 2025-02-10T20:52:09Z

&&= does not exist; you'd need &=. IMO such a change would be clear uglification of the code only to suppress a warning

mergennachin · 2025-02-10T21:06:43Z

such a change would be clear uglification of the code only to suppress a warning

But it's a legitimate warning, no? You can today pass in a random memory location and define it as "boolean" tensor, which will cause some undefined behavior.

unless you pass in UBSAN flag

swolchok · 2025-02-10T21:29:47Z

I don't think the code change you are considering would address the issue you just raised above.

mergennachin · 2025-02-10T23:08:51Z

Okay, I will suppress that error for now and create a follow-up issue to track it

CONTRIBUTING.md

test/build_size_test.sh

facebook-github-bot · 2025-02-11T02:29:31Z

@mergennachin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

digantdesai · 2025-02-11T05:50:35Z

kernels/portable/cpu/util/broadcast_util.cpp

-        b_shape_str.data());
+        tensor_shape_to_c_string(a_size_span).data(),
+        tensor_shape_to_c_string(b_size_span).data());
+#endif


swolchok · 2025-02-12T02:56:27Z

@mergennachin it seems suboptimal for us to enforce -Wall -Werror, but only in a CI job that runs only on trunk, as this will result in unnecessarily-delayed detection of broken trunk. could we perhaps have a pull-time CI job that builds with these flags?

swolchok · 2025-02-12T02:56:41Z

e.g. #8314 promptly broke this on trunk because of -Werror

mergennachin · 2025-02-12T15:03:21Z

@swolchok test-binary-size is a pull job

perhaps, in this #8314 case your base commit was old and it didn't catch during PR?

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 Jan 28, 2025

mergennachin added the topic: not user facing label Jan 28, 2025

mergennachin requested a review from swolchok January 28, 2025 21:35

mergennachin force-pushed the wall branch 2 times, most recently from 348754d to 89bd6ac Compare January 28, 2025 21:44

mergennachin added the ciflow/trunk label Jan 28, 2025

mergennachin requested review from GregoryComer, digantdesai and tarun292 January 28, 2025 22:02

swolchok approved these changes Jan 28, 2025

View reviewed changes

mergennachin force-pushed the wall branch from 89bd6ac to 5682316 Compare January 29, 2025 01:02

mergennachin mentioned this pull request Feb 3, 2025

Compile with -Wsign-compare flag #8149

Closed

mergennachin force-pushed the wall branch from 5682316 to ad9f0ed Compare February 3, 2025 20:54

mergennachin commented Feb 3, 2025

View reviewed changes

mergennachin requested a review from dbort February 3, 2025 20:56

mergennachin force-pushed the wall branch from ad9f0ed to 9a4b167 Compare February 3, 2025 20:57

mergennachin force-pushed the wall branch from 9a4b167 to c78ef20 Compare February 3, 2025 21:33

mergennachin force-pushed the wall branch from c78ef20 to a9d47d9 Compare February 3, 2025 21:56

mergennachin commented Feb 3, 2025

View reviewed changes

mergennachin force-pushed the wall branch from a9d47d9 to 44d6ae0 Compare February 3, 2025 22:14

swolchok reviewed Feb 4, 2025

View reviewed changes

mergennachin force-pushed the wall branch from 44d6ae0 to 7d915a9 Compare February 10, 2025 22:17

mergennachin marked this pull request as draft February 10, 2025 22:29

mergennachin force-pushed the wall branch 2 times, most recently from 988ee15 to ae885e7 Compare February 10, 2025 23:01

mergennachin mentioned this pull request Feb 11, 2025

Compile with -Wint-in-bool-context flag #8357

Open

mergennachin force-pushed the wall branch 3 times, most recently from 8b9425b to 0c82c8b Compare February 11, 2025 01:32

mergennachin marked this pull request as ready for review February 11, 2025 01:40

dbort reviewed Feb 11, 2025

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

dbort approved these changes Feb 11, 2025

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

test/build_size_test.sh Outdated Show resolved Hide resolved

Make sure size_test CI uses -Wall -Werror

92c525d

mergennachin force-pushed the wall branch from f8aa526 to 92c525d Compare February 11, 2025 02:27

mergennachin merged commit 4e6abf8 into main Feb 11, 2025
104 checks passed

mergennachin deleted the wall branch February 11, 2025 02:31

digantdesai reviewed Feb 11, 2025

View reviewed changes

Conversation

mergennachin commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

Uh oh!

mergennachin commented Jan 28, 2025

Uh oh!

swolchok left a comment

Choose a reason for hiding this comment

Uh oh!

swolchok commented Jan 28, 2025

Uh oh!

mergennachin commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergennachin commented Feb 3, 2025

Uh oh!

facebook-github-bot commented Feb 3, 2025

Uh oh!

facebook-github-bot commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swolchok commented Feb 8, 2025

Uh oh!

mergennachin commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swolchok commented Feb 10, 2025

Uh oh!

mergennachin commented Feb 10, 2025

Uh oh!

swolchok commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergennachin commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Feb 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swolchok commented Feb 12, 2025

Uh oh!

swolchok commented Feb 12, 2025

Uh oh!

mergennachin commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mergennachin commented Jan 28, 2025 •

edited

Loading

pytorch-bot bot commented Jan 28, 2025 •

edited

Loading

mergennachin commented Jan 29, 2025 •

edited

Loading

mergennachin commented Feb 10, 2025 •

edited

Loading

swolchok commented Feb 10, 2025 •

edited

Loading