This is an archive of the discontinued LLVM Phabricator instance.

Add freestanding parameter to libcxx lit
AbandonedPublic

Authored by ldionne on Jan 1 2021, 6:08 PM.

Details

Reviewers
jfb
bcraig
Group Reviewers
Restricted Project
Summary

Direct invocations of lit can now include a --param=freestanding=True flag. This will add all the libcxx lit features that are associated with conforming freestanding environments.

This patch also adds a new feature that isn't yet mentioned in any tests, libcpp-any-freestanding. The intent is to use this feature in UNSUPPORTED statements when a more specific feature isn't available.

The new freestanding parameter intentionally does not include no-rtti or no-exceptions, as RTTI and exceptions are currently required in conforming freestanding implementations. My expectation is the --param=enable-rtti=False and --param=enable-exceptions=False will frequently accompany --param=freestanding=True.

Diff Detail

Event Timeline

bcraig created this revision.Jan 1 2021, 6:08 PM
bcraig requested review of this revision.Jan 1 2021, 6:08 PM
bcraig added a subscriber: Restricted Project.Jan 1 2021, 6:24 PM
bcraig set the repository for this revision to rG LLVM Github Monorepo.Jan 4 2021, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2021, 6:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

gentle ping

ldionne requested changes to this revision.Jan 25 2021, 7:39 AM
ldionne added inline comments.
libcxx/utils/libcxx/test/params.py
42–50

These options are meant to be determined at libc++ configuration time (in CMake). They are not meant to be overriden on the command-line. In other words, a "general" build of libc++ may not support defining these macros like we're doing here.

I believe we should treat libc++ and its test suite as separate entities here. Basically, let's add support for running only the freestanding parts of the test suite, but let's not tie libc++ configuration to it. If you want to a C++ Library implementation against the Freestanding spec, the idea would be:

  1. Build your C++ Library implementation in a way that you think it satisfies Freestanding (for libc++, that would mean defining all the macros above at CMake configure time).
  2. Run the test suite against your implementation, enabling only the freestanding subset.

If we do it that way, this patch wouldn't contain anything specifically related to libc++. What do you think?

This revision now requires changes to proceed.Jan 25 2021, 7:39 AM
bcraig updated this revision to Diff 320314.Jan 30 2021, 10:43 AM

Removing the preprocessor defines, in order to divorce the tests from the library under test.

ldionne requested changes to this revision.Feb 2 2021, 12:53 PM

I like this much better! I guess a follow-up would be to rename some of these libcpp-has-no-xyz features to just has-no-xyz to divorce them from libc++ and make it clear that they are features of the test suite itself, too.

I'm going to be nit-picky and ask that we add a CI job for testing that configuration. This is easily done with the new buildkite setup. You can follow what I did for example in

commit e0d01294bc124211a8ffb55e69162eb34a242680
Author: Louis Dionne <ldionne@apple.com>
Date:   Thu Oct 15 10:32:09 2020 -0400

    [libc++] Allow building libc++ on platforms without a random device

    Some platforms, like several embedded platforms, do not provide a source
    of randomness through a random device. This commit makes it possible to
    build and test libc++ for such platforms, i.e. without std::random_device.

    Surprisingly, the only functionality that doesn't work on such platforms
    is std::random_device itself -- everything else in <random> still works,
    one just has to find alternative ways to seed the PRNGs.
This revision now requires changes to proceed.Feb 2 2021, 12:53 PM
bcraig marked an inline comment as done.Feb 4 2021, 3:37 PM

I haven't messed with the pipeline or buildbot scripts before. Is there some way for me to test those changes before landing them?

I haven't messed with the pipeline or buildbot scripts before. Is there some way for me to test those changes before landing them?

Yes, just make the changes and update your review. It'll run the updated CI and we can see if that works.

Messed with this some over the weekend, and realized there's ambiguity in the request. I also found that there are some challenges with each of the reasonable things that I think you could be requesting. Note that all 3 approaches result in failing tests, and fixing those tests will take time.

Potential path 1:
Test an (as much as possible) conforming freestanding libcxx configuration with the new -freestanding parameter. This seems mostly useful, but it doesn't do much to test the above code, as there is already code that will take libc++ configuration macros and turn them into features. This approach also has a ton of failures, from things like printf usage in helper functions. I got these results...

Unsupported        : 1664
Passed             : 4726
Expectedly Failed  :   46
Failed             :   71
Unexpectedly Passed:    3

Potential path 2:
Test a "useful" conforming freestanding libcxx configuration (i.e. turn off exceptions and RTTI). This has most of the same problems as above, but changes which tests need to be adjusted.

Potential path 3:
Narrowly test the specific changes in this request. The easiest way to do that is to build a hosted libc++, and run the freestanding tests against it. This results in a few test failures. Some of them are of dubious value. For example, there's a test that makes sure the lit feature and the _LIBCPP_HAS_NO_THREADS preprocessor value match. The advantage of this approach is that it tests the above changes, as old code paths would take configuration values and automatically add the associated lit features. There were something like 4 FAILS and 4 XPASSes with this approach.

I think both paths 1 and 2 make sense long term. They start "actually" testing the new freestanding parameter as soon as we start sprinkling UNSUPPORTED:libcpp-any-freestanding all over the place, but it will likely be months before we get a passing CI run.

Any thoughts on what you would like to see? Is it ok to have a broken CI bot for a while? Or do I need to fix the tests as part of this change as well?

ldionne commandeered this revision.Aug 31 2023, 12:42 PM
ldionne edited reviewers, added: bcraig; removed: ldionne.

I think both paths 1 and 2 make sense long term. They start "actually" testing the new freestanding parameter as soon as we start sprinkling UNSUPPORTED:libcpp-any-freestanding all over the place, but it will likely be months before we get a passing CI run.

Any thoughts on what you would like to see? Is it ok to have a broken CI bot for a while? Or do I need to fix the tests as part of this change as well?

I think this is the approach we should take. We should prepare this patch (the test suite would fail). Then, we can make iterative fixes to libc++ as separate patches until this patch finally passes.

We are moving to GitHub PRs so I'll need to close this, but I would suggest re-opening this as a PR if you still have interest in pursuing this. It would be really nice to make libc++ officially conform to Freestanding.

Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 12:42 PM
ldionne abandoned this revision.Aug 31 2023, 12:42 PM