[libcxx] Export the formatter_integral for module build.

[hokein]

Chromium encounters another similar issue, https://crbug.com/400870610. Workaround #120108 by adding necessary export *.

In Chromium, the vulkan_to_string.hpp file (included by VkStringify.cpp) uses the std::format function. The relevant include path is format_functions.h -> __format/format_integer.h -> __format/formatter_integral.h -> __charconv/to_chars_integral.h

When building VkStringify.cpp with libc++ modules, some internal function templates fail to instantiate due to template unreachability (caused by the clang bug #120108). This leads to linker errors, as seen in the build failures:

[478/479] 1m17.00s F SOLINK ./libvk_swiftshader.so
FAILED: 69e60d00-0e9a-45f0-9725-ff80fb25c239 "./libvk_swiftshader.so" SOLINK ./libvk_swiftshader.so
...
ld.lld: error: undefined hidden symbol: char* std::__Cr::__itoa::__append10<unsigned int>(char*, unsigned int)
>>> referenced by VkStringify.cpp            
>>>               obj/third_party/swiftshader/src/Vulkan/_swiftshader_libvulkan/VkStringify.o:(std::__Cr::__itoa::__base_10_u32(char*, unsigned int))

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Haojian Wu (hokein)

Changes

Chromium encounters another similar issue, https://crbug.com/400870610

Workaround #120108.

---

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

1 Files Affected:

• (modified) libcxx/include/module.modulemap (+8-2)

diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 43072aa0fb0f1..1dc1dc845cafd 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -926,7 +926,10 @@ module std [system] {
     module to_chars                   { header "__charconv/to_chars.h" }
     module to_chars_base_10           { header "__charconv/to_chars_base_10.h" }
     module to_chars_floating_point    { header "__charconv/to_chars_floating_point.h" }
-    module to_chars_integral          { header "__charconv/to_chars_integral.h" }
+    module to_chars_integral {
+      header "__charconv/to_chars_integral.h"
+      export *  // TODO: Workaround for https://github.com/llvm/llvm-project/issues/120108
+    }
     module to_chars_result            { header "__charconv/to_chars_result.h" }
     module traits                     { header "__charconv/traits.h" }
 
@@ -1316,7 +1319,10 @@ module std [system] {
     module formatter_char                     { header "__format/formatter_char.h" }
     module formatter_floating_point           { header "__format/formatter_floating_point.h" }
     module formatter_integer                  { header "__format/formatter_integer.h" }
-    module formatter_integral                 { header "__format/formatter_integral.h" }
+    module formatter_integral {
+      header "__format/formatter_integral.h"
+      export * // TODO: Workaround for https://github.com/llvm/llvm-project/issues/120108
+    }
     module formatter_output                   { header "__format/formatter_output.h" }
     module formatter_pointer                  { header "__format/formatter_pointer.h" }
     module formatter_string                   { header "__format/formatter_string.h" }

[ldionne]

Can you please provide a publicly accessible explanation of the issue? https://crbug.com/400870610 is not accessible from here, and we should strive to make commit messages self-explanatory.

FWIW I don't see a problem with the patch assuming the explanation makes sense, but I'd like to understand it first!

[atetubou]

I changed visibility of the bug.

[hokein]

Can you please provide a publicly accessible explanation of the issue? https://crbug.com/400870610 is not accessible from here, and we should strive to make commit messages self-explanatory.

Thanks, I updated the PR description.

[ldionne]

Thanks for sharing additional information. Can you describe how you're building? You seem to be using flags that are different from the ones we use in the CI, because we do have a modules build and various modules related tests, none of which triggered this issue. I'd like to check in a regression test for this.

[hokein]

Can you describe how you're building?

I think this is the config we use to build the libcxx. @atetubou is probably more familiar with the details than I am. @atetubou, could you provide more details on this?

[atetubou]

Chromium uses -fno-implicit-modules and that config caused much more errors than implicit modules build. And this repository doesn't have CI coverage with explicit libcxx module build?

[ldionne]

@ian-twilightcoder @Bigcheese Can you make a statement about the level of support of -fno-implicit-modules? Should we be adding yet another CI configuration for this "flavor" of modules?

Note that I'm not trying to push back on this patch, but I'm trying to figure out what our support matrix must be and how to catch these issues before our users (e.g. Chrome) runs into them because we have never considered those use cases. If the answer is "yes, we should be supporting -fno-implicit-modules", then I think it would make sense to add a CI configuration that tests with that flag, witness it failing without this patch, and then fix it with this patch.

[ilya-biryukov]

We use -fno-impilicit-modules at Google extensively and are definitely going to continue investmenting to support it.
It is important for us to make sure we don't accidentally invoke multiple compile actions in a single compiler invocation as we really want the build system to be responsible for scheduling each compilation (e.g. that way we can send it to the remote build service that caches things, etc).

That includes building libc++, although we use a wrapper around public STL headers rather than the module maps provided by libc++ for technical reasons. (Those wrappers just export * everything, so this error went unnoticed, though).

That being said, why would -fno-implicit-modules matter in that case? I would expect it to only affect whether the libc++ module is built automatically or not. Any build with -fimplicit-modules could be rewritten with -fno-implicit-modules and a bunch more -fmodule-file= flags, right?
It is supposed to give more fine-grained control over what the compiler does. If it affects semantics causing visibility errors, it would be a bug (I don't think it's the case here, though).

[atetubou]

What is the status of this PR?

[ldionne]

There seems to be conflicts that prevent merging.

I'm not opposing this patch, but I'd like us to add coverage for -fno-implicit-modules in the CI, otherwise we have no way to validate that the changes being made here actually solve anything. I don't doubt that it's the case, but we can't see it upstream.

[hokein]

I have rebased this PR.

[hokein]

@ldionne can we land this patch first to unblock the issue currently affecting chromium? I agree that adding CI to cover this case is important, but setting that up might take some time.

[atetubou]

Not sure why, but original link failure somehow disappeared in chromium. So feel free to close this. Thanks.

[ilya-biryukov]

Not sure why, but original link failure somehow disappeared in chromium. So feel free to close this. Thanks.

There seems to be an underlying compiler bug that we haven't yet addressed that keeps evading us. (Getting a minimal reproducer is really hard with modules).
I'm definitely supportive of closing this, just wanted to call out that the bug might show up again after some code change, keep an eye for it..

[hokein]

Not sure why, but original link failure somehow disappeared in chromium. So feel free to close this. Thanks.

There seems to be an underlying compiler bug that we haven't yet addressed that keeps evading us. (Getting a minimal reproducer is really hard with modules). I'm definitely supportive of closing this, just wanted to call out that the bug might show up again after some code change, keep an eye for it..

+1, I believe this bug is still not fixed. We got lucky that unrelated changes have made it "sleep" again. I'm closing it for now.

We know the reasons for #120108, fixing it properly will take some work -- the light-weight fix doesn't work since it breaks much existing code.