This is an archive of the discontinued LLVM Phabricator instance.

Differential D143443

[libc++] Use __builtin_FILE and __builtin_LINE in _LIBCPP_ASSERT
ClosedPublic

Authored by ldionne on Feb 6 2023, 4:13 PM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Commits
rGc5c329b75287: [libc++] Use __builtin_FILE and __builtin_LINE in _LIBCPP_ASSERT
Summary

This avoids theoretical potential ODR violations since LINE and
FILE expand to different things depending on where they are.

Diff Detail

Event Timeline

ldionne created this revision.Feb 6 2023, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 4:13 PM
ldionne requested review of this revision.Feb 6 2023, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 4:13 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
ldionne updated this revision to Diff 495317.Feb 6 2023, 4:20 PM

Fix typo.

Harbormaster completed remote builds in B212238: Diff 495317.Feb 6 2023, 4:25 PM
ldionne updated this revision to Diff 495363.Feb 6 2023, 8:10 PM

Rebase onto main

Harbormaster completed remote builds in B212270: Diff 495363.Feb 6 2023, 11:05 PM
Mordante accepted this revision.Feb 7 2023, 12:48 AM
Mordante added a subscriber: Mordante.

Thanks for fixing this, LGTM!

libcxx/include/__assert
20

Can you look at this in a follow-up commit?

40–41

When you apply clang-format does it fix-up the location of \ in this file. (I wouldn't mind it in a follow-up or just prior to committing.)

This revision is now accepted and ready to land.Feb 7 2023, 12:48 AM
ldionne marked 2 inline comments as done.Feb 7 2023, 7:35 AM
ldionne added inline comments.
libcxx/include/__assert
20

Will do, thanks for the heads up.

40–41

I will fix prior to committing.

This revision was landed with ongoing or failed builds.Feb 7 2023, 7:36 AM
Closed by commit rGc5c329b75287: [libc++] Use __builtin_FILE and __builtin_LINE in _LIBCPP_ASSERT (authored by ldionne). · Explain Why
This revision was automatically updated to reflect the committed changes.
ldionne marked 2 inline comments as done.
ldionne added a commit: rGc5c329b75287: [libc++] Use __builtin_FILE and __builtin_LINE in _LIBCPP_ASSERT.

Revision Contents

PathSize
libcxx/
include/
3 lines

Diff 495535

libcxx/include/__assert

Show All 11 Lines
#include <__config> #include <__config>
#include <__verbose_abort> #include <__verbose_abort>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header # pragma GCC system_header
#endif #endif
// TODO: Remove in LLVM 17. // TODO: Remove in LLVM 17.
MordanteUnsubmitted
Done
ReplyInline Actions

Can you look at this in a follow-up commit?

Mordante: Can you look at this in a follow-up commit?
ldionneAuthorUnsubmitted
Done
ReplyInline Actions

Will do, thanks for the heads up.

ldionne: Will do, thanks for the heads up.
#if defined(_LIBCPP_DEBUG) #if defined(_LIBCPP_DEBUG)
# error "Defining _LIBCPP_DEBUG is not supported anymore. Please use _LIBCPP_ENABLE_DEBUG_MODE instead." # error "Defining _LIBCPP_DEBUG is not supported anymore. Please use _LIBCPP_ENABLE_DEBUG_MODE instead."
#endif #endif
// Automatically enable assertions when the debug mode is enabled. // Automatically enable assertions when the debug mode is enabled.
#if defined(_LIBCPP_ENABLE_DEBUG_MODE) #if defined(_LIBCPP_ENABLE_DEBUG_MODE)
# ifndef _LIBCPP_ENABLE_ASSERTIONS # ifndef _LIBCPP_ENABLE_ASSERTIONS
# define _LIBCPP_ENABLE_ASSERTIONS 1 # define _LIBCPP_ENABLE_ASSERTIONS 1
# endif # endif
#endif #endif
#ifndef _LIBCPP_ENABLE_ASSERTIONS #ifndef _LIBCPP_ENABLE_ASSERTIONS
# define _LIBCPP_ENABLE_ASSERTIONS _LIBCPP_ENABLE_ASSERTIONS_DEFAULT # define _LIBCPP_ENABLE_ASSERTIONS _LIBCPP_ENABLE_ASSERTIONS_DEFAULT
#endif #endif
#if _LIBCPP_ENABLE_ASSERTIONS != 0 && _LIBCPP_ENABLE_ASSERTIONS != 1 #if _LIBCPP_ENABLE_ASSERTIONS != 0 && _LIBCPP_ENABLE_ASSERTIONS != 1
# 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) \
MordanteUnsubmitted
Done
ReplyInline Actions

When you apply clang-format does it fix-up the location of \ in this file. (I wouldn't mind it in a follow-up or just prior to committing.)

Mordante: When you apply clang-format does it fix-up the location of `\` in this file. (I wouldn't mind…
ldionneAuthorUnsubmitted
Done
ReplyInline Actions

I will fix prior to committing.

ldionne: I will fix prior to committing.
(__builtin_expect(static_cast<bool>(expression), 1) \ (__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \ ? (void)0 \
: _LIBCPP_VERBOSE_ABORT("%s:%d: assertion %s failed: %s", __FILE__, __LINE__, #expression, message)) : _LIBCPP_VERBOSE_ABORT( \
"%s:%d: assertion %s failed: %s", __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