[libcxx] Add a missing include for __bit_iterator #127015

atetubou · 2025-02-13T07:06:41Z

This is to fix compile error with explicit Clang modules like

../../third_party/libc++/src/include/__vector/vector_bool.h:85:11: error: default argument of '__bit_iterator' must be imported from module 'std.bit_reference_fwd' before it is required
   85 |   typedef __bit_iterator<vector, false> pointer;
      |           ^
../../third_party/libc++/src/include/__fwd/bit_reference.h:23:68: note: default argument declared here is not reachable
   23 | template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0>
      |                                                                    ^

This is to fix compile error with explicit Clang modules like ``` ../../third_party/libc++/src/include/__vector/vector_bool.h:85:11: error: default argument of '__bit_iterator' must be imported from module 'std.bit_reference_fwd' before it is required 85 | typedef __bit_iterator<vector, false> pointer; | ^ ../../third_party/libc++/src/include/__fwd/bit_reference.h:23:68: note: default argument declared here is not reachable 23 | template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0> | ^ ```

llvmbot · 2025-02-13T07:07:18Z

@llvm/pr-subscribers-libcxx

Author: Takuto Ikuta (atetubou)

Changes

This is to fix compile error with explicit Clang modules like

../../third_party/libc++/src/include/__vector/vector_bool.h:85:11: error: default argument of '__bit_iterator' must be imported from module 'std.bit_reference_fwd' before it is required
   85 |   typedef __bit_iterator&lt;vector, false&gt; pointer;
      |           ^
../../third_party/libc++/src/include/__fwd/bit_reference.h:23:68: note: default argument declared here is not reachable
   23 | template &lt;class _Cp, bool _IsConst, typename _Cp::__storage_type = 0&gt;
      |                                                                    ^

Full diff: https://github.com/llvm/llvm-project/pull/127015.diff

1 Files Affected:

(modified) libcxx/include/__vector/vector_bool.h (+1)

diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 35f0f745e201c..714c86ae2bb96 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -18,6 +18,7 @@
 #include <__bit_reference>
 #include <__config>
 #include <__functional/unary_function.h>
+#include <__fwd/bit_reference.h>
 #include <__fwd/functional.h>
 #include <__fwd/vector.h>
 #include <__iterator/distance.h>

var-const · 2025-02-13T07:54:08Z

@ldionne We really need to make sure this type of issues is caught by the CI.

var-const · 2025-02-13T07:55:08Z

@atetubou Thank you for the patches!

philnik777

This is an incorrect fix. vector_bool.h includes <__bit_reference>, which actually defines __bit_iterator.

var-const · 2025-02-13T08:13:11Z

@ian-twilightcoder Can you please take a look?

atetubou · 2025-02-13T08:15:45Z

@philnik777 Adding this include disappeared the error in our project. I'm still not sure when modules build can rely on indirect include and when not. Another way to remove the error is update modulemap config to export bit_reference_fwd from vector_bool.
Or is this bug of Clang module rather than missing include in libc++?

philnik777 · 2025-02-13T08:18:04Z

@philnik777 Adding this include disappeared the error in our project.

It's probably a fix. That doesn't mean it's the correct fix. Given that it should already be available, I don't think it's correct.

I'm still not sure when modules build can rely on indirect include and when not. Another way to remove the error is update modulemap config to export bit_reference_fwd from vector_bool. Or is this bug of Clang module rather than missing include in libc++?

We already do that, so this is probably something more complicated. Let's see what Ian thinks.

This is to fix compile error with explicit Clang modules like ``` ../../third_party/libc++/src/include/__vector/vector_bool.h:85:11: error: default argument of '__bit_iterator' must be imported from module 'std.bit_reference_fwd' before it is required 85 | typedef __bit_iterator<vector, false> pointer; | ^ ../../third_party/libc++/src/include/__fwd/bit_reference.h:23:68: note: default argument declared here is not reachable 23 | template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0> | ^ ```

ian-twilightcoder · 2025-02-13T19:05:08Z

I was looking at this a few days ago. I don't really understand the error, both the header includes and the module exports look like this should work as-is. However, explicit exports like libc++ does in its module map are not very well tested/kind of barely supported at all, so I'm not surprised that there are issues, especially with lsv. Mostly we've fixed these sorts of things by adding extra exports in the module map rather than extra includes in the headers I think. @ldionne would know better though.

This is to fix compile error with explicit Clang modules like ``` ../../third_party/libc++/src/include/__vector/vector_bool.h:85:11: error: default argument of '__bit_iterator' must be imported from module 'std.bit_reference_fwd' before it is required 85 | typedef __bit_iterator<vector, false> pointer; | ^ ../../third_party/libc++/src/include/__fwd/bit_reference.h:23:68: note: default argument declared here is not reachable 23 | template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0> | ^ ```

ian-twilightcoder · 2025-03-14T19:33:25Z

/cherry-pick 672e385

llvmbot · 2025-03-14T19:39:00Z

/pull-request #131382

ldionne · 2025-03-17T13:21:53Z

@atetubou Do you have a reproducer for this issue? Can you please share it?

ian-twilightcoder · 2025-03-17T22:21:46Z

@atetubou Do you have a reproducer for this issue? Can you please share it?

I got this one to reproduce on macOS after updating libc++ in my SDK to the llvm 20 one.

echo '#include <vector>' | xcrun --sdk macosx clang -isysroot "$(xcrun --sdk macosx --show-sdk-path)" -target arm64-apple-macosx$(xcrun --sdk macosx --show-sdk-version) -x c++ -std=c++17 -fmodules -fcxx-modules -Xclang -fmodules-local-submodule-visibility "-fmodules-cache-path=$(mktemp -d)" -fsyntax-only -

If I use C++20 or later it no longer reproduces, which is very strange.

atetubou · 2025-03-18T07:24:18Z

I tried to create small repro case even with -std=c++20, but it is not easy to make error case from chromium smaller as it is tightly coupled with build system and it complex configurations...

ldionne · 2025-03-18T13:05:54Z

Thanks a lot @ian-twilightcoder. So what's going on here is that we don't have a CI job that tests the combination of C++17 AND clang modules with LSV enabled. Our LSV modules build was using C++23, which is kinda pointless since LSV is implied by C++ >= 20 nowadays (I learned that yesterday).

If we wanted to have coverage for this, we could basically switch our clang-LSV build to use C++17 instead of removing it as I was planning to do in #131667. Edit: This is now #131815.

ldionne · 2025-03-18T14:58:57Z

I think I have a reduced reproducer. It looks like a Clang bug to me. This seems to depend on the order in which things are defined in the modulemap. I filed #131814 for this issue. In the meantime, I'll add a comment on the include that was added in this PR so it can be cleaned up when the Clang bug is fixed, or if we determine that another change in libc++ is required.

ldionne · 2025-03-18T15:11:28Z

Now that we have a better understanding of what's going on, I'd like to echo what @philnik777 said in #127015 (comment). We shouldn't fix things blindly by patching over them without understanding the root cause, especially when we do so without adding proper test coverage. I know those are just build issues so it may seem like a "simple hacky fix" is good enough, but it really isn't since this sort of technical debt compounds over years of development and gets us to a place where we don't understand our codebase anymore. Going forward, let's please try to go to the bottom of these kinds of issues, that's the only way we'll build something solid.

ian-twilightcoder · 2025-03-18T17:24:43Z

It looks like a Clang bug to me.

That's what I was trying to say in #127015 (comment). lsv is a bit buggy, and it seems to get worse without export *. It would be great if the lsv feature got some love...

ldionne · 2025-03-18T18:28:32Z

@atetubou Did you encounter this issue while building with modules and LSV enabled?

This is to fix compile error with explicit Clang modules like ``` ../../third_party/libc++/src/include/__vector/vector_bool.h:85:11: error: default argument of '__bit_iterator' must be imported from module 'std.bit_reference_fwd' before it is required 85 | typedef __bit_iterator<vector, false> pointer; | ^ ../../third_party/libc++/src/include/__fwd/bit_reference.h:23:68: note: default argument declared here is not reachable 23 | template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0> | ^ ``` (cherry picked from commit 672e385)

atetubou · 2025-03-21T06:46:41Z

@ldionne Yes, we use -fmodules-local-submodule-visibility in https://source.chromium.org/chromium/chromium/src/+/main:build/config/compiler/BUILD.gn;l=1705;drc=fdcf479f2f6f85ca3f8f8f3d43fde27fb345fa5a, and saw this error.

atetubou requested a review from a team as a code owner February 13, 2025 07:06

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 13, 2025

var-const approved these changes Feb 13, 2025

View reviewed changes

var-const merged commit 672e385 into llvm:main Feb 13, 2025
72 of 78 checks passed

atetubou deleted the patch-2 branch February 13, 2025 07:59

philnik777 reviewed Feb 13, 2025

View reviewed changes

ian-twilightcoder added this to the LLVM 20.X Release milestone Mar 14, 2025

github-project-automation bot added this to LLVM Release Status Mar 14, 2025

github-project-automation bot moved this to Needs Triage in LLVM Release Status Mar 14, 2025

llvmbot moved this from Needs Triage to Done in LLVM Release Status Mar 14, 2025

ldionne mentioned this pull request Mar 17, 2025

release/20.x: [libcxx] Add a missing include for __bit_iterator (#127015) #131382

Merged

ian-twilightcoder mentioned this pull request Mar 18, 2025

[libc++] Remove the explicit Clang/LSV CI job #131667

Closed

ldionne mentioned this pull request Mar 18, 2025

[modules] Compilation success depends on order of declarations in the modulemap #131814

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libcxx] Add a missing include for __bit_iterator #127015

[libcxx] Add a missing include for __bit_iterator #127015

atetubou commented Feb 13, 2025

llvmbot commented Feb 13, 2025

var-const commented Feb 13, 2025

var-const commented Feb 13, 2025

philnik777 left a comment •

edited

Loading

var-const commented Feb 13, 2025

atetubou commented Feb 13, 2025

philnik777 commented Feb 13, 2025

ian-twilightcoder commented Feb 13, 2025

ian-twilightcoder commented Mar 14, 2025

llvmbot commented Mar 14, 2025

ldionne commented Mar 17, 2025

ian-twilightcoder commented Mar 17, 2025 •

edited

Loading

atetubou commented Mar 18, 2025

ldionne commented Mar 18, 2025 •

edited

Loading

ldionne commented Mar 18, 2025

ldionne commented Mar 18, 2025

ian-twilightcoder commented Mar 18, 2025

ldionne commented Mar 18, 2025

atetubou commented Mar 21, 2025

[libcxx] Add a missing include for __bit_iterator #127015

[libcxx] Add a missing include for __bit_iterator #127015

Conversation

atetubou commented Feb 13, 2025

llvmbot commented Feb 13, 2025

var-const commented Feb 13, 2025

var-const commented Feb 13, 2025

philnik777 left a comment • edited Loading

Choose a reason for hiding this comment

var-const commented Feb 13, 2025

atetubou commented Feb 13, 2025

philnik777 commented Feb 13, 2025

ian-twilightcoder commented Feb 13, 2025

ian-twilightcoder commented Mar 14, 2025

llvmbot commented Mar 14, 2025

ldionne commented Mar 17, 2025

ian-twilightcoder commented Mar 17, 2025 • edited Loading

atetubou commented Mar 18, 2025

ldionne commented Mar 18, 2025 • edited Loading

ldionne commented Mar 18, 2025

ldionne commented Mar 18, 2025

ian-twilightcoder commented Mar 18, 2025

ldionne commented Mar 18, 2025

atetubou commented Mar 21, 2025

philnik777 left a comment •

edited

Loading

ian-twilightcoder commented Mar 17, 2025 •

edited

Loading

ldionne commented Mar 18, 2025 •

edited

Loading