crashpad: Convert legacy atomics -> std::atomic<> [crashpad/crashpad : main]
Venkatesh Srinivas (Gerrit)
Venkatesh Srinivas added 1 comment![]( "Open in Gerrit")
https://chromium-review.googlesource.com/c/chromium/src/+/6598917 removes Barrier_AtomicIncrement; I missed that crashpad used it - my initial test runs didn't cover crashpad. Removing here.
Related details
- Benoit Lize
- Mark Mentovai
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
https://chromium-review.googlesource.com/c/chromium/src/+/6598917 removes Barrier_AtomicIncrement; I missed that crashpad used it - my initial test runs didn't cover crashpad. Removing here.
https://chromium-review.googlesource.com/c/chromium/src/+/6598917 removes Barrier_AtomicIncrement; I missed that crashpad used it - my initial test runs didn't cover crashpad. Removing here.
You'll also need to remove it from mini_chromium in a separate change and then roll that in here.
Related details
- Benoit Lize
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benoit Lize
| Code-Review | +1 |
1 comment
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Venkatesh Srinivas (Gerrit)
Venkatesh Srinivas added 1 comment![]( "Open in Gerrit")
Mark Mentovaihttps://chromium-review.googlesource.com/c/chromium/src/+/6598917 removes Barrier_AtomicIncrement; I missed that crashpad used it - my initial test runs didn't cover crashpad. Removing here.
https://chromium-review.googlesource.com/c/chromium/src/+/6598917 removes Barrier_AtomicIncrement; I missed that crashpad used it - my initial test runs didn't cover crashpad. Removing here.
You'll also need to remove it from mini_chromium in a separate change and then roll that in here.
https://chromium-review.googlesource.com/c/chromium/mini_chromium/+/6786567 to remove from mini_chromium.
I think we should remove the user from here; then we can remove the mini_chromium implementation; then can remove the implementation in base/.
Related details
- Mark Mentovai
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
I’d like to include https://chromium-review.googlesource.com/c/6786567 in a mini_chromium roll in this change, but it’s waiting on Benoit’s review.
Once that happens, we can include the mini_chromium roll here via a DEPS change, and then land this too.
Thanks!
(startup_state = g_handler_startup_state.load(std::memory_order_acquire)) ==This ran past 80 columns. Run `git cl format`.
Related details
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benoit Lize
| Code-Review | +1 |
1 comment
static std::atomic<int> have_crashed;This sounds like conceptually a bool?
Related details
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Venkatesh Srinivas (Gerrit)
Venkatesh Srinivas added 2 comments![]( "Open in Gerrit")
(startup_state = g_handler_startup_state.load(std::memory_order_acquire)) ==This ran past 80 columns. Run `git cl format`.
Done
static std::atomic<int> have_crashed;This sounds like conceptually a bool?
Logically, it is a bool - I didn't change it because UnhandledExceptionHandler() increments it for every crashing thread (and non-first threads park themselves). I'd be very happy to swap that to bool/exchange, but have been trying to keep these changes to be as mechanical as possible.
WDYT?
Related details
- Benoit Lize
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
(startup_state = g_handler_startup_state.load(std::memory_order_acquire)) ==Venkatesh SrinivasThis ran past 80 columns. Run `git cl format`.
Done
Not done as of https://chromium-review.googlesource.com/c/crashpad/crashpad/+/6785253/2/client/crashpad_client_win.cc#162. Be sure to `git commit` any changes and then `git cl upload` to get them into the code review system.
Related details
- Benoit Lize
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Venkatesh Srinivas (Gerrit)
Venkatesh Srinivas added 1 comment![]( "Open in Gerrit")
(startup_state = g_handler_startup_state.load(std::memory_order_acquire)) ==Venkatesh SrinivasThis ran past 80 columns. Run `git cl format`.
Mark MentovaiDone
Not done as of https://chromium-review.googlesource.com/c/crashpad/crashpad/+/6785253/2/client/crashpad_client_win.cc#162. Be sure to `git commit` any changes and then `git cl upload` to get them into the code review system.
Operator error on my part -- now done as of patch #3.
Related details
- Benoit Lize
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
static std::atomic<int> have_crashed;Venkatesh SrinivasThis sounds like conceptually a bool?
Logically, it is a bool - I didn't change it because UnhandledExceptionHandler() increments it for every crashing thread (and non-first threads park themselves). I'd be very happy to swap that to bool/exchange, but have been trying to keep these changes to be as mechanical as possible.
WDYT?
Logically, it is a bool - I didn't change it because UnhandledExceptionHandler() increments it for every crashing thread (and non-first threads park themselves). I'd be very happy to swap that to bool/exchange, but have been trying to keep these changes to be as mechanical as possible.
WDYT?
- Benoit Lize
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Venkatesh Srinivas (Gerrit)
Venkatesh Srinivas added 1 comment![]( "Open in Gerrit")
static std::atomic<int> have_crashed;Venkatesh SrinivasThis sounds like conceptually a bool?
Mark MentovaiLogically, it is a bool - I didn't change it because UnhandledExceptionHandler() increments it for every crashing thread (and non-first threads park themselves). I'd be very happy to swap that to bool/exchange, but have been trying to keep these changes to be as mechanical as possible.
WDYT?
Logically, it is a bool - I didn't change it because UnhandledExceptionHandler() increments it for every crashing thread (and non-first threads park themselves). I'd be very happy to swap that to bool/exchange, but have been trying to keep these changes to be as mechanical as possible.
WDYT?
I think it should be `std::atomic<bool>::exchange(true)`.
- Benoit Lize
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Benoit Lize
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Venkatesh Srinivas (Gerrit)
Venkatesh Srinivas added 1 comment![]( "Open in Gerrit")
I’d like to include https://chromium-review.googlesource.com/c/6786567 in a mini_chromium roll in this change, but it’s waiting on Benoit’s review.
Once that happens, we can include the mini_chromium roll here via a DEPS change, and then land this too.
Thanks!
That change has landed - should the DEPS change to be part of this CR or separate?
Related details
- Benoit Lize
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Venkatesh SrinivasI’d like to include https://chromium-review.googlesource.com/c/6786567 in a mini_chromium roll in this change, but it’s waiting on Benoit’s review.
Once that happens, we can include the mini_chromium roll here via a DEPS change, and then land this too.
Thanks!
That change has landed - should the DEPS change to be part of this CR or separate?
That change has landed - should the DEPS change to be part of this CR or separate?
- Benoit Lize
- Venkatesh Srinivas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |