[libc++][format] Implements formatter thread::id.

[Mordante]

Summary

Since stacktrace header is WIP and it's not sure that will be done
before LLVM17 update the documentation. When the header is implemented
implementing the formatter is trivial, so that can be done quickly
afterwards.

Implements parts of:

• P2693R1 Formatting thread::id and stacktrace

[Mordante]

Retrigger CI.

[Mordante]

CI fixes.

[Mordante]

CI Fixes.

[Mordante]

CI fixes.

[Mordante]

CI fixes and try to fix an Apple system.

[Mordante]

Retry CI.

[Mordante]

CI Test.

[Mordante]

Implements pointer based formatting for Apple platforms.

[Mordante]

Rebased and get a full CI run again.

[Mordante]

Removes debug code.

[Mordante]

Attempts to fix the CI.

[Mordante]

CI fixes.

[Mordante]

Use a different approach to try to fix the CI.

[Mordante]

libcxx/include/thread

256

Note before I used a larger if constexpr. but that failed. In the not taken branches the reinterpret_cast caused compilation errors.

259

TODO remove.

[Mordante]

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/format.functions.format.pass.cpp

18

Review all test to match the availablity as changed in D134598.

[ldionne]

libcxx/include/thread

234

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/format.functions.tests.h

18

std::stringstream s; s << id;
assert(s.str() == std::format("{}", id);

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

50

This seems a bit clever to me. Can we pass the expected end position as a parameter to the function explicitly? So this would look like it == fmt.begin() + offset. If you have this pattern in other places, I'm OK to land the patch as-is but we could have a simple patch to change all of them.

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

27–28

While we're at it, can we also test the output stream version on an empty std::thread::id?

36–38

[Mordante]

Thanks for the review!

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/format.functions.tests.h

18

As discussed this change is already done in stream.pass.cpp.

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

27–28

Based on [thread.thread.id]/1

An object of type thread​::​id provides a unique identifier for each thread of execution and a single distinct value for all thread objects that do not represent a thread of execution ([thread.thread.class]).
Each thread of execution has an associated thread​::​id object that is not equal to the thread​::​id object of any other thread of execution and that is not equal to the thread​::​id object of any thread object that does not represent threads of execution.

That value is unspecified and may differ between implementations; I expect it will be different.

[Mordante]

Rebased, addresses review comment, and get a CI run.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[Mordante]

CI fixes.

[Mordante]

CI fixes

[Mordante]

CI fixes.

[Mordante]

CI fixes

[Mordante]

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

50

See D147885

[manojgupta]

Just a heads up, with this change, we are hitting issues in building gdb. Appreciate any ideas on what is wrong.

aarch64-cros-linux-gnu-clang++ -x c++    -I. -I. -I./config -DLOCALEDIR="\"/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber  -I./../gnulib/import -I../gnulib/import -I./.. -I..  -DTUI=1    -I./.. -pthread  -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-error=maybe-uninitialized -Wno-mismatched-tags -Wno-error=deprecated-register -Wsuggest-override -Wimplicit-fallthrough=3 -Wduplicated-cond -Wshadow=local -Wdeprecated-copy -Wdeprecated-copy-dtor -Wredundant-move -Wmissing-declarations -Wmissing-prototypes -Wstrict-null-sentinel -Wformat -Wformat-nonliteral  -Os -pipe -march=armv8-a+crc+crypto -ftree-vectorize -g -ffunction-sections -fdata-sections     -c -o minsyms.o -MT minsyms.o -MMD -MP -MF ./.deps/minsyms.Tpo minsyms.c
 In file included from minsyms.c:56:
 In file included from ./../gdbsupport/parallel-for.h:25:
 In file included from /usr/bin/../include/c++/v1/thread:100:
 In file included from /usr/bin/../include/c++/v1/__format/formatter_integral.h:32:
 In file included from /usr/bin/../include/c++/v1/locale:203:
 /usr/bin/../include/c++/v1/__locale:600:5: error: '__abi_tag__' attribute only applies to structs, variables, functions, and namespaces
     _LIBCPP_INLINE_VISIBILITY
     ^
 /usr/bin/../include/c++/v1/__config:668:37: note: expanded from macro '_LIBCPP_INLINE_VISIBILITY'
 #  define _LIBCPP_INLINE_VISIBILITY _LIBCPP_HIDE_FROM_ABI
                                     ^
 /usr/bin/../include/c++/v1/__config:647:26: note: expanded from macro '_LIBCPP_HIDE_FROM_ABI'
           __attribute__((__abi_tag__(_LIBCPP_TOSTRING(_LIBCPP_VERSIONED_IDENTIFIER))))
                          ^
 In file included from minsyms.c:56:
 In file included from ./../gdbsupport/parallel-for.h:25:
 In file included from /usr/bin/../include/c++/v1/thread:100:
 In file included from /usr/bin/../include/c++/v1/__format/formatter_integral.h:32:
 In file included from /usr/bin/../include/c++/v1/locale:203:
 /usr/bin/../include/c++/v1/__locale:601:37: error: expected ';' at end of declaration list
     char_type toupper(char_type __c) const
                                     ^
 /usr/bin/../include/c++/v1/__locale:607:48: error: too many arguments provided to function-like macro invocation
     const char_type* toupper(char_type* __low, const char_type* __high) const
                                                ^
 ./../include/safe-ctype.h:146:9: note: macro 'toupper' defined here
 #define toupper(c) do_not_use_toupper_with_safe_ctype
         ^
 In file included from minsyms.c:56:
 In file included from ./../gdbsupport/parallel-for.h:25:
 In file included from /usr/bin/../include/c++/v1/thread:100:
 In file included from /usr/bin/../include/c++/v1/__format/formatter_integral.h:32:
 In file included from /usr/bin/../include/c++/v1/locale:203:
 /usr/bin/../include/c++/v1/__locale:619:48: error: too many arguments provided to function-like macro invocation
     const char_type* tolower(char_type* __low, const char_type* __high) const
                                                ^
 ./../include/safe-ctype.h:148:9: note: macro 'tolower' defined here
 #define tolower(c) do_not_use_tolower_with_safe_ctype
         ^

Contents of __locale at line 600:

_LIBCPP_INLINE_VISIBILITY
char_type toupper(char_type __c) const
{
    return do_toupper(__c);
}

[manojgupta]

I have opened a bug at issuetracker.google.com/issues/277967395

[tahonermann]

Adding Corentin for awareness.

[manojgupta]

Hmm, looking deeper, gdb (actually binutils), is doing something weird.

https://github.com/bminor/binutils-gdb/blob/master/include/safe-ctype.h

/* Prevent the users of safe-ctype.h from accidently using the routines
   from ctype.h.  Initially, the approach was to produce an error when
   detecting that ctype.h has been included.  But this was causing
   trouble as ctype.h might get indirectly included as a result of
   including another system header (for instance gnulib's stdint.h).
   So we include ctype.h here and then immediately redefine its macros.  */

#include <ctype.h>
#undef isalpha
#define isalpha(c) do_not_use_isalpha_with_safe_ctype
#undef isalnum
#define isalnum(c) do_not_use_isalnum_with_safe_ctype
<snip>

So any use of isupper/islower etc will cause an error.
Why this patch is triggering it:
It adds an include of #include <__format/formatter_integral.h> which ends up including <locale> which has internal definitions of isupper/islower causing clang to complain.
Any suggestions on what would be the right fix here?

[tahonermann]

It adds an include of #include <__format/formatter_integral.h> which ends up including <locale> which has internal definitions of isupper/islower causing clang to complain.

Any suggestions on what would be the right fix here?

It sounds like safe-ctype.h should also add a #include of <locale> (when compiling as C++) to avoid interfering with the std::locale variants of those functions.

[manojgupta]

Thanks, sent a patch to gdb at https://sourceware.org/pipermail/gdb-patches/2023-April/198870.html