[libc++] cuchar redeclares ::mbstate_t when it's in its own clang module

[iana]

Summary

When cuchar is compiled independently on platforms that don't have <uchar.h>, it doesn't get a declaration of mbstate_t, and so makes an empty declaration for ::mbstate_t. That conflicts with the declarations in __mbstate_t.h and cwchar since both of those headers do get mbstate_t before making ::mbstate_t.

Change __mbstate_t.h to just get the underlying declaration for mbstate_t and not make a declaration for ::mbstate_t. Include __mbstate_t.h in uchar.h and wchar.h when their next headers aren't available so that at least mbstate_t gets defined.

Add __std_mbstate_t.h to declare ::mbstate_t for headers that need that when cuchar or cwchar aren't available (because either _LIBCPP_HAS_NO_WIDE_CHARACTERS or _LIBCPP_CXX03_LANG).

[philnik]

Why do we want to change this? The current version looks just fine.

[iana]

In D148542#4274570, @philnik wrote:

Why do we want to change this? The current version looks just fine.

The redeclaration becomes an error with clang modules when __mbstate.h and cuchar are in different modules.

[Mordante]

LGTM modulo one comment. I leave the final approval to @philnik.

libcxx/include/cuchar

53

Can you update this comment why this is needed, this is useful for future readers of the code.

[philnik]

In D148542#4274624, @iana wrote:

In D148542#4274570, @philnik wrote:

Why do we want to change this? The current version looks just fine.

The redeclaration becomes an error with clang modules when __mbstate.h and cuchar are in different modules.

Please add that information to the commit message.

LGTM with comments addressed.

libcxx/include/cuchar

52

I don't think this check makes sense. It seems like right now we might have this symbol in C++03 in some headers, but not in cuchar. If we only want this in C++11 we should guard it in __mbstate_t.h.

[iana]

In D148542#4274624, @iana wrote:

In D148542#4274570, @philnik wrote:

Why do we want to change this? The current version looks just fine.

The redeclaration becomes an error with clang modules when __mbstate_t.h and cuchar are in different modules.

I dug into it a little more. When <cuchar> is compiled all by itself on Apple platforms, mbstate_t isn't defined because Apple doesn't have a <uchar.h>, and <stddef.h> doesn't declare mbstate_t either. But when <__mbstate_t.h> (and <cwchar>) are compiled, they both get the OS declaration of mbstate_t and their using declaration gets a type. I think it works out with the current std mega module because declarations leak between the submodules, but when those three headers are in different top level modules, that's when things get mad at <cuchar>.

So it looks like it's fine to leave the using ::mbstate_t _LIBCPP_USING_IF_EXISTS; line in <cuchar>, but we need to include something to get mbstate_t defined. Do we want to unconditionally include __mbstate_t.h? That will have the side effect of often including <wchar.h>, and also declaring ::mbstate_t which maybe we don't want. We could leave that include conditional on _LIBCPP_CXX03_LANG? Or we could instead include <bits/types/mbstate_t.h> or <sys/_types/_mbstate_t.h> to get just the C declaration?

[iana]

Redo the fix to make the libc++ uchar.h try harder to find mbstate_t.

[iana]

The sanitizer and aix test failures don't look related to this change, I get the same ones in an unrelated review too.

libcxx/include/uchar.h

59 ↗

(On Diff #515086)

This should maybe be

# define __need_size_t
# include_next <stddef.h>

Or maybe that's a bit too stringent.

[ldionne]

libcxx/include/uchar.h

49 ↗

(On Diff #515086)

Shouldn't we include __mbstate_t.h instead? Then we can tweak what that header does as needed to avoid including wchar.h from it unconditionally, perhaps?

[iana]

libcxx/include/uchar.h

49 ↗

(On Diff #515086)

The problem with including __mbstate_t.h here is it adds ::mbstate_t which I don't think we want in the C header. I was originally doing that in cuchar, but then that was kind of weird because it would declare ::mbstate_t all the time when the rest of the file is gated on _LIBCPP_CXX03_LANG. People thought it was weird to conditionally include __mbstate_t.h in cuchar too. Also __mbstate_t.h starts off trying to include all of wchar.h which seems counter to the comment at its top?

[iana]

Edits with feedback from @ldionne

[ldionne]

So hard for such a small patch! Thanks for working through this with me!

libcxx/include/__mbstate_t.h

19–22 ↗

(On Diff #516959)

31–35 ↗

(On Diff #516959)

libcxx/include/wchar.h

126 ↗

(On Diff #516959)

[iana]

Review changes, fix failed test.

[iana]

__locale uses mbstate_t without including __mbstate_t.h

[iana]

std::mbstate_t is still needed even when uchar/wchar aren't available. Make a __std_mbstate_t.h header for that.

[iana]

Revert ignore_format.txt

[ldionne]

This LGTM to merge, the CI failures are unrelated, I just checked.

[sylvestre.ledru]

i wonder if this isn't caused this regression:
https://github.com/llvm/llvm-project/issues/62523

[sylvestre.ledru]

I confirmed that a local revert fixed the issue for me

libcxx/include/cuchar

 #ifndef _LIBCPP_UCHAR_H
 #   error <cuchar> tried including <uchar.h> but didn't find libc++'s <uchar.h> header. \
           This usually means that your header search paths are not configured properly. \
           The header search paths should contain the C++ Standard Library headers before \
           any C Standard Library, and you are probably using compiler flags that make that \
           not be the case.
 #endif
 
+#if !defined(_LIBCPP_CXX03_LANG)
+#  include <__mbstate_t.h>  // provides ::mbstate_t
+#endif
+
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_CXX03_LANG)
 
-using ::mbstate_t _LIBCPP_USING_IF_EXISTS;
 using ::size_t _LIBCPP_USING_IF_EXISTS;
 
 #  if !defined(_LIBCPP_HAS_NO_C8RTOMB_MBRTOC8)
 using ::mbrtoc8 _LIBCPP_USING_IF_EXISTS;
 using ::c8rtomb _LIBCPP_USING_IF_EXISTS;
 #  endif
 using ::mbrtoc16 _LIBCPP_USING_IF_EXISTS;
 using ::c16rtomb _LIBCPP_USING_IF_EXISTS;
 using ::mbrtoc32 _LIBCPP_USING_IF_EXISTS;
 using ::c32rtomb _LIBCPP_USING_IF_EXISTS;
 
 #endif // _LIBCPP_CXX03_LANG
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_CUCHAR