This is an archive of the discontinued LLVM Phabricator instance.

Make the presence of stdin and stdout optional
ClosedPublic

Authored by ed on Mar 14 2015, 5:07 AM.

Details

Summary

The idea behind Nuxi CloudABI is that it is targeted at (but not limited to) running networked services in a sandboxed environment. The model behind stdin, stdout and stderr is strongly focused on interactive tools in a command shell. CloudABI does not support the notion of stdin and stdout, as 'standard input/output' does not apply to services. The concept of stderr does makes sense though, as services do need some mechanism to log error messages in a uniform way.

This patch extends libc++ in such a way that std::cin and std::cout and the associated <cstdio>/<cwchar> functions can be disabled through the flags _LIBCPP_HAS_NO_STDIN and _LIBCPP_HAS_NO_STDOUT, respectively. At the same time it attempts to clean up src/iostream.cpp a bit. Instead of using a single array of mbstate_t objects and hardcoding the array indices, it creates separate objects that declared next to the iostream objects and their buffers. The code is also restructured by interleaving the construction and setup of c* and wc* objects. That way it is more obvious that this is done identically.

The c* and wc* objects already have separate unit tests. Make use of this fact by adding XFAILs in case libcpp-has-no-std* is set. That way the tests work in both directions. If stdin or stdout is disabled, these tests will therefore test for the absence of c* and wc*.

My promise is that this will be the last intrusive patch to add support for CloudABI. ;-)

Diff Detail

Repository
rL LLVM

Event Timeline

ed updated this revision to Diff 21983.Mar 14 2015, 5:07 AM
ed retitled this revision from to Make the presence of stdin and stdout optional.
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added reviewers: jroelofs, mclow.lists.
ed set the repository for this revision to rL LLVM.
ed added a subscriber: Unknown Object (MLST).
jroelofs edited edge metadata.Mar 14 2015, 8:56 AM

At the same time it attempts to clean up src/iostream.cpp

Mind separating this into two logical patches?

ed added a comment.Mar 14 2015, 9:17 AM

At the same time it attempts to clean up src/iostream.cpp

Mind separating this into two logical patches?

No problem at all! I've just sent out the following code review:

http://reviews.llvm.org/D8342

ed updated this revision to Diff 21987.Mar 14 2015, 9:20 AM
ed edited edge metadata.
ed removed rL LLVM as the repository for this revision.

This part looks OK.

Is there any value in gating the stdout & stdin bits separately?

test/std/input.output/iostream.objects/narrow.stream.objects/cin.pass.cpp
1

Wow, this test is confusing now. I'm not sure how to make that better.

ed added a comment.Mar 15 2015, 1:57 AM

This part looks OK.

Is there any value in gating the stdout & stdin bits separately?

The main reason I went for this approach was because I felt that placing it behind one knob would raise the question: "What's so special about these two? Why is stderr treated separately? Why not disable stdin and stderr, or stdout and stderr?" The patch demonstrates that apart from the tying, stdin, stdout and stderr are separate.

That said, I wouldn't mind merging this, as long as we can get a nice compromise between getting the code to work on CloudABI while keeping it readable. Thoughts?

jroelofs accepted this revision.Mar 16 2015, 8:27 AM
jroelofs edited edge metadata.

Ok, keeping them separate sounds reasonable to me. LGTM.

This revision is now accepted and ready to land.Mar 16 2015, 8:27 AM
EricWF edited edge metadata.Mar 16 2015, 8:45 AM

The LIT stuff looks good to me. I haven't reviewed the changes to the library though.

include/__config
735–741

What I like about _LIBCPP_HAS_NO_MONOTONIC_CLOCK and _LIBCPP_HAS_NO_THREADS is that they must be explicitly defined be the user. We don't automatically provide those configurations by way of the __config header. I like this because those flags make libc++ become a non-conforming standard library.

Along the same vein I'm not sure I like __config having configuration paths that make libc++ non-conforming. I understand why this is done in the case of __CloudABI__ and I'm not objecting. I just want to air my uneasiness.

test/std/input.output/iostream.objects/wide.stream.objects/wcin.pass.cpp
19

Unrelated to this patch but these tests need to be looked into. They seem fishy.

test/std/input.output/iostream.objects/wide.stream.objects/wcout.pass.cpp
18

There is something fishy about this test already.

jroelofs added inline comments.Mar 16 2015, 9:12 AM
include/__config
735–741

What I like about _LIBCPP_HAS_NO_MONOTONIC_CLOCK and _LIBCPP_HAS_NO_THREADS is that they must be explicitly defined be the user.

I can see the reasoning behind it, but this is really inconvenient for me. The problem is that it's not reasonable to expect my users to #define these things, so locally I added a <__config_site> that #defines _LIBCPP_HAS_NO_MONOTONIC_CLOCK and _LIBCPP_HAS_NO_THREADS, which is #included at the top of <__config>.

I didn't realize this before, but I think the best way forward here would be to have cmake generate the <__config_site>, and stick it in an include dir in the build directory. Then at install time, have it copy that file to the install dir. This would have the added benefit of making the -D_LIBCPP_HAS_NO_THREADS=1 things in config.py go away.

How does that sound, @EricWF?

test/std/input.output/iostream.objects/wide.stream.objects/wcout.pass.cpp
18

Indeed... this part is begging to be an ShTest test.

EricWF added inline comments.Mar 16 2015, 9:22 AM
include/__config
735–741

I like the idea of that but I'm not sure it helps fix this problem per se since it still allows for implicit non-conforming configurations (although I greatly sympathize with the rational for it) I would want to run it by @mclow.lists first. I've thought about this before and my main concern is that it would make reproducing bugs a lot more difficult because every user has a different <__config_site> header.

Perhaps we allow for a <__config_site> header to be used, but we only ever provide an empty one with a big comment at the top warning users about modifying it. Then if somebody really needs one of these configurations they can go take the time to manually fill it out with the required definitions. This would make it trickier to use the header in a regular build/test workflow though.

jroelofs added inline comments.Mar 16 2015, 9:27 AM
include/__config
735–741

This would be something that is completely generated from the cmake configure line. I don't think it would change the repro steps at all because we'd already have to know what their configure line was.

The added benefit here is that it would keep a record on the end user's system of what flags their libc++ library was built with.

mclow.lists edited edge metadata.Mar 17 2015, 11:35 AM

Does it make sense to have separate switches for stdin and stdout, as opposed to just "the standard streams"?

Or does CloudABI support stderr?

include/__config
735–741

That sounds promising.

Like @ErikWF, I am leery of making it convenient for users to (inadvertently, easily) build non-conforming versions of libc++ - for no reason other than the support questions/bug reports that filter back to us.

Ok, I see that @jroelofs already raised that issue.
Read the conversation too fast - sorry.

ed added a comment.Mar 17 2015, 11:47 AM

Or does CloudABI support stderr?

Yes, only stderr. For a service, stdin and stdout make little sense, whereas stderr can still be used for logging. For example, assert() will log its diagnostics to stderr.

jroelofs added inline comments.Mar 17 2015, 12:05 PM
include/__config
735–741

We could add cmake message(WARNING "Disabling FOO makes libc++ a non-conforming library") when those get turned on, and also leave a note in the generated __config_site file saying roughly the same thing. That way the person building the library gets a warning that maybe they should re-consider what they're doing, and their end-user gets an easy answer for why there aren't any FOO symbols/declarations in the library/headers.

This revision was automatically updated to reflect the committed changes.