This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use init_priority(100) when possible
ClosedPublic

Authored by ldionne on Feb 3 2021, 1:32 PM.

Details

Reviewers
aaron.ballman
ldionne
Group Reviewers
Restricted Project
Commits
rG7c49052b170f: [libc++] Use init_priority(100) when possible
Summary

Priorities below 101 are reserved for the implementation, so that's what
we should be using here. That is unfortunately only supported on more
recent versions of Clang. See https://reviews.llvm.org/D31413 for details.

Diff Detail

Event Timeline

ldionne created this revision.Feb 3 2021, 1:32 PM
ldionne requested review of this revision.Feb 3 2021, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 1:32 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 322110.Feb 8 2021, 7:52 AM

Try to fix the build by pretending we're in a system header.

ldionne added inline comments.
libcxx/src/experimental/memory_resource.cpp
80–81

@arthur.j.odwyer Pinging you here since you wrote that code. Does this LGTY?

Quuxplusone added inline comments.
libcxx/src/experimental/memory_resource.cpp
80–81

@ldionne: I just copied this line from experimental/... oh wait, this is experimental/... yeah, this was added by @EricWF in https://github.com/llvm/llvm-project/commit/4efaa30934e81 . IIUC, if we're sure this file is always compiled as C++14 at this point, then I don't even think it matters what we put here. In C++20 we should mark this variable constinit.

However, whatever you decide on, it'd be useful if you could add a review comment to D89057 .

ldionne updated this revision to Diff 377407.Oct 5 2021, 6:03 PM

Poke CI, this should be good to go.

ldionne updated this revision to Diff 377557.Oct 6 2021, 8:23 AM

Fix oversight in memory_resource.cpp

ldionne accepted this revision.Oct 6 2021, 12:53 PM
This revision is now accepted and ready to land.Oct 6 2021, 12:53 PM
This revision was automatically updated to reflect the committed changes.

CC @thakis

FYI, this change added an inconvenience in builds with clang-cl; the dummy preprocessor line directives (e.g. # 80 "iostream.cpp" 1 3) end up output as part of /showIncludes, which is what Ninja (and other tools) use for gathering info about the dependencies.

The command output contains this:

Note: including file:  C:/<builddir>/include/c++/v1\__undef_macros
[... more of the same ...]
Note: including file: iostream.cpp

The dependency on iostream.cpp is a dummy (the input file isn't found). This causes ninja builds to always rebuild iostream.cpp every time ninja is invoked, even when nothing has been touched. Running with ninja -d explain prints:

ninja explain: output iostream.cpp of phony edge with no inputs doesn't exist
ninja explain: iostream.cpp is dirty
ninja explain: src/CMakeFiles/cxx_shared.dir/iostream.cpp.obj is dirty

But I'm not sure if this is inconvenience enough to do something about it (I guess alternatives is to either use a windows specific mechanism directly, for setting a high enough priority for the constructors, or to just use the regular priority 101, so it doesn't need the dummy source markers).

CC @thakis

FYI, this change added an inconvenience in builds with clang-cl; the dummy preprocessor line directives (e.g. # 80 "iostream.cpp" 1 3) end up output as part of /showIncludes, which is what Ninja (and other tools) use for gathering info about the dependencies.

The command output contains this:

Note: including file:  C:/<builddir>/include/c++/v1\__undef_macros
[... more of the same ...]
Note: including file: iostream.cpp

The dependency on iostream.cpp is a dummy (the input file isn't found). This causes ninja builds to always rebuild iostream.cpp every time ninja is invoked, even when nothing has been touched. Running with ninja -d explain prints:

ninja explain: output iostream.cpp of phony edge with no inputs doesn't exist
ninja explain: iostream.cpp is dirty
ninja explain: src/CMakeFiles/cxx_shared.dir/iostream.cpp.obj is dirty

But I'm not sure if this is inconvenience enough to do something about it (I guess alternatives is to either use a windows specific mechanism directly, for setting a high enough priority for the constructors, or to just use the regular priority 101, so it doesn't need the dummy source markers).

FWIW, this workaround has always felt like a hack to me. Now that I think of it, perhaps we can push/pop the system header pragma around it instead?