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

Mon Oct 5 14:03:48 PDT 2020

MaskRay added a subscriber: aganea.
MaskRay added a comment.

In D87272#2312326 <https://reviews.llvm.org/D87272#2312326>, @dblaikie wrote:

> 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.

Then we will need to communicate the -Wno- invention to binutils.

Such an opt-in option is still a bit inconvenient. For the inappropriate usage of D87121 <https://reviews.llvm.org/D87121> and the out-of-range relocation warnings, if we do this patch, the performance is good enough that I don't need to care about using an option. With the opt-in option, I'll need to add it to the link.

An alternative is to introduce a warning limit, but that will add new code, too (and it needs to be justified as well).

>>> 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.

This is indeed a Windows problem. It is similar to https://reviews.llvm.org/D88348#2299650 Maybe @aganea has some thoughts whether buffering can be made.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87272