Skip to content

unix: workaround gcc bug on armv7 (part 2)#4564

Merged
santigimeno merged 1 commit intolibuv:v1.xfrom
santigimeno:santi/preadv_deopt
Oct 7, 2024
Merged

unix: workaround gcc bug on armv7 (part 2)#4564
santigimeno merged 1 commit intolibuv:v1.xfrom
santigimeno:santi/preadv_deopt

Conversation

@santigimeno
Copy link
Copy Markdown
Member

@santigimeno santigimeno commented Oct 5, 2024

Disable optimization on uv__preadv_or_pwritev.

Fixes: #4532
Fixes: #4550

@santigimeno santigimeno requested a review from bnoordhuis October 5, 2024 21:14
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Oct 6, 2024

That means you can undo the memcpy now (i.e. revert 32603fd)?

@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Oct 6, 2024

That means you can undo the memcpy now (i.e. revert 32603fd)?

Yes, I was going to propose that as well as I just noticed that the preadv tests never passed even with that fix, though I thought so: I just failed to see the failures 🤦‍♂️

@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Oct 6, 2024

Force pushed with the revert as first commit. PTAL

bnoordhuis
bnoordhuis approved these changes Oct 6, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I wouldn't keep the revert as a separate commit (lacks context without the next commit) but otherwise LGTM.

@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Oct 6, 2024

Ok, I was also reverting the doc update. If you're ok with it I'll squash them in 1 commit.

@santigimeno
unix: workaround gcc bug on armv7
eabcbbb 
Disable optimization on `uv__preadv_or_pwritev`.

Fixes: libuv#4532
Fixes: libuv#4550
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Oct 6, 2024

Yep, go for it.

@santigimeno santigimeno merged commit 0be52c8 into libuv:v1.x Oct 7, 2024
@santigimeno santigimeno deleted the santi/preadv_deopt branch October 7, 2024 07:51
@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Oct 7, 2024

Well, I fxxxed it up. Apparently this only fixes the issue when in combination with the memcpy() fix in 32603fd. I wrongly assumed disabling optimization would do it, but I should've known better and tested first. I'll open a PR to add it back. Sorry for the trouble.

@santigimeno santigimeno mentioned this pull request Oct 7, 2024
Merged
@bnoordhuis bnoordhuis mentioned this pull request Jan 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@saghul saghul saghul approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test failure on armv7l GNU/Linux fs: preadv failing on armv7

3 participants

@santigimeno @bnoordhuis @saghul