#17754: Lower Indestructible to Metal, add guidance on using static vars with non-trivial destructors #17899
Conversation
omilyutin-tt
requested review from
dmakoviichuk-tt,
rfurko-tt,
patrickroberts,
ayerofieiev-tt,
sminakov-tt,
abhullar-tt,
pgkeller,
aliuTT,
tt-aho,
tt-dma,
tt-asaigal,
ubcheema,
cfjchu and
a team
as code owners
| @@ -1,4 +1,4 @@ | |||
| # Best Practices for C++20 Repository | |||
| # Best Practices for Contributing to TT Metal | |||
There was a problem hiding this comment.
Dropping "C++20" part. This is misleading in some cases, for example, the doc suggests to use tt:stl::Span while C++20 has std::span
contributing/BestPractices.md
Outdated
|
|
||
| // Option 2: If dynamic initialization is required, use function-local statics with `Indestructible`. | ||
| const auto& get_data() { | ||
| static Indestructible<std::string> kData("Disabled destructor! Good!"); |
There was a problem hiding this comment.
mb provide another example as seems like static std::string is always bad anyway.
| // Bad: Using a static object with a non-trivial destructor. | ||
| static const std::map<int, std::string> kDeviceConfigFiles = { | ||
| {1, "n150.yaml"}, | ||
| {2, "n300.yaml"}, | ||
| {8, "t3000.yaml"} | ||
| }; | ||
| ``` |
There was a problem hiding this comment.
What can go wrong in this example? Is it better to include an example with custom destructor like device?
There was a problem hiding this comment.
If you retain references to either key/value in another static singleton, that other destructor might access it after the memory is freed. This is of course overly simplistic and in isolation looks unrealistic, but just to illustrate the point. Easier to always be safe than to think of when / how things might break.
|
|
||
| // Destructor does NOT call T's destructor. | ||
| // This leaves the object "indestructible." | ||
| ~Indestructible() = default; |
There was a problem hiding this comment.
So the moment we start leveraging LeakSanitizer we're going to get flooded with reports?
Why can't we just enforce proper scopes and avoid globals/leaks/UB?
There was a problem hiding this comment.
AFAIK asan and leak sanitizers would be happy about it - as memory remains reachable at program exit, while we never run into use-after-free because all dtors are disabled. Having said that, I don't know for sure, but I would be very surprised if sanitizers don't understand this very common pattern.
Why can't we just enforce proper scopes and avoid globals/leaks/UB?
Sometimes it is way easier to use this pattern, basically wherever singleton pattern actually applies, but safer and more efficient.
|
|
||
| namespace tt::stl { | ||
|
|
||
| // `Indestructible` is a wrapper around `T` that behaves like `T` but does not call the destructor of `T`. |
There was a problem hiding this comment.
While we're deliberately not calling destructor forT, doesn't this break RAII gurantees if T holds resources like file handles or mutexes and ~T() was supposed to execute some cleanup subroutine?
There was a problem hiding this comment.
Thats the point:) Static vars shouldn't be responsible for this, as the stuff they are cleaning up (or other dependencies that are needed for this) might be gone already.
omilyutin-tt
force-pushed
the
omilyutin/static
branch
from
1391409 to
bb2dee1
Compare
Ticket
#17754, #17607
Problem description
Variables with static storage duration should have trivial destructors. Add guidance on why this so, and lower
Indestructibleutility to Metal, as the suggested alternative.What's changed
Indestructiblefrom tt-train to Metal.Indestructible.Checklist