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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a subscriber: hans.
ldionne added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Astigeevich via Phabricator via cfe-commits]

eastig added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG39faf428164a: [libc++] Ensure streams are initialized early 
(authored by ldionne).

Changed prior to commit:
  https://reviews.llvm.org/D31413?vs=292261=292262#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

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

Index: libcxx/test/std/input.output/iostream.objects/init.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/input.output/iostream.objects/init.pass.cpp
@@ -0,0 +1,88 @@
+//===--===//
+//
+// 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
+//
+//===--===//
+
+// UNSUPPORTED: libcpp-has-no-stdin, 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.
+//
+// This test works by checking that std::{cin,cout,cerr} is the same in a
+// static object constructor and in the main function. It dumps the memory of
+// each stream in the static object constructor and compares it with the memory
+// in the main function.
+//
+// The assumption is that if there are no uses of the stream object (such as
+// construction), then its memory must be the same. In the case where the test
+// "fails" and we are actually accessing an uninitialized object when we perform
+// the memcpy, the behavior is technically undefined (so the test could still
+// pass).
+
+#include 
+#include 
+#include 
+
+struct Checker {
+char *cerr_mem_dump;
+char *cin_mem_dump;
+char *cout_mem_dump;
+char *clog_mem_dump;
+
+char *wcerr_mem_dump;
+char *wcin_mem_dump;
+char *wcout_mem_dump;
+char *wclog_mem_dump;
+
+Checker()
+: cerr_mem_dump(new char[sizeof(std::cerr)])
+, cin_mem_dump(new char[sizeof(std::cin)])
+, cout_mem_dump(new char[sizeof(std::cout)])
+, clog_mem_dump(new char[sizeof(std::clog)])
+
+, wcerr_mem_dump(new char[sizeof(std::wcerr)])
+, wcin_mem_dump(new char[sizeof(std::wcin)])
+, wcout_mem_dump(new char[sizeof(std::wcout)])
+, wclog_mem_dump(new char[sizeof(std::wclog)])
+ {
+std::memcpy(cerr_mem_dump, (char*)::cerr, sizeof(std::cerr));
+std::memcpy(cin_mem_dump, (char*)::cin, sizeof(std::cin));
+std::memcpy(cout_mem_dump, (char*)::cout, sizeof(std::cout));
+std::memcpy(clog_mem_dump, (char*)::clog, sizeof(std::clog));
+
+std::memcpy(wcerr_mem_dump, (char*)::wcerr, sizeof(std::wcerr));
+std::memcpy(wcin_mem_dump, (char*)::wcin, sizeof(std::wcin));
+std::memcpy(wcout_mem_dump, (char*)::wcout, sizeof(std::wcout));
+std::memcpy(wclog_mem_dump, (char*)::wclog, sizeof(std::wclog));
+}
+
+~Checker() {
+delete[] cerr_mem_dump;
+delete[] cin_mem_dump;
+delete[] cout_mem_dump;
+delete[] clog_mem_dump;
+
+delete[] wcerr_mem_dump;
+delete[] wcin_mem_dump;
+delete[] wcout_mem_dump;
+delete[] wclog_mem_dump;
+}
+};
+
+static Checker check;
+
+int main() {
+assert(std::memcmp(check.cerr_mem_dump, (char const*)::cerr, sizeof(std::cerr)) == 0);
+assert(std::memcmp(check.cin_mem_dump, (char const*)::cin, sizeof(std::cin)) == 0);
+assert(std::memcmp(check.cout_mem_dump, (char const*)::cout, sizeof(std::cout)) == 0);
+assert(std::memcmp(check.clog_mem_dump, (char const*)::clog, sizeof(std::clog)) == 0);
+
+assert(std::memcmp(check.wcerr_mem_dump, (char const*)::wcerr, sizeof(std::wcerr)) == 0);
+assert(std::memcmp(check.wcin_mem_dump, (char const*)::wcin, sizeof(std::wcin)) == 0);
+assert(std::memcmp(check.wcout_mem_dump, (char const*)::wcout, sizeof(std::wcout)) == 0);
+assert(std::memcmp(check.wclog_mem_dump, (char const*)::wclog, sizeof(std::wclog)) == 0);
+}
Index: libcxx/src/iostream.cpp
===
--- libcxx/src/iostream.cpp
+++ libcxx/src/iostream.cpp
@@ -77,7 +77,7 @@
 #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

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/test/std/input.output/iostream.objects/init.pass.cpp:15
+//
+// This test works by checking that 'std::{cin/cout,cerr}' is the same in a
+// static object constructor and in the main function. It dumps the memory of

