Skip to content

Tiny fixes to make gcc pedantic build happy #8933

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
Merged
merged 6 commits into from
Mar 25, 2025
Merged

Tiny fixes to make gcc pedantic build happy #8933

merged 6 commits into from
Mar 25, 2025

Conversation

digantdesai
Copy link
Contributor

@digantdesai digantdesai commented Mar 4, 2025 •
edited
Loading

Summary

Tiny fixes to make gcc 13 baremetal, pedantic build happy.

Test plan

CI

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Mar 4, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 4c0c697 with merge base c890809 (image):
💚 Looks good so far! There are no failures yet. 💚

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 Mar 4, 2025
kirklandsign
kirklandsign approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@kirklandsign kirklandsign left a comment

Choose a reason for hiding this comment

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

linter

@kirklandsign
Copy link
Contributor

Also please update the title/commit message a bit. It has more than Fixup CMake summary message

@github-actions github-actions bot mentioned this pull request Mar 10, 2025
Open
@github-actions github-actions bot mentioned this pull request Mar 17, 2025
Open
@digantdesai digantdesai requested a review from swolchok as a code owner March 21, 2025 02:58
@digantdesai digantdesai changed the title Fixup CMake summary message Tiny fixes to make gcc pedantic build happy Mar 21, 2025
swolchok
swolchok requested changes Mar 21, 2025
View reviewed changes
Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

changes requested for strncmp bug

kernels/portable/cpu/op_constant_pad_nd.cpp Outdated
@@ -189,7 +189,7 @@ Tensor& constant_pad_nd_out(
CTYPE value_v;
ET_SWITCH_SCALAR_OBJ_TYPES(
value_type, ctx, "constant_pad_nd.out", CTYPE_VALUE, [&]() {
CTYPE_VALUE val;
CTYPE_VALUE val{};
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the error message motivating this? it seems silly and reminiscent of misguided linter advice I've seen previously, but perhaps I don't know something. Is it -Wuninitialized? If so, why empty braces instead of = 0;?

Copy link
Contributor Author

@digantdesai digantdesai Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah it was that flag. COMPLEX_TYPES is what I was thinking about when not using =0. But I guess I can use it when there is no complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

runtime/executor/method_meta.cpp Outdated
@@ -233,7 +233,7 @@ bool MethodMeta::uses_backend(const char* backend_name) const {
const auto delegates = s_plan_->delegates();
for (size_t i = 0; i < delegates->size(); i++) {
auto delegate = delegates->Get(i);
if (std::strcmp(delegate->id()->c_str(), backend_name) == 0) {
if (strncmp(delegate->id()->c_str(), backend_name, delegate->id()->size()) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. nit: why drop std::?
  2. this is incorrect -- it passes if the delegate ID is a prefix of the backend_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std:: is an oversight.

Prefix is a real issue, let me also check the length of the incoming in addition to strncmp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

swolchok
swolchok reviewed Mar 21, 2025
View reviewed changes
runtime/executor/method_meta.cpp
Comment on lines +236 to +240
auto backend_name_len = std::strlen(backend_name);
auto delegate_id_len = delegate->id()->size();
if (backend_name_len == delegate_id_len &&
std::strncmp(delegate->id()->c_str(), backend_name, backend_name_len) ==
0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you know what, I'm really confused about what error we're fixing here. how is this better than strcmp?

swolchok
swolchok approved these changes Mar 21, 2025
View reviewed changes
Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

unblocking, but I'd like to hear about why we can't simply use strcmp

@digantdesai
Copy link
Contributor Author

unblocking, but I'd like to hear about why we can't simply use strcmp

Thanks. Basically -Werror=stringop-overread was triggering here. I tried some tweaks but couldn't get it to stop on strcmp so just did strncmp.

@swolchok
Copy link
Contributor

-Werror=stringop-overread was triggering here.

IIRC this one being buggy in some random gcc version has come up previously, possibly on @mergennachin's size_test warning stuff? In general I don't think we should be held responsible for every warning in every quasi-reasonable version of every compiler because of stuff like this.

@digantdesai
Copy link
Contributor Author

yeah this was arm's bare metal gcc 13, I generally agree with you. That said since this was the only one I got tempted 😅

@github-actions github-actions bot mentioned this pull request Mar 24, 2025
Open
@digantdesai digantdesai merged commit 12f4431 into main Mar 25, 2025
81 checks passed
@digantdesai digantdesai deleted the fix-warnings branch March 25, 2025 15:39
@github-actions github-actions bot mentioned this pull request Mar 31, 2025
Open
@github-actions github-actions bot mentioned this pull request Apr 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swolchok swolchok swolchok approved these changes

@kirklandsign kirklandsign kirklandsign approved these changes

@dbort dbort Awaiting requested review from dbort

@JacobSzwejbka JacobSzwejbka Awaiting requested review from JacobSzwejbka JacobSzwejbka is a code owner

@lucylq lucylq Awaiting requested review from lucylq lucylq is a code owner

@manuelcandales manuelcandales Awaiting requested review from manuelcandales manuelcandales is a code owner

@GregoryComer GregoryComer Awaiting requested review from GregoryComer

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@digantdesai @kirklandsign @swolchok @facebook-github-bot