[PATCH] D87272: [lld] Buffer writes when composing a single diagnostic

Thu Sep 9 17:32:01 PDT 2021

dblaikie added a comment.

cs/raw

In D87272#2993067 <https://reviews.llvm.org/D87272#2993067>, @MaskRay wrote:

> In D87272#2991991 <https://reviews.llvm.org/D87272#2991991>, @dblaikie wrote:
>
>> I still don't think this patch is appropriate, in part due to the colour diagnostic issue - coloured output can't, in general, be buffered (due to Windows having a stateful/API based colour changing, rather than ANSI based colour changing - at least in some cases)
>
> `sys::Process::UseANSIEscapeCodes(true);` enables ANSCI escape codes for Windows.

Presumably there's a reason that's not the only option/hardcoded, though?

>> If people are getting pages and pages of errors they're ignoring/want to perform quickly - that seems like something that deserves a different solution: turning off the warnings they are ignoring.
>
> This is separate: whether we need an option like `--warn-limit=N`.

I don't think it is separate, though - this change seems to add extra complications to the code to improve performance for a case that doesn't seem like it should matter (generally I don't think error code paths are optimized like this/planned for performance of large numbers of errors being produced, the general design guidance is that once you have to show a message a human has to read and think about, the actual runtime it takes to produce that message is always going to be smaller than the human time required to understand and act on it)

> Separate options turning off warnings seem less useful to me. There are some warnings which don't deserve an option (which would make things more complicate). Off-by-default options need the users to rerun the link to turn off it.

But you've mentioned multiple cases (both in the patch description ("For an invocation of ld.lld with 62000+ lines of..."), and in the renewed motivation to commit this patch (" I noticed another internal user complaining about link timeout due to spending too much time dumping the messages.")).

> This patch addresses another problem: I have seen interleaved diagnostic (something like `msg_from_process_0 msg_from_process_1 msg_from_process_1` on one line) from either Clang or ld.lld from multiple processes.
> This patch will avoid such issue.

That motivation isn't mentioned in the commit message - the scalability of 10s of thousands of diagnostics is mentioned (& the convenience of strace - also doesn't seem like a huge motivation to me).

(I'm not sure hardcoding the ANSI escape code option to on in rGbcc34ab6c8ab7bdff6c520c824d6d71ddac7a896 <https://reviews.llvm.org/rGbcc34ab6c8ab7bdff6c520c824d6d71ddac7a896> is right either - clang doesn't do that for instance, FileCheck probably does because it doesn't have to be as portable and mostly its output goes through lit anyway, so it's the only way it can produce colour through the extra layer of buffering being done by lit anyway)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87272