This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] change dylib- XFAILs to UNSUPPORTED
AbandonedPublic

Authored by ldionne on May 14 2019, 1:14 PM.

Details

Reviewers
EricWF
bcain

Diff Detail

Event Timeline

bcain created this revision.May 14 2019, 1:14 PM

I'd like to better understand what problem this is solving:

  • What platform are you testing on?
  • How did you configure CMake?
  • How do you run lit?
bcain added a comment.May 14 2019, 3:08 PM

I'd like to better understand what problem this is solving:

  • What platform are you testing on?
  • How did you configure CMake?
  • How do you run lit?

Here's what I think are relevant commits that introduced this behavior:

The problem I get are XPASS on the following tests:

Unexpected Passing Tests (7):
    libc++ :: std/input.output/file.streams/fstreams/filebuf.members/open_path.pass.cpp
    libc++ :: std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp
    libc++ :: std/input.output/file.streams/fstreams/fstream.members/open_path.pass.cpp
    libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
    libc++ :: std/input.output/file.streams/fstreams/ifstream.members/open_path.pass.cpp
    libc++ :: std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp
    libc++ :: std/input.output/file.streams/fstreams/ofstream.members/open_path.pass.cpp

I was able to reproduce this problem with ToT and x86_64 linux if I set LIBCXX_ENABLE_FILESYSTEM to "False" or "OFF" with just ninja check-cxx. AFAICT diffusion/phabricator won't let me upload arbitrary files like the full unabridged logs so I'll find another place to put them.

AFAIK the idiom for XFAIL is intended for a WIP feature or a defect that can't or shouldn't be fixed (yet) and if this test starts passing it requires some investigation. I am assuming that dylib-has-no-* is just indicating that a relevant feature for this test is missing or disabled on this target, and as such I would suggest that it doesn't make sense to execute the test at all if the feature is missing. And if it passes for some reason in this configuration it's not interesting and worth investigation. Maybe the test no longer depends on the feature, but is that worthy of failing the test suite? So that's why I have applied this change to all similar tests, not just the ones that I see the XPASS on.

But maybe that's not the case? Maybe these tests don't really depend on this feature? Or maybe r356640 should have created a new feature instead of wiring ENABLE_FILESYSTEM into the dylib-has-no-filesystem one?

AFAIK the idiom for XFAIL is intended for a WIP feature or a defect that can't or shouldn't be fixed (yet) and if this test starts passing it requires some investigation. I am assuming that dylib-has-no-* is just indicating that a relevant feature for this test is missing or disabled on this target, and as such I would suggest that it doesn't make sense to execute the test at all if the feature is missing. And if it passes for some reason in this configuration it's not interesting and worth investigation. Maybe the test no longer depends on the feature, but is that worthy of failing the test suite? So that's why I have applied this change to all similar tests, not just the ones that I see the XPASS on.

But maybe that's not the case? Maybe these tests don't really depend on this feature? Or maybe r356640 should have created a new feature instead of wiring ENABLE_FILESYSTEM into the dylib-has-no-filesystem one?

That's not my thinking.

  • XFAIL is "expected to fail" (for whatever reason)
  • UNSUPPORTED is "this test is not worth running"

(I'm going to comment on just the variant/optional/ any tests here, because I think they're straightforward).
They're marked as XFAIL when the dylib doesn't have the appropriate exception classes - and they should fail.
They also should fail if they are built with exceptions disabled - because the exceptions will never get thrown. This behavior should be confirmed.

If they pass, then something very strange is going on, and should be investigated.
Either the test is not testing what we want, or the dylib is not getting built in the manner that we expect.

bcain added a comment.May 15 2019, 9:43 AM

...

(I'm going to comment on just the variant/optional/ any tests here, because I think they're straightforward).
They're marked as XFAIL when the dylib doesn't have the appropriate exception classes - and they should fail.
They also should fail if they are built with exceptions disabled - because the exceptions will never get thrown. This behavior should be confirmed.

Fair enough.

So I took a minute to try and understand the XPASS and I think that the filesystem header installation isn't guarded by LIBCXX_ENABLE_FILESYSTEM -- only the sources.

Maybe the presence of the filesystem header is enough to get some of these tests to pass. So maybe I should go ahead and change this patch to do that instead?

bcain added a comment.May 15 2019, 9:44 AM

...

Maybe the presence of the filesystem header is enough to get some of these tests to pass. So maybe I should go ahead and change this patch to do that instead?

"do that" -- change the include/CMakeLists.txt to make the filesystem header installation conditional on LIBCXX_ENABLE_FILESYSTEM, I mean.

Maybe the presence of the filesystem header is enough to get some of these tests to pass. [...]

