[PATCH] D81525: [Support] Initialize outs() errs() nulls() once

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D81525#2084092 <https://reviews.llvm.org/D81525#2084092>, @MaskRay wrote:

> In D81525#2083967 <https://reviews.llvm.org/D81525#2083967>, @dblaikie wrote:
>
> > Adding global ctors is generally avoided - as they add startup cost (which might be very important to interactive programs, etc) to code linking in the LLVM libraries.
>
>
> From a friend: "The output and error streams might be essential for logging in case of other initialization errors, so such cost is not worthless."


My understanding is that this cost is one of the reasons that raw_ostream exists in the first place, and it is *the* reason why `#include <iostream>` is banned in llvm.

Now, this solution does not introduce a global constructor in every compile unit like `<iostream>` does, but that does come with a cost -- it makes the code susceptible to static initialization order fiasco -- calling `outs()` before the constructor has executed will return a pointer to uninitialized memory.

So, I think it would be good to keep the current design of doing the initialization in function-local statics, at least until a wider discussion takes place. And I believe the current problem can be solved with function-local statics.

> (It is hard to imagine an application which does not call outs() or errs() if they use raw_ostream.cpp)
> 
>> As for the gotchas:
>> 
>>> if errs() is constructed before outs()
>> 
>> Why is that a problem? when errs() calls outs(), the static local ctor for outs() will be initialized as intended, right?
>> 
>>> and if errs() is changed to a buffered state
>> 
>> Not sure I understand this issue - could you describe it in more detail?
>> 
>>> when destructing errs() (outs() has been destructed), the tied-to pointer to outs() is dangling.
>> 
>> Yeah, that's potentially a problem - a wrapper that avoids running the dtor on these types (but does flush them, perhaps?) might be useful in that regard.
> 
> The three conditions are conjunctions:/ When all three are satisfied, "the tied-to pointer to outs() is dangling" will be the result.

So, if it's a conjuction, it should be enough to shoot down one of these points. The first one appears to be the easiest.

>>> if errs() is constructed before outs()

In the first version of D81156 <https://reviews.llvm.org/D81156>, `errs()` was implemented like so:

  static tied_raw_ostream<raw_fd_ostream> S(outs(), STDERR_FILENO, false, true);
  return S;

As `outs()` was called as a part of the initialization of the `errs()` object, this implementation guaranteed that `outs()` gets constructed first (and destructed last) and did not suffer from the above problem. Then the implementation was changed to:

  static raw_fd_ostream S(STDERR_FILENO, false, true);
  S.tie(&outs());
  return S;

Here the `outs()` call is moved outside of the initializer for the `errs()` object, and so it is possible that the `outs()` is constructed later than `errs()`. Apart from making it susceptible to the above problem, this also causes another issue (which we really should have caught during review) -- the call to `errs()` is no longer thread safe: as the `S.tie(&outs())` call is no longer guarded by the implicit `static` mutex, it gets executed every time that `errs()` is called. This introduces data races between concurrent calls to `errs()` -- the races are mostly bening as they are just re-setting the same piece of memory with the same value over and over again but they are still there. This also makes it impossible for someone to manually "untie" the errs stream like we discussed, as the next call to `errs()` will just tie it back again (and this untieing will result in more serious data races).

It sounds to me like both of these problems can be solved by moving the tieing back into the static initializer. If we want to keep the current `tie` interface, then the simplest way to fix that would be to have a helper static object which initializes things in the correct order:

  struct InitializeAndTie {
    raw_fd_ostream S; // This could also be a base class.
    InitializeAndTie(raw_ostream &TieTo) : S(STDERR_FILENO, false, true) { S.tie(&TieTo); }
  } Init(outs());
  return Init.S;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81525