Simplify failureBreakpoint() and add a unit test.
#557
Conversation
This PR simplifies the "unique" action performed by `failureBreakpoint()` to avoid de-duplication and implements a unit test to confirm that action is actually occurring and not being optimized away (at least under the current compiler settings at the time of test.)
grynspan
added
enhancement
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@stmontgomery What do you think? This eliminates the need to capture a value |
|
@swift-ci please test |
| NoOp.perform(&NoOp.ignored) | ||
| // operation on a usable-from-inline value, which the compiler must assume | ||
| // cannot be optimized away. | ||
| failureBreakpointValue = 1 |
There was a problem hiding this comment.
If we were to enable the Thread Sanitizer and run our test suite, would it flag this as a data race? (Despite always assigning the same value)
There was a problem hiding this comment.
Nope, but if it did I would expect the original implementation would have the same issue.
There was a problem hiding this comment.
I'd be surprised if both of them did not report a TSan race. But you're saying you tried and they did not? 🤔
Either way, this PR seems fine. I just really want to avoid transforming this into a global incrementing counter in the future, since then it would require a lock and would increase the overhead of every test failure.
There was a problem hiding this comment.
I tried and didn't see any race reported even when I bombarded it with a whole bunch of concurrent detached tasks. So if TSan is watching that address, it needs more coffee.
But that all said, we can switch to an atomic store in the future if we did want a counter. No lock needed.
This PR simplifies the "unique" action performed by
failureBreakpoint()to avoid de-duplication and implements a unit test to confirm that action is actually occurring and not being optimized away (at least under the current compiler settings at the time of test.)Checklist: