Fix the failure memory order argument to compareExchangeStrong #22049

AtariDreams · 2023-12-19T19:20:34Z

`d2ba4a0`

Fix the failure memory order argument to compareExchangeStrong
https://bugs.webkit.org/show_bug.cgi?id=266649

Reviewed by Kimmo Kinnunen.

The failure memory order cannot be release or acq_rel.
Clang since llvm/llvm-project@fed5644 diagnoses an invalid argument.
Furthermore, the current way it is written is undefined behavior anyway.

From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange:
"If failure is stronger than success or(until C++17) is one of
std::memory_order_release and std::memory_order_acq_rel, the behavior is
undefined."

* Source/WebKit/Platform/IPC/StreamClientConnectionBuffer.h:
  (IPC::StreamClientConnectionBuffer::tryAcquire): Use
  memory_order_acquire as fallback.
* Source/WebKit/Platform/IPC/StreamServerConnectionBuffer.h:
  (IPC::StreamServerConnectionBuffer::tryAcquire): Ditto.

Canonical link: https://commits.webkit.org/272461@main

83b9eb4

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
~~🧪 style~~	~~🛠 ios~~	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
	~~🛠 ios-sim~~	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 gtk~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~
✅ 🛠 🧪 merge	~~🛠 watch~~
	~~🛠 watch-sim~~

webkit-early-warning-system · 2023-12-19T19:21:00Z