I'll fix that typo when committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 292261.
ldionne added a comment.

Add tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

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

Index: libcxx/test/std/input.output/iostream.objects/init.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/input.output/iostream.objects/init.pass.cpp
@@ -0,0 +1,88 @@
+//===--===//
+//
+// 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
+//
+//===--===//
+
+// UNSUPPORTED: libcpp-has-no-stdin, 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.
+//
+// This test works by checking that 'std::{cin/cout,cerr}' is the same in a
+// static object constructor and in the main function. It dumps the memory of
+// each stream in the static object constructor and compares it with the memory
+// in the main function.
+//
+// The assumption is that if there are no uses of the stream object (such as
+// construction), then its memory must be the same. In the case where the test
+// "fails" and we are actually accessing an uninitialized object when we perform
+// the memcpy, the behavior is technically undefined (so the test could still
+// pass).
+
+#include 
+#include 
+#include 
+
+struct Checker {
+char *cerr_mem_dump;
+char *cin_mem_dump;
+char *cout_mem_dump;
+char *clog_mem_dump;
+
+char *wcerr_mem_dump;
+char *wcin_mem_dump;
+char *wcout_mem_dump;
+char *wclog_mem_dump;
+
+Checker()
+: cerr_mem_dump(new char[sizeof(std::cerr)])
+, cin_mem_dump(new char[sizeof(std::cin)])
+, cout_mem_dump(new char[sizeof(std::cout)])
+, clog_mem_dump(new char[sizeof(std::clog)])
+
+, wcerr_mem_dump(new char[sizeof(std::wcerr)])
+, wcin_mem_dump(new char[sizeof(std::wcin)])
+, wcout_mem_dump(new char[sizeof(std::wcout)])
+, wclog_mem_dump(new char[sizeof(std::wclog)])
+ {
+std::memcpy(cerr_mem_dump, (char*)::cerr, sizeof(std::cerr));
+std::memcpy(cin_mem_dump, (char*)::cin, sizeof(std::cin));
+std::memcpy(cout_mem_dump, (char*)::cout, sizeof(std::cout));
+std::memcpy(clog_mem_dump, (char*)::clog, sizeof(std::clog));
+
+std::memcpy(wcerr_mem_dump, (char*)::wcerr, sizeof(std::wcerr));
+std::memcpy(wcin_mem_dump, (char*)::wcin, sizeof(std::wcin));
+std::memcpy(wcout_mem_dump, (char*)::wcout, sizeof(std::wcout));
+std::memcpy(wclog_mem_dump, (char*)::wclog, sizeof(std::wclog));
+}
+
+~Checker() {
+delete[] cerr_mem_dump;
+delete[] cin_mem_dump;
+delete[] cout_mem_dump;
+delete[] clog_mem_dump;
+
+delete[] wcerr_mem_dump;
+delete[] wcin_mem_dump;
+delete[] wcout_mem_dump;
+delete[] wclog_mem_dump;
+}
+};
+
+static Checker check;
+
+int main() {
+assert(std::memcmp(check.cerr_mem_dump, (char const*)::cerr, sizeof(std::cerr)) == 0);
+assert(std::memcmp(check.cin_mem_dump, (char const*)::cin, sizeof(std::cin)) == 0);
+assert(std::memcmp(check.cout_mem_dump, (char const*)::cout, sizeof(std::cout)) == 0);
+assert(std::memcmp(check.clog_mem_dump, (char const*)::clog, sizeof(std::clog)) == 0);
+
+assert(std::memcmp(check.wcerr_mem_dump, (char const*)::wcerr, sizeof(std::wcerr)) == 0);
+assert(std::memcmp(check.wcin_mem_dump, (char const*)::wcin, sizeof(std::wcin)) == 0);
+assert(std::memcmp(check.wcout_mem_dump, (char const*)::wcout, sizeof(std::wcout)) == 0);
+assert(std::memcmp(check.wclog_mem_dump, (char const*)::wclog, sizeof(std::wclog)) == 0);
+}
Index: libcxx/src/iostream.cpp
===
--- libcxx/src/iostream.cpp
+++ libcxx/src/iostream.cpp
@@ -77,7 +77,7 @@
 #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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Astigeevich via Phabricator via cfe-commits]

