cmcfarlen commented on PR #13616: URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5514618659
> FlagStorage uses `std::atomic<uint8_t>`, whose default constructor does not initialize the stored value. That was true through C++17, but [P0883](https://wg21.link/p0883) changed `std::atomic`'s default constructor to value-initialize the contained value in C++20, and this tree builds `-std=c++20`. It also does not depend on that change. `addBlob()` allocates with `std::make_unique<Metrics::NamesAndAtomics>()`, which is `new T()` — value-initialization — and that recurses through the tuple and the `std::array` to every element. Under the pre-C++20 rules the atomic's defaulted default constructor was trivial, so value-initialization zero-initialized its storage anyway. Both readings give a zero flag byte, so a new slot starts listed. I would rather not argue that from the standard, so 02d30f95db asserts it instead. The blob growth test already creates `MAX_SIZE + 100` metrics, which spans a blob boundary; it now also requires `listed(id)` for every one of them, so a full blob's worth of freshly allocated slots is checked. Confirmed the assertion is not vacuous. Storing `0xFF` into the flag array immediately after the allocation fails it: ``` test_Metrics.cc:600: FAILED: REQUIRE( h.listed(id) ) ``` That matters more than the standard argument, because reading uninitialized heap frequently *does* return zero — fresh pages are zero-filled — so this class of bug hides well and a test that only samples a metric or two would not catch it. I did not add an explicit initialization loop. It would be dead work on every blob, and the real risk is not today's behavior but a future change to something like `make_unique_for_overwrite`; the test catches that, and `addBlob()` now says so at the allocation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
