[libc++] Fixes thread::id's operator<<.

[Mordante]

Summary

The output of

template<class charT, class traits>
  basic_ostream<charT, traits>&
    operator<<(basic_ostream<charT, traits>& out, thread::id id);

is affected by the state of out. The wording states

[thread.thread.id]/2

The text representation for the character type charT of an object of
type thread::id is an unspecified sequence of charT such that, for two
objects of type thread::id x and y, if x == y is true, the thread::id
objects have the same text representation, and if x != y is true, the
thread::id objects have distinct text representations.

[thread.thread.id]/9

template<class charT, class traits>
  basic_ostream<charT, traits>&
    operator<< (basic_ostream<charT, traits>& out, thread::id id);

Effects: Inserts the text representation for charT of id into out.

This wording changed in C++23 due to adding a formatter specialization for
thread::id. However the requirement was the same in older versions of C++.

This issue is that thread::id is an integral or pointer and affected by the
formatting manipulators for them. Thus the text representation can differ if
x == y which violates the requirements.

The fix has to hard-code some formatting style for the text
representation. It uses the Standard specified default values

Table 124: basic_ios::init() effects [tab:basic.ios.cons] flags()

flags() skipws | dec

Fixes PR: https://llvm.org/PR62073

[Mordante]

CI fixes.

[Mordante]

Fixes C++03 compatibility of the test.

[Mordante]

CI fixes.

[Mordante]

libcxx/include/__thread/thread.h

128–185

@ldionne This fails with localization disabled. To old operator<< would also fail but was not "removed" when localization was disabled. I can put to old code in the #else branch, but I wonder whether that is useful.

Note that we normally not comment out operator<<. Maybe we can discuss whether we should do that. I expect it "works" in our tests since they are disabled and these operators are templated on the _CharT of the basic_ostream thus not instantiated automatically.

[ldionne]

libcxx/include/__thread/thread.h

27

Is there any reason why we need <ios> and not just <iosfwd>? It would be really nice to try to avoid including <ios> from here since that's kind of a heavy header (especially with backward compatibility transitive includes enabled).

128–185

I think you did the right thing in this patch. These operator<< should be #ifdef'd out, and if they are not then it's just an oversight that happens not to trigger any issue. We probably forward declare basic_ostream somewhere and it all "works" as long as you don't actually instantiate the operator<<.

147–148

I don't understand that sentence. Do you mean that fill, align and width can or can't affect the data in the stream?

150–152

163–179

Is it ever possible for a thread id to be a negative number? If not, am I right that this isn't required?

[JMazurkiewicz]

libcxx/include/__thread/thread.h

181

Should we should use C locale while printing? Values from std::numpunct (thousands_sep and grouping) may affect text representation when type of __id.__id_ is integral.

I've mentioned it in this issue too: https://github.com/llvm/llvm-project/issues/62073.

[Mordante]

libcxx/include/__thread/thread.h

27

Good point, I need locale too. However adding these two headers does not change the transitive includes.

147–148

They can since they affect text.

163–179

I don't know, depends on what the platform does.

181

Good point, I overlooked the locale part in the bug report. I already felt the original patch was a bit ugly saving and restoring locale seems even worse. Instead of that approach I now create a new stream in a known state with a known locale and output that stream. This might be a bit more expensive, but I don't expect people not to try to use logging unconditionally in expensive parts of their code.

[Mordante]

Addresses review comments.

[ldionne]

Thanks for fixing this! LGTM w/ comments applied and green CI. Also make sure to rebase cause I made some changes to __thread/.

libcxx/include/__thread/thread.h

132

I think this whole comment could actually be changed to:

// [thread.thread.id]/9
//   Effects: Inserts the text representation for charT of id into out.
//
// [thread.thread.id]/2
//   The text representation for the character type charT of an
//   object of type thread::id is an unspecified sequence of charT
//   such that, for two objects of type thread::id x and y, if
//   x == y is true, the thread::id objects have the same text
//   representation, and if x != y is true, the thread::id objects
//   have distinct text representations.
//
// Since various flags in the output stream can affect how the
// thread id is represented (e.g. numpunct or showbase), we
// use a temporary stream instead and just output the thread
// id representation as a string.

156

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/stream.pass.cpp

80–81

[Mordante]

Rebased and addresses review comments.

