This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Remove no-op flush of errs
ClosedPublic

Authored by abrachet on Jun 13 2019, 9:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

abrachet created this revision.Jun 13 2019, 9:18 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 13 2019, 9:31 PM
This revision was automatically updated to reflect the committed changes.

@jhenderson said that I should put this into a micro patch and that didn't need review if all tests pass. They did so I went ahead and committed this before review. https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/raw_ostream.cpp#L854 shows that errs is unbuferred.

That's actually kind-of a problem. Unless *something* ensures that all the output
that was written into that is flushed, it may end up being dropped, rL360124.

That's actually kind-of a problem. Unless *something* ensures that all the output
that was written into that is flushed, it may end up being dropped, rL360124.

It depends on the stream being used errs() shouldn't need flushing because of its unbuffered state. Other streams (as fixed by rL360124) will do.

That's actually kind-of a problem. Unless *something* ensures that all the output
that was written into that is flushed, it may end up being dropped, rL360124.

It depends on the stream being used errs() shouldn't need flushing because of its unbuffered state.

What about llvm::outs()?

Other streams (as fixed by rL360124) will do.

What about llvm::outs()?

llvm::outs() is buffered, so would need flushing, although I vaguely remember seeing something in the code somewhere that automatically did it on process termination.

To be clear, is there an issue with this specific change, which is dealing with llvm::errs() only?

What about llvm::outs()?

llvm::outs() is buffered, so would need flushing,

Ah yes, i see it now, in llvm::errs() (in raw_ostream.cpp) the stream is created explicitly unbuffered, which is not the case for llvm::outs()

although I vaguely remember seeing something in the code somewhere that automatically did it on process termination.

No, there is no such thing i'm afraid, else i would not have had that bug..

To be clear, is there an issue with this specific change, which is dealing with llvm::errs() only?

No, looks like all good, sorry about the noise.

although I vaguely remember seeing something in the code somewhere that automatically did it on process termination.

No, there is no such thing i'm afraid, else i would not have had that bug..

raw_ostream does not flush in its destructor (although it does assert that the buffer is empty) but all sub classes in raw_ostream.h flush their buffers in their destructors. outs() would have its destructor called by __cxa_atexit, @jhenderson is right.

raw_fd_ostream's destructor can be found here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/raw_ostream.cpp#L592