EWS run on previous version of this PR (hash 202718e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 wincairo
	⏳ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	~~🧪 api-mac~~	⏳ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt		⏳ 🛠 gtk
⏳ 🛠 🧪 jsc	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc-arm64	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	⏳ 🛠 tv-sim		⏳ 🛠 jsc-armv7
	⏳ 🛠 watch		⏳ 🧪 jsc-armv7-tests
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-12-19T19:27:25Z

EWS run on previous version of this PR (hash a96afde)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	⏳ 🧪 api-mac	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	✅ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	⏳ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	⏳ 🛠 tv-sim
	✅ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-12-19T20:42:37Z

EWS run on previous version of this PR (hash dd07c43)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt		✅ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-12-20T00:57:19Z

EWS run on previous version of this PR (hash b476b50)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt		✅ 🛠 gtk
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	⏳ 🧪 mac-wk2	⏳ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	⏳ 🛠 tv-sim
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-12-20T02:36:57Z

EWS run on previous version of this PR (hash f19157d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

AtariDreams · 2023-12-21T18:28:21Z

@kkinnunen-apple Could you please review this?

AtariDreams · 2023-12-21T21:31:46Z

@aproskuryakov Do you think this is safe to merge? Considering the failure only means we have to acquire anyway, no behavior should really change. I would be surprised if it generated different binary honestly.

aproskuryakov · 2023-12-21T21:53:48Z

I am not familiar with this code, neither WebKit side nor C++ library side.

aprotyas

This will need a review from someone who's familiar with WTF::Atomic.

I think the change is good, but you should have explained better in your changelog that the status quo is undefined behavior. Can you update your commit message to reflect that?

From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

If failure is stronger than success or(until C++17) is one of std::memory_order_release and std::memory_order_acq_rel, the behavior is undefined.

AtariDreams · 2023-12-21T22:33:43Z

This will need a review from someone who's familiar with WTF::Atomic.

I think the change is good, but you should have explained better in your changelog that the status quo is undefined behavior. Can you update your commit message to reflect that?

From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

If failure is stronger than success or(until C++17) is one of std::memory_order_release and std::memory_order_acq_rel, the behavior is undefined.

Done!

webkit-early-warning-system · 2023-12-21T22:37:19Z

EWS run on previous version of this PR (hash d811db7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-12-22T02:40:46Z

EWS run on previous version of this PR (hash ef1bc7b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-12-22T04:21:38Z

EWS run on previous version of this PR (hash 886c433)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-12-22T05:41:11Z

EWS run on previous version of this PR (hash a7ba673)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~		✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	~~🛠 tv-sim~~
	~~🛠 watch~~
	✅ 🛠 watch-sim

AtariDreams · 2023-12-22T15:12:28Z

@aproskuryakov Can we please merge this?

MenloDorian · 2023-12-22T15:53:36Z

This will need a review from someone who's familiar with WTF::Atomic.
I think the change is good, but you should have explained better in your changelog that the status quo is undefined behavior. Can you update your commit message to reflect that?
From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

If failure is stronger than success or(until C++17) is one of std::memory_order_release and std::memory_order_acq_rel, the behavior is undefined.

Done!

Please add the url above in the commit message as well. The url provides details to explain that this is the correct fix. Thanks.

AtariDreams · 2023-12-22T18:03:50Z

From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

Done!

AtariDreams · 2023-12-22T18:24:23Z

From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

Done!

@MenloDorian Can we please merge this now?

aprotyas · 2023-12-22T18:49:19Z

I'll merge this later today.

webkit-early-warning-system · 2023-12-22T19:01:32Z

EWS run on previous version of this PR (hash dc080e5)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
~~🧪 style~~	~~🛠 ios~~	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
	~~🛠 ios-sim~~	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 gtk~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~
	~~🛠 watch~~
	~~🛠 watch-sim~~

AtariDreams · 2023-12-22T19:02:53Z

I'll merge this later today.

Alright. I added the requested changes to the actual pull request message.

webkit-early-warning-system · 2023-12-22T19:18:38Z

EWS run on current version of this PR (hash 83b9eb4)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
~~🧪 style~~	~~🛠 ios~~	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
	~~🛠 ios-sim~~	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 gtk~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~
✅ 🛠 🧪 merge	~~🛠 watch~~
	~~🛠 watch-sim~~

https://bugs.webkit.org/show_bug.cgi?id=266649 Reviewed by Kimmo Kinnunen. The failure memory order cannot be release or acq_rel. Clang since llvm/llvm-project@fed5644 diagnoses an invalid argument. Furthermore, the current way it is written is undefined behavior anyway. From https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange: "If failure is stronger than success or(until C++17) is one of std::memory_order_release and std::memory_order_acq_rel, the behavior is undefined." * Source/WebKit/Platform/IPC/StreamClientConnectionBuffer.h: (IPC::StreamClientConnectionBuffer::tryAcquire): Use memory_order_acquire as fallback. * Source/WebKit/Platform/IPC/StreamServerConnectionBuffer.h: (IPC::StreamServerConnectionBuffer::tryAcquire): Ditto. Canonical link: https://commits.webkit.org/272461@main

webkit-commit-queue · 2023-12-22T21:05:11Z

Committed 272461@main (d2ba4a0): https://commits.webkit.org/272461@main

Reviewed commits have been landed. Closing PR #22049 and removing active labels.

AtariDreams requested a review from cdumez as a code owner December 19, 2023 19:20

AtariDreams force-pushed the atomic branch from 202718e to a96afde Compare December 19, 2023 19:26

Ahmad-S792 assigned Ahmad-S792 and AtariDreams and unassigned Ahmad-S792 Dec 19, 2023

Ahmad-S792 added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Dec 19, 2023

AtariDreams force-pushed the atomic branch from a96afde to dd07c43 Compare December 19, 2023 20:42

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 20, 2023

AtariDreams force-pushed the atomic branch from dd07c43 to b476b50 Compare December 20, 2023 00:55

AtariDreams force-pushed the atomic branch from b476b50 to f19157d Compare December 20, 2023 02:36

aproskuryakov requested a review from kkinnunen-apple December 20, 2023 18:00

aprotyas reviewed Dec 21, 2023

View reviewed changes

AtariDreams force-pushed the atomic branch from f19157d to d811db7 Compare December 21, 2023 22:37

AtariDreams force-pushed the atomic branch from d811db7 to ef1bc7b Compare December 22, 2023 02:40

AtariDreams force-pushed the atomic branch from ef1bc7b to 886c433 Compare December 22, 2023 04:21

AtariDreams force-pushed the atomic branch from 886c433 to a7ba673 Compare December 22, 2023 05:41

aprotyas added the skip-ews Applied to prevent a change from being run on EWS label Dec 22, 2023

kkinnunen-apple approved these changes Dec 22, 2023

View reviewed changes

AtariDreams force-pushed the atomic branch from a7ba673 to dc080e5 Compare December 22, 2023 19:01

AtariDreams force-pushed the atomic branch from dc080e5 to 83b9eb4 Compare December 22, 2023 19:18

webkit-commit-queue force-pushed the atomic branch from 83b9eb4 to d2ba4a0 Compare December 22, 2023 21:05

webkit-commit-queue merged commit d2ba4a0 into WebKit:main Dec 22, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 22, 2023

AtariDreams deleted the atomic branch December 22, 2023 21:10

This was referenced Apr 4, 2024

Weaken memory ordering of objects we do not care about #22235

Closed

[WTF] Make compareExchangeWeak have relaxed memory order after failure #22248

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the failure memory order argument to compareExchangeStrong #22049

Fix the failure memory order argument to compareExchangeStrong #22049

AtariDreams commented Dec 19, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Dec 19, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 19, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 19, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 20, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 20, 2023 •

edited

Loading

AtariDreams commented Dec 21, 2023

AtariDreams commented Dec 21, 2023

aproskuryakov commented Dec 21, 2023

aprotyas left a comment

AtariDreams commented Dec 21, 2023

webkit-early-warning-system commented Dec 21, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

AtariDreams commented Dec 22, 2023

MenloDorian commented Dec 22, 2023 •

edited

Loading

AtariDreams commented Dec 22, 2023

AtariDreams commented Dec 22, 2023

aprotyas commented Dec 22, 2023

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

AtariDreams commented Dec 22, 2023

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

webkit-commit-queue commented Dec 22, 2023

Fix the failure memory order argument to compareExchangeStrong #22049

Fix the failure memory order argument to compareExchangeStrong #22049

Conversation

AtariDreams commented Dec 19, 2023 • edited by webkit-early-warning-system Loading

d2ba4a0

webkit-early-warning-system commented Dec 19, 2023 • edited Loading

webkit-early-warning-system commented Dec 19, 2023 • edited Loading

webkit-early-warning-system commented Dec 19, 2023 • edited Loading

webkit-early-warning-system commented Dec 20, 2023 • edited Loading

webkit-early-warning-system commented Dec 20, 2023 • edited Loading

AtariDreams commented Dec 21, 2023

AtariDreams commented Dec 21, 2023

aproskuryakov commented Dec 21, 2023

aprotyas left a comment

Choose a reason for hiding this comment

AtariDreams commented Dec 21, 2023

webkit-early-warning-system commented Dec 21, 2023 • edited Loading

webkit-early-warning-system commented Dec 22, 2023 • edited Loading

webkit-early-warning-system commented Dec 22, 2023 • edited Loading

webkit-early-warning-system commented Dec 22, 2023 • edited Loading

AtariDreams commented Dec 22, 2023

MenloDorian commented Dec 22, 2023 • edited Loading

AtariDreams commented Dec 22, 2023

AtariDreams commented Dec 22, 2023

aprotyas commented Dec 22, 2023

webkit-early-warning-system commented Dec 22, 2023 • edited Loading

AtariDreams commented Dec 22, 2023

webkit-early-warning-system commented Dec 22, 2023 • edited Loading

webkit-commit-queue commented Dec 22, 2023

AtariDreams commented Dec 19, 2023 •

edited by webkit-early-warning-system

Loading

`d2ba4a0`

webkit-early-warning-system commented Dec 19, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 19, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 19, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 20, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 20, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 21, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

MenloDorian commented Dec 22, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 22, 2023 •

edited

Loading