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

Mon Oct 5 11:21:03 PDT 2020

dblaikie added a comment.

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

> In D87272#2311061 <https://reviews.llvm.org/D87272#2311061>, @dblaikie wrote:
>
>> In D87272#2310448 <https://reviews.llvm.org/D87272#2310448>, @MaskRay wrote:
>>
>>> In D87272#2295704 <https://reviews.llvm.org/D87272#2295704>, @dblaikie wrote:
>>>
>>>> Not sure I follow - the patch description seems to be quite focussed on the performance implications of this change - it sounds like, reading the patch description/commit message that that is the motivation.
>>>>
>>>> Oh, perhaps I see another reason that might be less nuanced: This will probably break color printing on Windows when using the normal Windows color API - because it's a stateful API, not something that can be encoded in the stream: https://github.com/llvm/llvm-project/blob/d3f6972abb9c0ac06ddabf61697754c3c6f5d11b/llvm/lib/Support/raw_ostream.cpp#L513
>>>
>>> I find another reason, probably still weak: I want to check a large binary with many out of range relocations. lld will emit a large number of warnings (if --noinhibit-exec). This patch can make it faster.
>>
>> Could the uninteresting warnings be disabled? if all the warnings are interesting, I'd guess a bunch of processing would need to be done to make it actionable - or a lot of human time to read through them all, either way that time probably dwarfs the extra time lld takes to print those errors.
>
> No. They were errors but downgraded to warnings. For the binaries I investigated, its rodata+text+data are significantly larger than 2GiB so many out of range relocation are expected.

Clang has a concept of warnings that are default errors - so they can be downgraded to warnings again, and disabled individually. Perhaps that could be used here.

>> In any case - I think colour can't be portably buffered (well, in addition to the existing buffering in the ostream itself) so far as I can see in the implementation. Does that understanding seem right? Is there a way that could be addressed?
>
> The code makes messages line buffered, instead of full buffered. (Not to say that buffered ANSI escape sequences work as well). The currently mere downside of this patch is the introduction of `reportDiagnostic` (added complexity). There are a number of small advantage on the up side, though.

Hmm, I think we're still miscommunicating here. Not all platforms use ANSI escape sequences - Windows uses APIs to change colour on the terminal, so there's no way to buffer coloured output there. Well, it seems like LLVM has an option to enable ANSI escape codes on Windows - so perhaps on some terminals that's used? But generally it's done via API on the file descriptor (see: https://github.com/llvm/llvm-project/blob/6e25586990b93e2c9eaaa4f473b6720ccd646c46/llvm/lib/Support/Windows/Process.inc#L369 and related use ( https://github.com/llvm/llvm-project/blob/6e25586990b93e2c9eaaa4f473b6720ccd646c46/llvm/lib/Support/raw_ostream.cpp#L513 )). So my understanding is that portable color rendering cannot be buffered as you're doing here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87272