This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Mordante on Feb 18 2023, 10:00 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG88622aabf107: [libc++][format] Implements formatter thread::id.
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

Diff Detail

Event Timeline

Mordante created this revision.Feb 18 2023, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2023, 10:00 AM
Mordante requested review of this revision.Feb 18 2023, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2023, 10:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 498598.Feb 18 2023, 10:24 AM

Retrigger CI.

Mordante updated this revision to Diff 498671.Feb 19 2023, 5:51 AM

CI fixes and try to fix an Apple system.

Mordante updated this revision to Diff 498672.Feb 19 2023, 6:00 AM

Retry CI.

Mordante updated this revision to Diff 498676.Feb 19 2023, 7:23 AM

Implements pointer based formatting for Apple platforms.

Mordante updated this revision to Diff 498677.Feb 19 2023, 7:46 AM

Rebased and get a full CI run again.

Mordante updated this revision to Diff 498679.Feb 19 2023, 8:13 AM

Removes debug code.

Mordante updated this revision to Diff 498685.Feb 19 2023, 10:11 AM

Attempts to fix the CI.

Mordante updated this revision to Diff 498887.Feb 20 2023, 9:13 AM

Use a different approach to try to fix the CI.

Mordante added inline comments.Feb 21 2023, 8:25 AM
libcxx/include/thread
261

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

264

TODO remove.

Mordante added inline comments.Mar 22 2023, 9:25 AM
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 accepted this revision.Mar 28 2023, 9:46 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/thread
239
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
30–31

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

39–41
This revision is now accepted and ready to land.Mar 28 2023, 9:46 AM
Mordante marked 4 inline comments as done.Apr 7 2023, 9:38 AM

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
30–31

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.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 7 2023, 9:38 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Mordante updated this revision to Diff 511705.Apr 7 2023, 9:38 AM
Mordante marked 2 inline comments as done.

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 updated this revision to Diff 511709.Apr 7 2023, 9:47 AM

CI fixes.

Mordante updated this revision to Diff 511865.Apr 8 2023, 2:02 AM

CI fixes.

This revision was landed with ongoing or failed builds.Apr 8 2023, 7:39 AM
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.Apr 9 2023, 10:38 AM
Mordante added inline comments.
libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/parse.pass.cpp
50

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);
}

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

MyDeveloperDay removed a project: Restricted Project.

Adding Corentin for awareness.

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?

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.