Yes, I think this is exactly what's happening, and this is what I wanted to understand. I think the best solution would be for LIBCXX_ENABLE_FILESYSTEM to actually control whether filesystem is enabled in the library, however this includes:

  • whether we build support for filesystem in the dylib (that's the only thing we already do)
  • whether the <filesystem> header is provided
  • whether we provide operations like std::ifstream::open() on std::filesystem::path & friends

I think if we had that, then we could rename the lit feature from dylib-has-no-filesystem to just has-no-filesystem, and that feature would be enabled whenever:

  • we are testing back-deployment to a platform that doesn't have dylib support for filesystem (and has availability markup that makes it invalid to use stuff like std::ifstream::open(std::filesystem::path))
  • we are testing libc++ trunk without filesystem support (your use case)

Is this something you'd be willing to investigate? I think having a good example of how to disable complete features from the library would be useful for other things as well, so this is something we need to do at some point. For example, our story for things like LIBCXX_ENABLE_STDIN and LIBCXX_ENABLE_STDOUT is similarly broken.

bcain added a comment.May 15 2019, 1:33 PM

Maybe the presence of the filesystem header is enough to get some of these tests to pass. [...]

Yes, I think this is exactly what's happening, and this is what I wanted to understand. I think the best solution would be for LIBCXX_ENABLE_FILESYSTEM to actually control whether filesystem is enabled in the library, however this includes:

  • whether we build support for filesystem in the dylib (that's the only thing we already do)
  • whether the <filesystem> header is provided
  • whether we provide operations like std::ifstream::open() on std::filesystem::path & friends

I think if we had that, then we could rename the lit feature from dylib-has-no-filesystem to just has-no-filesystem, and that feature would be enabled whenever:

  • we are testing back-deployment to a platform that doesn't have dylib support for filesystem (and has availability markup that makes it invalid to use stuff like std::ifstream::open(std::filesystem::path))
  • we are testing libc++ trunk without filesystem support (your use case)

Is this something you'd be willing to investigate? I think having a good example of how to disable complete features from the library would be useful for other things as well, so this is something we need to do at some point. For example, our story for things like LIBCXX_ENABLE_STDIN and LIBCXX_ENABLE_STDOUT is similarly broken.

Sure, I will take a look. I tried whipping up a quick patch but I wasn't able to clean up all of the references from std::*fstream (though I think I figured out chrono).

Also, I based that quick patch on the handling for LIBCXX_ENABLE_THREADS / _LIBCPP_HAS_NO_THREADS. In that case, <thread> is still provided but hits an #error when included. Does that suffice for what you think <filesystem> should do or should we really suppress the installation?

Maybe the presence of the filesystem header is enough to get some of these tests to pass. [...]

Yes, I think this is exactly what's happening, and this is what I wanted to understand. I think the best solution would be for LIBCXX_ENABLE_FILESYSTEM to actually control whether filesystem is enabled in the library, however this includes:

  • whether we build support for filesystem in the dylib (that's the only thing we already do)
  • whether the <filesystem> header is provided
  • whether we provide operations like std::ifstream::open() on std::filesystem::path & friends

I think if we had that, then we could rename the lit feature from dylib-has-no-filesystem to just has-no-filesystem, and that feature would be enabled whenever:

  • we are testing back-deployment to a platform that doesn't have dylib support for filesystem (and has availability markup that makes it invalid to use stuff like std::ifstream::open(std::filesystem::path))
  • we are testing libc++ trunk without filesystem support (your use case)

Is this something you'd be willing to investigate? I think having a good example of how to disable complete features from the library would be useful for other things as well, so this is something we need to do at some point. For example, our story for things like LIBCXX_ENABLE_STDIN and LIBCXX_ENABLE_STDOUT is similarly broken.

Sure, I will take a look. I tried whipping up a quick patch but I wasn't able to clean up all of the references from std::*fstream (though I think I figured out chrono).

Also, I based that quick patch on the handling for LIBCXX_ENABLE_THREADS / _LIBCPP_HAS_NO_THREADS. In that case, <thread> is still provided but hits an #error when included. Does that suffice for what you think <filesystem> should do or should we really suppress the installation?

I think an #error is OK for now, but in the long term we'll want to make this friendlier to __has_include.

Is this still relevant? If not, can we close this to clean up the review queue?

ldionne commandeered this revision.Nov 2 2020, 2:20 PM
ldionne edited reviewers, added: bcain; removed: ldionne.

Abandoning to clear up the review queue since I don't think that's relevant anymore. If it still is, please re-open!

ldionne abandoned this revision.Nov 2 2020, 2:20 PM