Skip to content
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

Fix GCC 11 warns that writing 1 byte into a region of size 0 [-Wstringop-overflow=] #9578

Closed
wants to merge 1 commit into from
Closed

Fix GCC 11 warns that writing 1 byte into a region of size 0 [-Wstringop-overflow=] #9578

wants to merge 1 commit into from

Conversation

yma11
Copy link
Contributor

@yma11 yma11 commented Apr 23, 2024

This PR fixes following error popped during compilation using GCC 11:

..../Parquet_Read_Fuzz/ep/build-velox/build/velox_ep/./velox/vector/SelectivityVector.h:160:20: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
  160 |       allSelected_ = false;
      |       ~~~~~~~~~~~~~^~~~~~~

@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 Apr 23, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 23, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 65fb7d9
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66275f31eab6c20008a5c25c

acvictor
acvictor approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@acvictor acvictor left a comment

Choose a reason for hiding this comment

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

Love that these are getting caught!

@yma11
Copy link
Contributor Author

yma11 commented Apr 23, 2024

@Yuhta can you help take a look this change too? It's similar as 9526. By the way, do we consider adding a CI job for compilation using gcc 11?

Yuhta
Yuhta reviewed Apr 23, 2024
View reviewed changes
velox/vector/SelectivityVector.h
@@ -156,7 +156,7 @@ class SelectivityVector {
bits::fillBits(bits_.data(), 0, size_, false);
begin_ = 0;
end_ = 0;
allSelected_ = false;
allSelected_.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. allSelected_ has 3 states, nullopt, true, false, here we should set it to false, not nullopt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I intended to keep this logic but set it to false directly will cause the issue as mentioned, even with a check like if (allSelected_.has_value() && *allSelected_) first. The variable is used in isAllSelected() and countSelected(), which can also give correct return if it doesn't have value, so using allSelected_.reset() here is acceptable? This error is kind of code sensitive and I didn't search out a formal resolution for it, any other suggestion from you?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an optimization and we don't want to lose it. The issue apparently is a bug in compiler and should be worked around by some compiler flag/hints

velox/vector/SelectivityVector.h
@@ -279,7 +279,7 @@ class SelectivityVector {
if (begin_ == -1) {
begin_ = 0;
end_ = 0;
allSelected_ = false;
allSelected_.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@majetideepak majetideepak mentioned this pull request May 15, 2024
Open
@majetideepak
Copy link
Collaborator

By the way, do we consider adding a CI job for compilation using gcc 11?

@yma11 what is your OS version? We do build Velox on Ubuntu-22.04 which comes with GCC 11.4.0.
Here is a build https://github.com/facebookincubator/velox/actions/runs/9093396254/job/24992206168

@yma11
Copy link
Contributor Author

yma11 commented May 16, 2024 •
edited
Loading

By the way, do we consider adding a CI job for compilation using gcc 11?

@yma11 what is your OS version? We do build Velox on Ubuntu-22.04 which comes with GCC 11.4.0. Here is a build https://github.com/facebookincubator/velox/actions/runs/9093396254/job/24992206168

The root cause was finally identified that we made some changes in compile options in gluten and it causes this error pop up. Velox default compilation doesn't have this problem. Let me close this PR. Thanks for your info.

@yma11 yma11 closed this May 16, 2024
@Yohahaha
Copy link
Contributor

We found this issue when building pure meta/velox, just add -Wno-stringop-overflow to suppress it. fmtlib/fmt#3334 (comment)

This was referenced Jul 15, 2024
Merged
Closed
facebook-github-bot pushed a commit that referenced this pull request Jul 17, 2024
@majetideepak @facebook-github-bot
Fix GCC 11 warning: writing 1 byte into a region of size 0 [-Wstringo…
1375fa3 
…p-overflow=] (#10469)

Summary:
I hit this on ubuntu-22.04 arm64 with GCC11.
This was reported in the past as well #9578

Pull Request resolved: #10469

Reviewed By: Yuhta

Differential Revision: D59872624

Pulled By: kevinwilfong

fbshipit-source-id: 0fa03176dc664121103bf1f521369f1452a990d1
weiting-chen pushed a commit to weiting-chen/velox that referenced this pull request Nov 27, 2024
@majetideepak @weiting-chen
Fix GCC 11 warning: writing 1 byte into a region of size 0 [-Wstringo…
3743f83 
…p-overflow=] (facebookincubator#10469)

Summary:
I hit this on ubuntu-22.04 arm64 with GCC11.
This was reported in the past as well facebookincubator#9578

Pull Request resolved: facebookincubator#10469

Reviewed By: Yuhta

Differential Revision: D59872624

Pulled By: kevinwilfong

fbshipit-source-id: 0fa03176dc664121103bf1f521369f1452a990d1
@weiting-chen weiting-chen mentioned this pull request Nov 27, 2024
Merged
weiting-chen added a commit to oap-project/velox that referenced this pull request Nov 27, 2024
@weiting-chen @majetideepak
Fix GCC 11 warning: writing 1 byte into a region of size 0 [-Wstringo…
d3e2d4f 
…p-overflow=] (facebookincubator#10469) (#507)

Summary:
I hit this on ubuntu-22.04 arm64 with GCC11.
This was reported in the past as well facebookincubator#9578

Pull Request resolved: facebookincubator#10469

Reviewed By: Yuhta

Differential Revision: D59872624

Pulled By: kevinwilfong

fbshipit-source-id: 0fa03176dc664121103bf1f521369f1452a990d1

Co-authored-by: Deepak Majeti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yuhta Yuhta Yuhta left review comments

@acvictor acvictor acvictor approved these changes

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yma11 @majetideepak @Yohahaha @Yuhta @acvictor @facebook-github-bot