eastig added a comment.

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 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam Clegg via Phabricator via cfe-commits]

sbc100 accepted this revision.
sbc100 added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 292254.
ldionne added a comment.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31413

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


Index: 
libcxx/test/std/input.output/iostream.objects/narrow.stream.objects/cout.init_priority.sh.cpp
===
--- /dev/null
+++ 
libcxx/test/std/input.output/iostream.objects/narrow.stream.objects/cout.init_priority.sh.cpp
@@ -0,0 +1,33 @@
+//===--===//
+//
+// 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 
+#include "test_macros.h"
+
+
+struct Foo {
+Foo() {
+std::cout << "SHOULD BE PRINTED" << std::endl;
+}
+};
+
+Foo foo;
+
+int main(int, char**) {
+return 0;
+}
Index: libcxx/src/iostream.cpp
===
--- libcxx/src/iostream.cpp
+++ libcxx/src/iostream.cpp
@@ -77,7 +77,7 @@
 #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


Index: libcxx/test/std/input.output/iostream.objects/narrow.stream.objects/cout.init_priority.sh.cpp
===
--- /dev/null
+++ libcxx/test/std/input.output/iostream.objects/narrow.stream.objects/cout.init_priority.sh.cpp
@@ -0,0 +1,33 @@
+//===--===//
+//
+// 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 
+#include "test_macros.h"
+
+
+struct Foo {
+Foo() {
+std::cout << "SHOULD BE PRINTED" << std::endl;
+}
+};
+
+Foo foo;
+
+int main(int, char**) {
+return 0;
+}
Index: libcxx/src/iostream.cpp
===
--- libcxx/src/iostream.cpp
+++ libcxx/src/iostream.cpp
@@ -77,7 +77,7 @@
 #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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne commandeered this revision.
ldionne added a reviewer: EricWF.
ldionne added a comment.
Herald added subscribers: dexonsmith, jkorous.

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


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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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


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

https://reviews.llvm.org/D31413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/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++?


https://reviews.llvm.org/D31413



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


https://reviews.llvm.org/D31413



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF created this revision.

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?


https://reviews.llvm.org/D31413

Files:
  src/iostream.cpp


Index: src/iostream.cpp
===
--- src/iostream.cpp
+++ src/iostream.cpp
@@ -73,7 +73,7 @@
 #endif
 ;
 
-ios_base::Init __start_std_streams;
+ios_base::Init __start_std_streams __attribute__((init_priority(101)));
 
 ios_base::Init::Init()
 {


Index: src/iostream.cpp
===
--- src/iostream.cpp
+++ src/iostream.cpp
@@ -73,7 +73,7 @@
 #endif
 ;
 
-ios_base::Init __start_std_streams;
+ios_base::Init __start_std_streams __attribute__((init_priority(101)));
 
 ios_base::Init::Init()
 {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits