This is an archive of the discontinued LLVM Phabricator instance.

Don't check that std::pair is trivially copyable on FreeBSD
ClosedPublic

Authored by dim on Apr 3 2021, 3:24 AM.

Details

Summary

As FreeBSD already used libc++ before it changed its ABI, we still use
the non-trivially copyable version of std::pair, which used to be
exposed via _LIBCPP_TRIVIAL_PAIR_COPY_CTOR, but more recently via
_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR.

Diff Detail

Event Timeline

dim created this revision.Apr 3 2021, 3:24 AM
dim requested review of this revision.Apr 3 2021, 3:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2021, 3:24 AM
serge-sans-paille accepted this revision.Apr 3 2021, 9:33 AM

Looks good, thanks for spotting this.

This revision is now accepted and ready to land.Apr 3 2021, 9:33 AM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Apr 6 2021, 11:48 AM
llvm/unittests/Support/TypeTraitsTest.cpp
113

I'm really not fond of using a libc++ internal macro here. I think we should instead detect that we're on FreeBSD.

dim added inline comments.Apr 6 2021, 12:09 PM
llvm/unittests/Support/TypeTraitsTest.cpp
113

At the moment in __config the part where this macro gets defined is indeed only when __FreeBSD__ is defined:

// Feature macros for disabling pre ABI v1 features. All of these options
// are deprecated.
#  if defined(__FreeBSD__)
#    define _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR
#  endif
#endif

So this could as well be #if !defined(__FreeBSD__), yes. At some point though we'll have to migrate to libc++ ABI v2 (whenever that comes), so then this check would have to be adjusted too...

ldionne added inline comments.Apr 6 2021, 12:19 PM
llvm/unittests/Support/TypeTraitsTest.cpp
113

Then it can be adjusted if you want to move to a different ABI based on some other detection. I'm very very uncomfortable with letting people use implementation details of libc++, that is the source of huge headache for us.

dim added inline comments.Apr 6 2021, 12:28 PM
llvm/unittests/Support/TypeTraitsTest.cpp
113

Even though this is all within the llvm project, I get your point. It becomes hard to rename that macro once this gets duplicated somehow. So let's change this test to #if !defined(__FreeBSD__) for now.

(That said, it's still an annoying wart in FreeBSD that we have the sort-of-pre-v1 ABI that has the non-trivial pair constructor. However we'd have to keep a compat lib in place if we ever want to change the ABI, so I'd rather wait until the libc++ v2 ABI is mostly finalized. Users of the old ABI can then install a compat libc++.so.1 binary.)

ldionne added inline comments.Apr 6 2021, 12:59 PM
llvm/unittests/Support/TypeTraitsTest.cpp
113

Thank you!