[libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early

[ldionne]

Summary

This patch fixes http://llvm.org/PR28954 using the init_priority attribute. All supported compilers accept this attribute, including clang-cl.
I'm only putting this up for review because IDK how to write a test for it.

Can anybody suggest a way to test this?

[aaron.ballman]

I'm not certain of a good way to test it, but I have a question about the value you picked for init_priority. My understanding of the values starting from 101 is that 1-100 are reserved for implementation use. Is that understanding correct? If so, you may want to pick a value below 100 to ensure there's not an arms race with the user. I believe this may require some alteration to SemaDeclAttr.cpp to not diagnose when this is coming from a system header. Otherwise, what's to stop the user from having something marked constructor(101) that attempts to use a stream, but can't because they're not initialized yet?

[hfinkel]

In D31413#712013, @aaron.ballman wrote:

I'm not certain of a good way to test it, but I have a question about the value you picked for init_priority. My understanding of the values starting from 101 is that 1-100 are reserved for implementation use. Is that understanding correct? If so, you may want to pick a value below 100 to ensure there's not an arms race with the user. I believe this may require some alteration to SemaDeclAttr.cpp to not diagnose when this is coming from a system header. Otherwise, what's to stop the user from having something marked constructor(101) that attempts to use a stream, but can't because they're not initialized yet?

I agree; using 100 probably makes sense here. By spec, that's before any regular user code might execute.

Regarding testing, it is easy to test this (i.e. use cout/cerr/etc. from a constructor of a global object) if you link with libc++.a. Maybe just add a simple test and then we can see about setting up a buildbot which builds/tests a static libc++?

[sbc100]

This came up again in wasi-sdk: https://github.com/WebAssembly/wasi-sdk/issues/153

[ldionne]

I did some work on trying to reproduce this a while ago but never got to reproducing it on macOS.

[ldionne]

Added a test. I can't reproduce the issue, so I don't know whether the test is useful or not. Please help with that!

[sbc100]

I'd love to see this land so we can drop our downstream patch in emscripten and also fix the outstanding wasi-sdk issue.

[sbc100]

Might even be worth backporting such as simple but useful fix to the 11 release?

[eastig]

In D31413#2277070, @ldionne wrote:

Added a test. I can't reproduce the issue, so I don't know whether the test is useful or not. Please help with that!

There are tests in https://reviews.llvm.org/D12689 .

[ldionne]

In D31413#2277118, @eastig wrote:

In D31413#2277070, @ldionne wrote:

Added a test. I can't reproduce the issue, so I don't know whether the test is useful or not. Please help with that!

There are tests in https://reviews.llvm.org/D12689 .

Oh, nice, I missed that. I'll borrow those if you don't mind. Can you close https://reviews.llvm.org/D12689 and leave a trace to this review? I'm trying to de-tangle things here.

[ldionne]

Add tests.

libcxx/test/std/input.output/iostream.objects/init.pass.cpp

15 ↗

(On Diff #292261)

I'll fix that typo when committing.

[eastig]

In D31413#2277133, @ldionne wrote:

In D31413#2277118, @eastig wrote:

In D31413#2277070, @ldionne wrote:

Added a test. I can't reproduce the issue, so I don't know whether the test is useful or not. Please help with that!

There are tests in https://reviews.llvm.org/D12689 .

Oh, nice, I missed that. I'll borrow those if you don't mind. Can you close https://reviews.llvm.org/D12689 and leave a trace to this review? I'm trying to de-tangle things here.

I don't mind.
Just to check, in PR28954 it is mentioned the solution based on 'init_priority' works for Linux. What about other platforms?
The idea was to fall back to the conservative method if the attribute is not supported by a compiler. Maybe, as an option a warning can be issued.

[ldionne]

@hans Is there still time to cherry-pick this to the 11 release?

In D31413#2277198, @eastig wrote:

I don't mind.
Just to check, in PR28954 it is mentioned the solution based on 'init_priority' works for Linux. What about other platforms?
The idea was to fall back to the conservative method if the attribute is not supported by a compiler. Maybe, as an option a warning can be issued.

Eric Fiselier said the attribute worked on all compilers we support, including clang-cl. So as long as you compile the library with a supported compiler, my understanding is that it should work.

[smeenai]

What was the conclusion for the comments about the priority level (100 vs. 101)?

[ldionne]

In D31413#2277630, @smeenai wrote:

What was the conclusion for the comments about the priority level (100 vs. 101)?

My understanding is that values below 101 are literally not allowed:

<...>/llvm/libcxx/src/iostream.cpp:80:66: error: 'init_priority' attribute requires integer constant between 101 and 65535 inclusive
_LIBCPP_HIDDEN ios_base::Init __start_std_streams __attribute__((init_priority(100)));
                                                                 ^             ~~~
1 error generated.

If there's a way around that, and if values below 101 are reserved for the implementation, then I agree 100 is what we should use. @aaron.ballman where did you read that values below 101 were reserved for the implementation? The GCC docs at https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Attributes.html don't imply that -- they say the attribute starts at 101. I agree it's a fairly logical thing to think values before that would be reserved, but it doesn't say explicitly.

Is it possible that GCC reserves values before 101 for the implementation, but Clang implemented the attribute "naively" and just errors out?

[hans]

In D31413#2277085, @sbc100 wrote:

Might even be worth backporting such as simple but useful fix to the 11 release?

I think it's too late in the process for that. Might be a good candidate for 11.0.1 though.

[aaron.ballman]

In D31413#2277985, @ldionne wrote:

In D31413#2277630, @smeenai wrote:

What was the conclusion for the comments about the priority level (100 vs. 101)?

My understanding is that values below 101 are literally not allowed:

<...>/llvm/libcxx/src/iostream.cpp:80:66: error: 'init_priority' attribute requires integer constant between 101 and 65535 inclusive
_LIBCPP_HIDDEN ios_base::Init __start_std_streams __attribute__((init_priority(100)));
                                                                 ^             ~~~
1 error generated.

If there's a way around that, and if values below 101 are reserved for the implementation, then I agree 100 is what we should use. @aaron.ballman where did you read that values below 101 were reserved for the implementation?

From GCC itself: https://godbolt.org/z/zajPsj but also from libstdc++ maintainers https://gcc.gnu.org/legacy-ml/gcc-help/2014-08/msg00117.html

The GCC docs at https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Attributes.html don't imply that -- they say the attribute starts at 101. I agree it's a fairly logical thing to think values before that would be reserved, but it doesn't say explicitly.

Is it possible that GCC reserves values before 101 for the implementation, but Clang implemented the attribute "naively" and just errors out?

Yes, and the way I would handle this is to change the init_priority value checking to allow values <= 100 if the attribute location is within a system header. This would allow libc++ to use the reserved priority but would still give errors when attempted from user code.

[ldionne]

In D31413#2279182, @aaron.ballman wrote:

Yes, and the way I would handle this is to change the init_priority value checking to allow values <= 100 if the attribute location is within a system header. This would allow libc++ to use the reserved priority but would still give errors when attempted from user code.

Ok, that makes sense. If Clang implements that change, I can go back and change it in libc++. I unfortunately don't have bandwidth to go and do that, since this isn't affecting a platform I directly work on. So if anyone's willing to go and change Clang to make that work, ping me here and I'll fix libc++.

[aaron.ballman]

In D31413#2279404, @ldionne wrote:

In D31413#2279182, @aaron.ballman wrote:

Yes, and the way I would handle this is to change the init_priority value checking to allow values <= 100 if the attribute location is within a system header. This would allow libc++ to use the reserved priority but would still give errors when attempted from user code.

Ok, that makes sense. If Clang implements that change, I can go back and change it in libc++. I unfortunately don't have bandwidth to go and do that, since this isn't affecting a platform I directly work on. So if anyone's willing to go and change Clang to make that work, ping me here and I'll fix libc++.

I can make the change, but it may be a few weeks before I get to it. If someone gets to it sooner, I'd be happy to review the patch for them.

[aaron.ballman]

In D31413#2279498, @aaron.ballman wrote:

In D31413#2279404, @ldionne wrote:

In D31413#2279182, @aaron.ballman wrote:

Yes, and the way I would handle this is to change the init_priority value checking to allow values <= 100 if the attribute location is within a system header. This would allow libc++ to use the reserved priority but would still give errors when attempted from user code.

Ok, that makes sense. If Clang implements that change, I can go back and change it in libc++. I unfortunately don't have bandwidth to go and do that, since this isn't affecting a platform I directly work on. So if anyone's willing to go and change Clang to make that work, ping me here and I'll fix libc++.

I can make the change, but it may be a few weeks before I get to it. If someone gets to it sooner, I'd be happy to review the patch for them.

I've made the change and pushed it in af1d3e655991e5f0c86df372b8583a60d20268e0.

libcxx/src/iostream.cpp

 #endif
 ;
 _ALIGNAS_TYPE (wostream) _LIBCPP_FUNC_VIS char wclog[sizeof(wostream)]
 #if defined(_LIBCPP_ABI_MICROSOFT) && defined(__clang__)
 __asm__("?wclog@" _LIBCPP_ABI_NAMESPACE_STR "@std@@3V?$basic_ostream@_WU?$char_traits@_W@" _LIBCPP_ABI_NAMESPACE_STR "@std@@@12@A")
 #endif
 ;
 
-_LIBCPP_HIDDEN ios_base::Init __start_std_streams;
+_LIBCPP_HIDDEN ios_base::Init __start_std_streams __attribute__((init_priority(101)));
 
 // On Windows the TLS storage for locales needs to be initialized before we create
 // the standard streams, otherwise it may not be alive during program termination
 // when we flush the streams.
 static void force_locale_initialization() {
 #if defined(_LIBCPP_MSVCRT_LIKE)
   static bool once = []() {
     auto loc = newlocale(LC_ALL_MASK, "C", 0);

libcxx/test/std/input.output/iostream.objects/narrow.stream.objects/cout.init_priority.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// XFAIL: libcpp-has-no-stdout
+
+// Make sure that the iostreams are initialized before everything else.
+// This has been an issue when statically linking libc++ in some contexts.
+// See https://llvm.org/PR28954 for details.
+
+// RUN: %{build}
+// RUN: %{run} > %t.out
+// RUN: grep -e 'SHOULD BE PRINTED' %t.out
+
+#include <iostream>
+#include "test_macros.h"
+
+
+struct Foo {
+    Foo() {
+        std::cout << "SHOULD BE PRINTED" << std::endl;
+    }
+};
+
+Foo foo;
+
+int main(int, char**) {
+    return 0;
+}