Page MenuHomePhabricator

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

Authored by ldionne on Mar 27 2017, 8:01 PM.

Details

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?

Diff Detail

Event Timeline

EricWF created this revision.Mar 27 2017, 8:01 PM

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 edited edge metadata.Apr 11 2017, 4:21 AM

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

ldionne commandeered this revision.Sep 16 2020, 9:11 AM
ldionne added a reviewer: EricWF.
ldionne added a subscriber: ldionne.

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

ldionne updated this revision to Diff 292254.Sep 16 2020, 9:30 AM

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!

Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 9:30 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
sbc100 accepted this revision.Sep 16 2020, 9:36 AM

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

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

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 .

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 updated this revision to Diff 292261.Sep 16 2020, 10:17 AM

Add tests.

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

I'll fix that typo when committing.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 16 2020, 10:19 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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 added a subscriber: hans.Sep 16 2020, 10:30 AM

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

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.

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

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 added a comment.Sep 17 2020, 4:36 AM

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.

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.

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

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.

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.