[PATCH] D80803: [DebugInfo] Add flush to ensure debug line errors are in right place

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D80803#2065086 <https://reviews.llvm.org/D80803#2065086>, @dblaikie wrote:

> In D80803#2065028 <https://reviews.llvm.org/D80803#2065028>, @MaskRay wrote:
>
> > In D80803#2065004 <https://reviews.llvm.org/D80803#2065004>, @dblaikie wrote:
> >
> > > In D80803#2064936 <https://reviews.llvm.org/D80803#2064936>, @MaskRay wrote:
> > >
> > > > I am in favor of synchronizing stdout/stderr, i.e. when redirected to a file, making the behavior similar to a pty (line buffered). This makes stderr output in tests more relevant, i.e. we usually do `2>&1 | FileCheck`, now we can check stderr output is emitted in appropriate places. However, like other opinions, I am wary of littering `flush` all over the code base. Also note that flushing too frequently has a performance penalty when full buffering is otherwise used. (I once made `llvm-objdump -d` slow due to a change to make it line buffered)
> > > >
> > > > Can we do something similar to D64165 <https://reviews.llvm.org/D64165>? Teach the warning/error (stderr) handler to flush stdout? If there is no stderr output, usual buffering (full buffering when the output is a file) is used. There is no performance penalty.
> > >
> > >
> > > If we're going to do it, yeah, D64165 <https://reviews.llvm.org/D64165> (flushing outs() before the diagnostic, flushing the diagnostic stream after the diagnostic) would be the better direction. But even in that case - it seems it isn't consistently applied even within llvm-objdump.cpp (isn't done for command line warnings, nor for errors?)
> >
> >
> > The idea is that some warning/error reporting facilities need `outs().flush()`. For llvm-objdump.cpp, `reportWarning` and `reportError` are the two utilities.
>
>
> reportError doesn't seem to have the flushing (& also I'm not sure if it needs to use an extra raw_string_ostream - could write out to errs() directly, I think) & reportCmdLineWarning, reportCmdLineError (hmm, why are there different reportCmdLine* distinct from report*?)


`reportCmdLineWarning` was introduced in D66418 <https://reviews.llvm.org/D66418>. It reports a warning not associated with a file. Many warnings/errors are associated with a file. `error: filename: ....` style output is convenient when a user greps a log file.

Good catch. If I add `outs().flush()` to `reportError`, no tests fail. We can add it when we see abnormal output. The `logAllUnhandledErrors` use can indeed be simplified.

>> It may be possible that some code path does not consistently flush outs(). We can fix them if someone observes problems.
> 
> Yeah, not the end of the world/fixing at least fewer places.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80803/new/

https://reviews.llvm.org/D80803