fix static initialization fiasco problem

[zyfjeff]

Signed-off-by: tianqian.zyf tianqian.zyf@alibaba-inc.com

Description: fix static initialization fiasco problem in #4288
Risk Level: low
Testing: N/A
Docs Changes: N/A
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

[htuch]

I thought static initialization fiasco only applies to non-PoD... is this addressing a bug you noticed?

[zyfjeff]

static order initialization fiasco for the non-Pod types of data still exist problems, because I'm here to initialize the SEED using a function call

https://isocpp.org/wiki/faq/ctors#static-init-order-on-intrinsics

When I was in fix #4288, accidentally triggered a asan error, so I think this place is a potential problem
related discussion

==21403==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x00000b5e4e20 at pc 0x000003961e2a bp 0x7ffeb6aada40 sp 0x7ffeb6aada38
READ of size 4 at 0x00000b5e4e20 thread T0
    #0 0x3961e29 in std::__atomic_base<unsigned int>::load(std::memory_order) const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/atomic_base.h:396:9
    #1 0x3961e29 in absl::base_internal::SpinLock::TryLockImpl() /proc/self/cwd/external/com_google_absl/absl/base/internal/spinlock.h:167
    #2 0x395ec13 in absl::base_internal::SpinLock::Lock() /proc/self/cwd/external/com_google_absl/absl/base/internal/spinlock.h:81:10
    #3 0x399291a in absl::GetCurrentTimeNanosSlowPath() /proc/self/cwd/external/com_google_absl/absl/time/clock.cc:393:8
    #4 0x39927cc in absl::GetCurrentTimeNanos() /proc/self/cwd/external/com_google_absl/absl/time/clock.cc:353:10
    #5 0x39911eb in absl::Now() /proc/self/cwd/external/com_google_absl/absl/time/clock.cc:39:15
    #6 0x588fcc in __cxx_global_var_init.49 /proc/self/cwd/external/envoy/test/test_common/utility.cc:37:47
    #7 0x5891c2 in _GLOBAL__sub_I_utility.cc /proc/self/cwd/external/envoy/test/test_common/utility.cc

[htuch]

OK, makes sense (https://isocpp.org/wiki/faq/ctors#static-init-order-on-intrinsics). Can you make this a static internally in this function, so that if we instantiate another TestRandomGenerator object, we get the same seed? Thanks.

[zyfjeff]

This is the previous talk about static
#4288 (comment)

It will only get called once (per test) when TestRandomGenerator is constructed, and that should be fine.

[htuch]

I've replied at #4288 (comment), we can discuss in that thread, thanks.

[jmarantz]

#4288 is now a minimal fix to an unrelated bug, which doesn't touch this seed-construction at all (anymore); my suggestion is to work toward merging there; and discuss the static fiasco here.

I am not understanding why TestRandomGenerator wants to get the same seed on each test run within a single process, but is OK getting different seeds when linked into multiple processes. One way that's confusing is that the coverage run would get a single seed for the entire suite of envoy tests, whereas the unit tests ordinarily would get a different seed for each _test.cc. If there's some rationale why that needs to work that way, it would be good to have a comment.

But in any case, if we really do want to share the seed within the binary, we need to do that in a non-fiasco way :) Our attention was called to the fiasco by the explicit use of that term in the ASAN test that failed when @zyfjeff switched from std::chrono to absl::Time in the static initialization expression, in an earlier version of #4288 . My suspicion is that the absl::Time function had its own evil static construction and the linker chose to run that after this one. I didn't investigate deeply because I prefer to avoid static initialization involving function calls.

I'm also not entirely clear when the static initialization is lazy (which at least makes order deterministic). I might have thought it would be lazy here but the ASAN error suggested it was not.

gtest does provide a mechanism for complex static initialization: SetUpTestCase. Could we init the seed in that (if it really needs to be static)?

[htuch]

I think we're in agreement that we should fix the static initialization order fiasco and that this current PR is roughly the right thing, just trying to zero in on some minor detail.

I agree that we don't need to have every TestRandomGenerator have the exact same seed (in fact in the interest of intended random behavior, it would be ideal that if we have more than one instance that the do use slightly different seeds). However, what we do need is the ability to be able to replay deterministically the behavior that is provided from logs.

In this current PR, if we don't have the CLI flag set and have multiple generators, we actually have two dynamically created seeds. Later on, if the user wants to recreate behavior, which of these logged seeds do they use? Either way, the rerun won't exhibit the original behavior.

What I'm suggesting is to make an inner variable of getSeed() static. C++ is very clear that this will then only get initialized on first use and solve the static initialization order fiasco. We can chat IRL; I don't think this is controversial.

On a related note, why do we even need this "initialize seed from time" ability? Doesn't gtest always set random_seed? Is this for some situation that we are outside of gtest??

[zyfjeff]

If you say that from the point of view that you can replay and reproduce the problem, I recognize that SEED should be static.

[jmarantz]

lgtm -- I forgot that SEED was previously a statically initialized variable. So making it a lazy-initialized static in a helper function will solve the fiasco.

Can you just add a comment that this wants to be a static so there's just one seed specified for replay?

[zyfjeff]

Ok

[dnoe]

Looks like the CI failure is some CircleCI resource problem. Please make an empty commit to re-trigger the CI.

Check out https://gist.github.com/htuch/79caffa2b16eaeda8f74093da4a71326 for an example you can add to your git config to do this easily.

[zyfjeff]

@dnoe I rebase the master code and re-trigger CI.

[dnoe]

Perfect, that should work too. I see Circle CI has some alert about known resource issue right now, so let's hope it's OK this time.

[htuch]

Thanks!