[Mordante]

In D153336#4490426, @ldionne wrote:

Thanks for fixing this! LGTM w/ comments applied and green CI. Also make sure to rebase cause I made some changes to __thread/.

The rebase when smooth not git issues, however is it intended that thread::id's operator<< is moved to the id.h header?

[Mordante]

CI fixes.

[Mordante]

libcxx/include/__thread/thread.h

27

@ldionne it seems there were additional includes needed after all.

libcxx/include/__thread/thread.h

 #include <__exception/terminate.h>
 #include <__functional/hash.h>
 #include <__functional/unary_function.h>
 #include <__memory/unique_ptr.h>
 #include <__mutex/mutex.h>
 #include <__system_error/system_error.h>
 #include <__threading_support>
 #include <__utility/forward.h>
+#include <ios>
 #include <tuple>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp> class __thread_specific_ptr;
 class _LIBCPP_EXPORTED_FROM_ABI __thread_struct;
 class _LIBCPP_HIDDEN __thread_struct_imp;
 class __assoc_sub_state;

 {
     _LIBCPP_INLINE_VISIBILITY
     size_t operator()(__thread_id __v) const _NOEXCEPT
     {
         return hash<__libcpp_thread_id>()(__v.__id_);
     }
 };
 
-template<class _CharT, class _Traits>
-_LIBCPP_INLINE_VISIBILITY
-basic_ostream<_CharT, _Traits>&
-operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id)
-{return __os << __id.__id_;}
+template <class _CharT, class _Traits>
+_LIBCPP_INLINE_VISIBILITY basic_ostream<_CharT, _Traits>&
+operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id) {
+    // [thread.thread.id]/9
+    //   Effects: Inserts the text representation for charT of id into out.
+    //
+    // [thread.thread.id]/2
+    //   The text representation for the character type charT of an
+    //   object of type thread::id is an unspecified sequence of charT
+    //   such that, for two objects of type thread::id x and y, if
+    //   x == y is true, the thread::id objects have the same text
+    //   representation, and if x != y is true, the thread::id objects
+    //   have distinct text representations.
+    //
+    // In libc++ the thread::id is either an integral or a pointer.
+    // The wording only acknowledges a text representation, which
+    // means the formatting flags for integrals and pointers may not
+    // affect the text representation. It does not mention that the data
+    // in the stream must be identical for the same thread. So fill
+    // align and width are unaffected can affect the data in the stream.
+    //
+    // Note this wording it the wording of C++23, which changed due to
+    // the formatter thread::id, however earlier versions of C++
+    // effectively had the same wording.
+
+    static_assert(is_integral<decltype(__id.__id_)>::value || is_pointer<decltype(__id.__id_)>::value,
+                  "Validate that for the new type the formatting flags are properly guarded, specifically the "
+                  "precision is not safe yet.");
+
+    // Per Table 124: basic_ios::init() effects [tab:basic.ios.cons] flags()
+    ios_base::fmtflags __default = ios_base::skipws | ios_base::dec;
+
+    ios_base::fmtflags __flags = __os.flags();
+
+    // The internal alignment may affect the value when the value is
+    // negative
+    // Table 114: Fill padding [tab:facet.num.put.fill]
+    //   adjustfield == internal and a sign occurs in the representation
+    //   pad after the sign
+    //
+    // For a sequence of char
+    // [ostream.formatted.reqmts]/3
+    //   If a formatted output function of a stream os determines
+    //   padding, it does so as follows.  Given a charT character
+    //   sequence seq where charT is the character type of the stream,
+    //   if the length of seq is less than os.width(), then enough
+    //   copies of os.fill() are added to this sequence as necessary to
+    //   pad to a width of os.width() characters.  If (os.flags() &
+    //   ios_base::adjustfield) == ios_base::left is true, the fill
+    //   characters are placed after the character sequence; otherwise,
+    //   they are placed before the character sequence
+
+    __os.flags(__default | ((__flags & ios_base::left) ? ios_base::left : ios_base::right));
+    __os << __id.__id_;
+
+    __os.flags(__flags);
+    return __os;
+}
 
 class _LIBCPP_EXPORTED_FROM_ABI thread
 {
     __libcpp_thread_t __t_;
 
     thread(const thread&);
     thread& operator=(const thread&);
 public:

libcxx/test/libcxx/transitive_includes/cxx03.csv

 thread chrono
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread cstring
 thread ctime
 thread functional
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/libcxx/transitive_includes/cxx11.csv

 thread chrono
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread cstring
 thread ctime
 thread functional
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/libcxx/transitive_includes/cxx14.csv

 thread chrono
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread cstring
 thread ctime
 thread functional
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/libcxx/transitive_includes/cxx17.csv

 thread chrono
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread cstring
 thread ctime
 thread functional
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/libcxx/transitive_includes/cxx20.csv

 thread cerrno
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread cstring
 thread ctime
 thread functional
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/libcxx/transitive_includes/cxx23.csv

 thread array
 thread cerrno
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread ctime
 thread initializer_list
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/libcxx/transitive_includes/cxx26.csv

 thread array
 thread cerrno
 thread compare
 thread cstddef
 thread cstdint
 thread cstdlib
 thread ctime
 thread initializer_list
+thread ios
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio
 thread stdexcept
 thread string
 thread string_view

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/stream.pass.cpp

 #include <format>
 #include <sstream>
 #include <cassert>
 
 #include "make_string.h"
 #include "test_macros.h"
 
 template <class CharT>
-static void test() {
+static void basic() {
   std::thread::id id0 = std::this_thread::get_id();
   std::basic_ostringstream<CharT> os;
   os << id0;
 
 #if TEST_STD_VER > 20
   // C++23 added a formatter specialization for thread::id.
   // This changed the requirement of ostream to have a
   // [thread.thread.id]/2
   //   The text representation for the character type charT of an object of
   //   type thread::id is an unspecified sequence of charT ...
   // This definition is used for both streaming and formatting.
   //
   // Test whether the output is identical.
   std::basic_string<CharT> s = std::format(MAKE_STRING_VIEW(CharT, "{}"), id0);
   assert(s == os.str());
 #endif
 }
 
+template <class CharT>
+static std::basic_string<CharT> format(std::ios_base::fmtflags flags) {
+  std::basic_stringstream<CharT> sstr;
+  sstr.flags(flags);
+  sstr << std::this_thread::get_id();
+  return sstr.str();
+}
+
+template <class CharT>
+static void stream_state() {
+  std::basic_stringstream<CharT> sstr;
+  sstr << std::this_thread::get_id();
+  std::basic_string<CharT> expected = sstr.str();
+
+  // Unaffected by fill, width, and align.
+
+  assert(expected == format<CharT>(std::ios_base::dec | std::ios_base::skipws)); // default flags
+
+  assert(expected == format<CharT>(std::ios_base::oct));
+  assert(expected == format<CharT>(std::ios_base::hex));
+
+  assert(expected == format<CharT>(std::ios_base::scientific));
+  assert(expected == format<CharT>(std::ios_base::fixed));
+
+  assert(expected == format<CharT>(std::ios_base::boolalpha));
+  assert(expected == format<CharT>(std::ios_base::showbase));
+  assert(expected == format<CharT>(std::ios_base::showpoint));
+  assert(expected == format<CharT>(std::ios_base::showpos));
+  assert(expected == format<CharT>(std::ios_base::skipws));  // added for completion
+  assert(expected == format<CharT>(std::ios_base::unitbuf)); // added for completion
+  assert(expected == format<CharT>(std::ios_base::uppercase));
+
+  // Test fill, width, and align.
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size() + 10); // Make sure fill and align affect the output.
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::right);
+  sstr << std::this_thread::get_id();
+  expected = sstr.str();
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('*'));
+  sstr.width(expected.size());
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::right);
+  sstr << std::this_thread::get_id();
+  assert(expected != sstr.str());
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size() - 1);
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::right);
+  sstr << std::this_thread::get_id();
+  assert(expected != sstr.str());
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size());
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::left);
+  sstr << std::this_thread::get_id();
+  assert(expected != sstr.str());
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size());
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::internal);
+  sstr << std::this_thread::get_id();
+  assert(expected == sstr.str()); // internal does *not* affect strings
+}
+
+template <class CharT>
+static void test() {
+  basic<CharT>();
+  stream_state<CharT>();
+}
+
 int main(int, char**) {
   test<char>();
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test<wchar_t>();
 #endif
 
   return 0;
 }