[PATCH] D92415: [GWP-ASan] Fix flaky test on Fuchsia

Tue Dec 1 13:28:36 PST 2020

mcgrathr accepted this revision.
mcgrathr added a comment.
This revision is now accepted and ready to land.

In D92415#2426531 <https://reviews.llvm.org/D92415#2426531>, @hctim wrote:

> In D92415#2426397 <https://reviews.llvm.org/D92415#2426397>, @mcgrathr wrote:
>
>> I'm troubled in two ways.
>>
>> 1. If this test is polluted by global (TLS) state from prior tests, that seems like a non-hermeticity problem for all these tests.  Should we maybe be using a test fixture with SetUp/TearDown to wipe the global state between tests?
>
> In other environments (both for upstream testing and Android <https://cs.android.com/android/platform/superproject/+/master:external/gwp_asan/Android.bp;drc=97b33726d10616f2531366bb38534bdead7bbb53;l=187>) we run this test in isolation to avoid this problem.

What kind of isolation?  In the upstream tests/CMakeLIsts.txt I don't see anything segregating this test from others.  I see that Android build file has a comment and "isolated: true", but I have no idea what that means because I am not familiar with the Android build system or test-running facilities.

> I'm happy with Kostya's solution to spawn a thread, or resetting the TLS in the start of this test (`*gwp_asan::getThreadLocals() = ThreadLocalPackedVariables();`)

The latter seems mildly preferable to me.  That is, it makes clear that this particular TLS state is not doing something useful across tests, it just needs to be wiped.

>> 2. It seems like the code under test actually has semantics that when uninitialized, it will sometimes sample (just rarely).  That seems like a problem to me.  If the initialized state as configured by the environment will be to never do a gwp allocation, then there should never be a window where one is randomly allowed to happen.
>
> There's an uninitialized check in the slow path. We might enter `GPA::allocate()` after `1 << 31` allocations, but we never actually provide a GWP-ASan allocation (it just returns `nullptr`) and we reset the counter to wait another `1 << 31` times before entering the slow path again.

I see.  Thanks for the explanation.  That eliminates my only concern that wasn't purely about how the test works, and I'm much more sanguine deferring to you on the test scenario.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92415/new/

https://reviews.llvm.org/D92415