Page MenuHomePhabricator

[lld] Enable Visual Studio compatible output
Needs ReviewPublic

Authored by chrisjackson on Feb 15 2019, 12:13 PM.

Details

Summary

This builds a little on @ikudrin's diff D50560 which was a much improved version of D47540. I have attempted to take into account the comments on those diffs.

Adds diagnostic output functions to enable Visual Studio
compatibility with the flag --vs-diagnostics. Clang has a
flag which enables similar support, see
"-fdiagnostics-format=clang/msvc/vi" described
in https://clang.llvm.org/docs/UsersManual.html.
The existing diagnostics output format for warning and error messages is
not fully compatible with Visual Studio (VS) and can even cause VS to become
unresponsive.

The format accepted by VS
(https://msdn.microsoft.com/en-us/library/yxkt8b26.aspx):

{filename (line# [, column#]) | toolname} : [any text] {error | warning}
code####:localizable string [ any text ]

or more simply:

Origin : {warning|error} code#### : Message

The current default lld format uses the lld executable path for Origin.
Clicking the message inside VS will lead to a hang or long delay as VS
attempts to access the executable as a source code origin. Additionally, the
line and column information accepted by VS differs from the lld format.

Diff Detail

Event Timeline

chrisjackson created this revision.Feb 15 2019, 12:13 PM
aganea added inline comments.Feb 15 2019, 1:09 PM
Common/ErrorHandler.cpp
143

Could you please use clang-format on the whole patch? (only changed lines)

ruiu added a comment.Feb 15 2019, 3:00 PM

I think we need to be very careful how to implement the feature. Currently it is very easy to understand what lld prints out as an error message and how to construct an error message. Basically, what you see in the source code is what you get at runtime. This patch added an abstraction layer between the current code and stderr, and now it is not easy to figure out what is the actual output and how to construct a desired error message.

If we have no choice other than implementing two different error format styles, the cost of abstraction can be justified. Everybody would understand why we would have to abstract an error message so that they are outputted in an appropriate format. However, frankly speaking, MSVC-compatible error output is a nice-to-have for lld/ELF but for most lld/ELF users that's not something they care about too much. It wouldn't be wrong to expect that the current error message format remains as "primary" and the MSVC-style output remains as a distant number two.So I'm not personally happy to add an abstraction layer to support both. I'd like to search for a way to implement the MSVC-style error message in such a way that that doesn't complicate the normal execution pass.

I remember we had a similar discussion when we were talking about how to add MinGW support to lld. One way of doing it was to directly add MinGW support to lld/coff, so that lld-link works both as a MSVC link.exe and a MinGW linker. That way, both the existing lld/coff and lld/mingw pays the cost of supporting the abstraction cost of supporting lld/mingw, but that didn't look like a good design choice given that the need for lld/mingw is not as large as lld/coff. We ended up having a good compromise, which is, implementing lld/mingw as a wrapper to lld/coff by translating GNU-ish command line options to Windows-ish ones and then call the entry point function of lld/coff. That was less intrusive than directly adding a mingw support.

So I think we should do something similar for supporting MSVC-style error message, instead of fully abstracting the error message so that they can be output in any format.

I'd propose detecting error message patterns in error() and textually replacing it, so that the actual output becomes the MSVC style. I believe that would work for most cases. If there's an error message that doesn't work well with the textual conversion approach, we could change the original error message so that it is easy to handle by a text matcher. It may not look very "clean" approach, and it may even look a dumb approach, but I'd like to choose the dumb one if it works for the sake of code readability.

Used clang-format-diff against the patch is requested by @aganea.

I like @ruiu 's suggestion in D58484, it is simple and elegant. We don't need performance there, so a regex is fine.
In both cases, yours and @ruiu 's, it would be good to demonstrate usage in the COFF driver as well.
There will be less changes to the LLD codebase with the regex approach. More code changes/additions means more maintenance in the long term.
What do you think on building on @ruiu 's proposal, Chris?

Common/ErrorHandler.cpp
148

There are a lot of new functions here, and it is not immediately clear which ones should be used. I fail to see at first sight which APIs to use when I want to report errors. Before, they were local to a TU, and maybe more specific for certain use-cases. Probably adding some comments and/or usage examples here would help. However - please see my other comment before doing that.

chrisjackson marked an inline comment as done.Feb 27 2019, 9:25 AM

I like @ruiu 's suggestion in D58484, it is simple and elegant. We don't need performance there, so a regex is fine.
In both cases, yours and @ruiu 's, it would be good to demonstrate usage in the COFF driver as well.
There will be less changes to the LLD codebase with the regex approach. More code changes/additions means more maintenance in the long term.
What do you think on building on @ruiu 's proposal, Chris?

I intend to extend Rui's example to pass a couple of the existing tests and see how it works out. I'll comment more on this at D58484.