Page MenuHomePhabricator

Define new/delete in libc++ when using libcxxrt
ClosedPublic

Authored by dim on Feb 15 2021, 9:25 AM.

Details

Summary

Always turn on LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS, if libcxxrt is used
as the C++ ABI library, since libcxxrt does not provide the full set
ofnew and delete operators. In particular, the aligned versions of these
operators are completely missing. This primarily addresses builds on
FreeBSD, as this platform uses libcxxrt by default.

Also, attempt to provide a FreeBSD.cmake cache file, with hopefully sane
settings, partially copied from the Apple.cmake cache file. This needs
more work, probably some additions to ci build scripts (although I am
not aware of any 'official' FreeBSD build bots).

Diff Detail

Event Timeline

dim created this revision.Feb 15 2021, 9:25 AM
dim requested review of this revision.Feb 15 2021, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 9:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
dim retitled this revision from Summary: Always turn on LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS, if libcxxrt is used as the C++ ABI library, since libcxxrt does not provide the full set of new and delete operators. In particular, the aligned versions of these operators are... to Define new/delete in libc++ when using libcxxrt.Feb 15 2021, 9:27 AM
dim edited the summary of this revision. (Show Details)

Note, this is mainly aimed at getting the 12.0.0 release built, as I had to apply a custom hack to turn the operators on. But I would like to get rid of the hack and have this applied in a way acceptable to the libc++ maintainers.

ldionne accepted this revision.Feb 15 2021, 11:31 AM

Is the 12.x release fixed, or does this patch need to be cherry-picked to the branch?

Would you be willing to add a CI job using FreeBSD?

This revision is now accepted and ready to land.Feb 15 2021, 11:31 AM

Is the 12.x release fixed, or does this patch need to be cherry-picked to the branch?

Would you be willing to add a CI job using FreeBSD?

That would be great. However, there are currently some failing tests which should probably be XFAILed with a tracking bug report:
Here's the list of currently failing tests on FreeBSD 12.2:

Failed Tests (25):
  libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
  libc++ :: std/localization/locale.categories/category.collate/locale.collate.byname/compare.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_ru_RU.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_zh_CN.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.money.put/locale.money.put.members/put_long_double_ru_RU.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.money.put/locale.money.put.members/put_long_double_zh_CN.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.moneypunct.byname/grouping.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.moneypunct.byname/neg_format.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.moneypunct.byname/pos_format.pass.cpp
  libc++ :: std/localization/locale.categories/category.monetary/locale.moneypunct.byname/thousands_sep.pass.cpp
  libc++ :: std/localization/locale.categories/category.time/locale.time.get.byname/get_monthname.pass.cpp
  libc++ :: std/localization/locale.categories/category.time/locale.time.get.byname/get_monthname_wide.pass.cpp
  libc++ :: std/localization/locale.categories/category.time/locale.time.get.byname/get_one.pass.cpp
  libc++ :: std/localization/locale.categories/category.time/locale.time.get.byname/get_one_wide.pass.cpp
  libc++ :: std/localization/locale.categories/category.time/locale.time.put.byname/put1.pass.cpp
  libc++ :: std/localization/locale.categories/facet.numpunct/locale.numpunct.byname/grouping.pass.cpp
  libc++ :: std/localization/locale.categories/facet.numpunct/locale.numpunct.byname/thousands_sep.pass.cpp
  libc++ :: std/re/re.traits/lookup_collatename.pass.cpp
  libc++ :: std/re/re.traits/transform_primary.pass.cpp
  libc++ :: std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_shared_for.pass.cpp
  libc++abi :: thread_local_destruction_order.pass.cpp
  libunwind :: libunwind_01.pass.cpp
  libunwind :: signal_frame.pass.cpp
  libunwind :: unw_getcontext.pass.cpp

I've tried to fix some of them, but the locale tests are quite awkward to debug and determine what the correct behaviour should be.
I think thread.sharedtimedmutex.class/try_lock_shared_for.pass.cpp may be be a flaky failure (I believe it usually passes) since I was running it on a system with high CPU load.

dim added a subscriber: tstellar.Feb 15 2021, 12:04 PM

Is the 12.x release fixed, or does this patch need to be cherry-picked to the branch?

It will need to be cherry-picked. I will commit this and create a bugzilla ticket for @tstellar, and link it to the 12.0.0 release ticket.

Would you be willing to add a CI job using FreeBSD?

I don't personally have any hardware that can do the job, but I'd love to defer to either @emaste or @arichardson here. But in any case, we still have a bunch of failing-by-default test cases, mainly in the locale area, that would have to be fixed (or XFAILed, but I dislike this).

dim edited the summary of this revision. (Show Details)Feb 15 2021, 12:21 PM
This revision was automatically updated to reflect the committed changes.

What I would suggest is that we setup a builder for FreeBSD and mark it as a soft fail for the time being. We can then work on making sure all tests are green, and make it a hard-fail. That way, we can make immediate progress on setting up the tester without being tied to fixing a bunch of tests. I think fixing those tests will become a lot easier once we have the CI in place, too.