This is an archive of the discontinued LLVM Phabricator instance.

Differential D142184

[libcxx] Add new line after assert message
ClosedPublic

Authored by vitalybuka on Jan 19 2023, 11:34 PM.

Details

Reviewers
ldionne
philnik
Mordante
Group Reviewers
Restricted Project
Commits
rGc4bd5977f429: [libcxx] Add new line after assert message
Summary

Abort signal handler may print aswell. Without new line it goes into the
same.

Diff Detail

Event Timeline

vitalybuka created this revision.Jan 19 2023, 11:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 11:34 PM
vitalybuka requested review of this revision.Jan 19 2023, 11:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 11:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
vitalybuka updated this revision to Diff 490732.Jan 19 2023, 11:36 PM

putc

vitalybuka added a reviewer: ldionne.Jan 19 2023, 11:37 PM
Harbormaster completed remote builds in B208912: Diff 490732.Jan 20 2023, 3:59 AM
Mordante added a subscriber: Mordante.Jan 20 2023, 8:42 AM
Mordante added inline comments.
libcxx/src/verbose_abort.cpp
39 ↗(On Diff #490732)

would it be possible to add require the newline in the format of __libcpp_verbose_abort? If not I'm happy with this approach.

vitalybuka added inline comments.Jan 20 2023, 5:42 PM
libcxx/src/verbose_abort.cpp
39 ↗(On Diff #490732)

would it be possible to add require the newline in the format of __libcpp_verbose_abort? If not I'm happy with this approach.

Here? https://github.com/llvm/llvm-project/blob/de6b75c4ab130a3c798f691923b4b78b773e53cc/libcxx/include/__assert#L44

\n is reasonable for stderr, but I am not sure if \n is appropriate for Apple and Android stuff below

ldionne added inline comments.Jan 25 2023, 7:52 AM
libcxx/src/verbose_abort.cpp
39 ↗(On Diff #490732)

I think it would be reasonable. I think I'd be happier with that approach too:

::std::__libcpp_verbose_abort("%s:%d: assertion %s failed: %s\n", __FILE__, __LINE__, #expression, message))
vitalybuka updated this revision to Diff 492606.Jan 26 2023, 5:11 PM

move \n

Harbormaster completed remote builds in B210252: Diff 492606.Jan 26 2023, 6:31 PM
vitalybuka added a comment.Feb 5 2023, 9:49 PM

ping

vitalybuka added a comment.Feb 14 2023, 1:28 PM
In D142184#4105656, @vitalybuka wrote:

ping

ping

vitalybuka updated this revision to Diff 497455.Feb 14 2023, 3:14 PM

rebase

Harbormaster completed remote builds in B213744: Diff 497455.Feb 14 2023, 5:33 PM
vitalybuka updated this revision to Diff 503490.Mar 8 2023, 1:11 PM

remove assert

Harbormaster completed remote builds in B218191: Diff 503490.Mar 8 2023, 4:32 PM
vitalybuka updated this revision to Diff 525379.May 24 2023, 5:21 PM

rebase

Harbormaster completed remote builds in B234357: Diff 525379.May 25 2023, 4:13 AM
vitalybuka added a comment.May 25 2023, 2:16 PM

ping

vitalybuka added a comment.Jun 22 2023, 2:43 PM

ping

philnik accepted this revision.Jun 22 2023, 2:48 PM
philnik added a subscriber: philnik.

Let's wait a few days (maybe until Tuesday or Wednesday) and see if @Mordante or @ldionne have any more comments. If not, feel free to land this. This has been open way too long for the amount of change.

This revision is now accepted and ready to land.Jun 22 2023, 2:48 PM
Mordante accepted this revision.Jun 24 2023, 6:55 AM
In D142184#4442724, @vitalybuka wrote:

ping

Sorry this slipped under the radar. LGTM!

Closed by commit rGc4bd5977f429: [libcxx] Add new line after assert message (authored by vitalybuka). · Explain WhyJun 27 2023, 1:32 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka added a commit: rGc4bd5977f429: [libcxx] Add new line after assert message.

Revision Contents

PathSize
libcxx/
include/
2 lines

Diff 535117

libcxx/include/__assert

Show All 31 Lines
# error "_LIBCPP_ENABLE_ASSERTIONS must be set to 0 or 1" # error "_LIBCPP_ENABLE_ASSERTIONS must be set to 0 or 1"
#endif #endif
#if _LIBCPP_ENABLE_ASSERTIONS #if _LIBCPP_ENABLE_ASSERTIONS
# define _LIBCPP_ASSERT(expression, message) \ # define _LIBCPP_ASSERT(expression, message) \
(__builtin_expect(static_cast<bool>(expression), 1) \ (__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \ ? (void)0 \
: _LIBCPP_VERBOSE_ABORT( \ : _LIBCPP_VERBOSE_ABORT( \
"%s:%d: assertion %s failed: %s", __builtin_FILE(), __builtin_LINE(), #expression, message)) "%s:%d: assertion %s failed: %s\n", __builtin_FILE(), __builtin_LINE(), #expression, message))
#elif !defined(_LIBCPP_ASSERTIONS_DISABLE_ASSUME) && __has_builtin(__builtin_assume) #elif !defined(_LIBCPP_ASSERTIONS_DISABLE_ASSUME) && __has_builtin(__builtin_assume)
# define _LIBCPP_ASSERT(expression, message) \ # define _LIBCPP_ASSERT(expression, message) \
(_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume") \ (_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume") \
__builtin_assume(static_cast<bool>(expression)) _LIBCPP_DIAGNOSTIC_POP) __builtin_assume(static_cast<bool>(expression)) _LIBCPP_DIAGNOSTIC_POP)
#else #else
# define _LIBCPP_ASSERT(expression, message) ((void)0) # define _LIBCPP_ASSERT(expression, message) ((void)0)
#endif #endif
#endif // _LIBCPP___ASSERT #endif // _LIBCPP___ASSERT