How do we handle "immortal" pointers that are reachable for entire lifetime of an app?

[scottslacksmith]

The "Google C++ Style Guide" forbids variables of static storage duration and Titus Winters in
"CppCon 2017: Titus Winters “Hands-On With Abseil" ( https://www.youtube.com/watch?v=xu7q8dGvuwk ) gave some of the engineering reasons behind this rules which seems to be entirely reasonable.

In short , at the global namespace, Google's prefers this

#include <iostream>

const std::string& str() {
  static std::string* s = new std::string("Hello");
  return *s;
}

main() {
  std::cout << str();
}

over

#include <iostream>

const std::string str("Hello");

main() {
  std::cout << str;
}

as new std::string("Hello") is intended to be reachable for the entire lifetime of the app and thus it's not strictly necessary to run std::string's dtor and deallocate memory as the OS will eventually recover the memory when process finally terminates. In fact it helps if new std::string("Hello") is never deleted as we can avoid the complication of coordinating its destruction order with respect to other static variables/threads that may refer to it when a program exits main.

In this situation, I assume static gsl::owner<std::string*> s = new std::string("Hello"); is not appropriate as the code checking tools should warn that a delete s is missing?

Instead, does it makes sense to have a gsl::immortal to indicate that the object is intentionally never deleted? e.g.

const std::string& str() {
  static gsl::immortal<std::string*> s = new std::string("Hello");
  return *s;
}

In addition, we could complement gsl::immortal with gsl::make_immortal(...) as this give us :

1. Ability to allocate from a mem pool to reduce possible memory fragmentation.
2. Convenient opportunity to inform a mem leak detector that the mem alloc is intentionally not matched by a dealloc.

This would be especially helpful for the light-weight mem leak detector that's built into MSVC's debug CRT.
e.g.

namespace gsl {
  template<typename T, typename Args>
  gsl::immortal<T> make_immortal(Args&&... a) {
    T p;
    int flag = _CrtSetDbgFlag(_CRTDBG_REPORT_FLAG); 
    _CrtSetDbgFlag(flag & ~_CRTDBG_ALLOC_MEM_DF); 
    try {
      p = new T(std::forward<Args>(a)...);
    }
    catch(...) {
      _CrtSetDbgFlag(flag); 
      throw;
    }
    _CrtSetDbgFlag(flag); 
    return p;
  }
}

const std::string& str() {
  static auto s = gsl::make_immortal<std::string*>("Hello");
  return *s;
}

[AndrewPardoe]

Per our editor's discussion, Herb, can you please:

1. We should at least change I.2's title to "avoid non-const non-global variables."
2. We are on track to having constexpr string variables. Would this change our recommendations with regards to this discussion?

[hsutter]

0. Would someone please post a summary of the reasons (preferred) or else provide a link to Titus' slides with what slide# to look at (next best) or at least the the timestamp in the video where he talks about the reasons (minimum)? I don't remember the reasons and don't want to scour the talk to find/infer what part you mean.

1. Done. Our guidance has always been to discourage only non-const global variables -- const ones are okay, and in the code above I think superior to the indirect workaround style.

2. My comment about constexpr string (i.e., necessarily compile-time string) variables was that (as of our Albuquerque meeting this month) we are actively considering allowing that in the future of C++. While the semantics are not nailed down, that would likely eliminate (or at least change) the static destruction question, as we look forward to what style we would recommend in the future and make sure that what we recommend now is on a straight path to that.

Besides that issue, are there other problems why const global variables, which are simpler to write, shouldn't be recommended?

[scottslacksmith]

0. I can't find any slides for this talk at https://github.com/CppCon/CppCon2017. The timestamp in the video for Titus' reasons is at the 34th minute.

My motivating example is the following pull request google/googletest#1142 for gtest. In summary, MSVC's debug CRT built-in memory leak detector is reporting false positives due to pointers to objects that are intentionally reachable for the entire lifetime of the app. Some previous authors have ignored Google's forbidden variables of static storage duration guideline and have attempted to fix this issue by ensuring all memory is deallocated before app terminates. I believe this fix is theoretically possible given sufficient engineering effort. However the engineering effort maybe too costly as it's very difficult to verify the fix given all the different build/platform configurations used for gtest in the wild, e.g. the .obj files maybe complied in different order which could change the destruction order of static variables. Instead, as Google recommends, a simpler engineering solution is to allow pointers to "immortal" objects that are never destructed as this can avoid a lot of engineering complications, especially when it's difficult coordinate the destruction order of static variables with destruction order of various threads.

[scottslacksmith]

0. My summary would be:

(a) In the correct context, a pointer to an object that’s intentionally reachable for the entire lifetime of the app, let’s call it an “immortal pointer”, should be a supported coding idiom as it leads to simpler code. Especially if the alternative is to coordinate the destruction order of static variables with destruction of various threads that access the before mentioned static variables (I believe this is the essence of Titus’ reasoning).

(b) If we accept that immortal pointers should be a supported coding idiom then it would be helpful to use a special coding convention to identify its usage in code. e.g.

const std::string& str() {
  static gsl::immortal<const std::string*> s = gsl::make_immortal<const std::string*>("Hello");
  return *s;
}

for the following reasons:

• To inform coding checking tools that delete is intentionally missing.
• If required, gsl::make_immortal provides a convenient opportunity to allocate from a memory pool to reduce possible memory fragmentation.
• If required, gsl::make_immortal provides a convenient opportunity to inform a memory leak detector that the memory allocation has no matching deallocation.

[jwakely]

In this situation, I assume static gsl::owner<std::string*> s = new std::string("Hello"); is not appropriate as the code checking tools should warn that a delete s is missing?

IMHO that is a perfectly good way to say "this pointer has an owner that lives for the duration of the program". Maybe add a const to it, to make it clear it's not given a new value at any time.

I appreciate it doesn't help solve the debug CRT giving false positives.

[scottslacksmith]

IMHO that is a perfectly good way to say "this pointer has an owner that lives for the duration of the program". Maybe add a const to it, to make it clear it's not given a new value at any time.

I agree and, by convention, the following

static const gsl::owner<const std::string *> s = new std::string("Hello"); 

could implicitly mean "this pointer has an owner that lives for the duration of the program".

The debug CRT giving false positives can be solved by

static const gsl::owner<const std::string *> s = gsl::make_immortal<const std::string*>("Hello");

So strictly speaking the type gsl::immortal isn't needed in this situation.

However in the gtest, which is my original motivation, the pointer I need to mark as "immortal" is a non-const member variable and not a static which, at first glance, does seem odd.

[hsutter]

From this discussion, it seems to me that if you really want to say "dynamically allocated object that is never deleted, not even during static destruction," then

static const gsl::owner<const std::string *> s = new std::string("Hello"); 

is a reasonable answer today. I would expect static checkers not to warn on the missing delete, because checkers operate locally (per-function) and verifying that there is no delete in any function that could access s is not a local analysis.

Have you tried that solution?

[scottslacksmith]

Have you tried that solution?

Yes

const std::string& str() {
  static const gls::owner<const std::string*> s = new std::string("Hello");
  return *s;
}

is a good solution for this particular pattern.
However Titus' original example maybe oversimplified.

The other use case from gtest is

class Mutex {
  ...
  void LazyInit();
  ...
  CRITICAL_SECTION* critical_section_;
};

void Mutex::LazyInit() {
  ...
  critical_section_ = new CRITICAL_SECTION; // THIS OBJECT IS INTENTIONALLY NEVER DELETED
  ...
}

is the following the right answer?

class Mutex {
  ...
  void LazyInit();
  ...
  gsl::owner<CRITICAL_SECTION*> critical_section_;
};

I was expecting that static checkers should or, sometime in the future, warn that critical_section_ is missing a delete.

[AstralStorm]

The main problem with anything remotely immortal is that it breaks dynamic linking and reuse in libraries. At least static globals or locals do not.
(Explicitly handled in platforms which support such things.)

That is in addition to producing spurious warnings in memory leak analysis tools.

The static global initialization order rule is important to check but then again is mostly about tracing dependencies between these globals. The actual order rarely matters.

[scottslacksmith]

The main problem with anything remotely immortal is that it breaks dynamic linking and reuse in libraries. At least static globals or locals do not.
(Explicitly handled in platforms which support such things.)

That's true and Google also notes that their Abeil library suffers from the same limitation: From https://abseil.io/about/compatibility.

• "Do not rely on dynamic unloading. We consistently use patterns that may not be friendly to dynamic unloading of shared libraries. We make no claims that any of this code is usable in a system with regular unloading of such libraries."

I believe Google's reasoning is that, if you can live with this limitation, then immortal pointers can greatly simplify code as don't need to track dependencies to ensure correct destruction order. Especially for multi-threaded apps.