[PATCH] D47540: [lld] Enable Visual Studio compatible output

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added a comment.

In https://reviews.llvm.org/D47540#1124137, @ruiu wrote:

> Could you share a few real examples of error messages with and without this patch to see how they look like?


I've expanded the vs-diagnostics.s lit test with some extra examples to show the 
difference in output with and without --vs-diagnostics enabled for several error 
call scenarios. I've enumerated the examples so they can be checked against the output
at the end of the message. Currently the errors originate from:

1: 'unrecognised flag' which calls error without a source parameter and arguably

  shouldn't have this parameter i.e. error(Msg) 

2: 'undefined symbol' which uses the new error(Msg, Src) interface and debug

  line information is available.

3: 'duplicate symbol' which uses an unmodified error(Msg) call and no debug line

  information is available.

In --vs-diagnostics mode, calling warn()/ error() without the source parameter 
defaults the 'Origin' component of the message to the linker executable name 
without the .exe extension. This lets the  message indicate the originator
 of the diagnostic while remaining VS diagnostic format conformant.

I've included the lld output from the lit tests below for comparison as requested. The lit 
tests also check that the newline behaviour for multi-line diagnostic messages has
been preserved, by including two undefined symbols in test 2.

Test 1 without --vs-diagnostics
X:\git\upstream\lld_scripts_build\Debug\bin\ld.lld.EXE : error: undefined symbol: flump

>>> referenced by vs-diagnostics.s:1215
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp.o:(.text+0x1)

X:\git\upstream\lld_scripts_build\Debug\bin\ld.lld.EXE : error: undefined symbol: foo

>>> referenced by vs-diagnostics.s:1066
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp.o:(.text+0x6)

Test 1 with --vs-diagnostics
vs-diagnostics.s (1215) : error: undefined symbol: flump

>>> referenced by vs-diagnostics.s (1215)
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp.o:(.text+0x1)

vs-diagnostics.s (1066) : error: undefined symbol: foo

>>> referenced by vs-diagnostics.s (1066)
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp.o:(.text+0x6)

Test 2 without --vs-diagnostics
X:\git\upstream\lld_scripts_build\Debug\bin\ld.lld.EXE : error: duplicate symbol: wump

>>> defined at vs-diagnostics-multiply-defined.s
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp2.o:(.text+0x0)
>>> 
>>> defined at vs-diagnostics-multiply-defined.s
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp2.o:(.text+0x0)

Test 2 with --vs-diagnostics
ld.lld : error: duplicate symbol: wump

>>> defined at vs-diagnostics-multiply-defined.s
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp2.o:(.text+0x0)
>>> 
>>> defined at vs-diagnostics-multiply-defined.s
>>> 
>>>   X:\git\upstream\lld_scripts_build\tools\lld\test\ELF\Output\vs-diagnostics.s.tmp2.o:(.text+0x0)

Test 3 without --vs-diagnostics
X:\git\upstream\lld_scripts_build\Debug\bin\ld.lld.EXE : error: unknown argument: --nonexistantflag

Test 3 with --vs-diagnostics
ld.lld : error: unknown argument: --nonexistantflag



================
Comment at: ELF/Driver.cpp:76-78
+  errorHandler().LogName = errorHandler().VSDiagnostics
+                               ? sys::path::stem(sys::path::filename(Args[0]))
+                               : Args[0];
----------------
ruiu wrote:
> This should be moved to ErrorHandlers.cpp.
This additional set of LogName was unnecessary and is now solely in DriverUtils.cpp ELFOptTable::parse(). Other errorHandler properties are set here too, so I don't know if they should all be moved? I'd be happy to create a common SetupErrorHandler() function that sets the errorhandler properties in one place -- should this be a separate change?


================
Comment at: ELF/Driver.cpp:682-683
 
+  errorHandler().VSDiagnostics =
+      Args.hasArg(OPT_visual_studio_diagnostics_format);
+
----------------
ruiu wrote:
> This is perhaps too late. This should be processed at the same timing as --color-diagnostics.
Agreed. I've moved this.


================
Comment at: ELF/DriverUtils.cpp:134
+
+  // remove executable name from the argument vectorso it is not later
+  // interpreted as an input object file
----------------
ruiu wrote:
> Why do you need to add this code? If it is working now, you don't need to do this.
As we need the linker executable name for the diagnostics output, I removed the ArgsArr.slice(1) from LinkerDriver::main, as well as removing this Vec.erase. To make things clearer I've separated the linker name into a separate Stringref that is passed alongside the Argv ArrayRef.


================
Comment at: ELF/InputFiles.cpp:88
+    return Path.str() + " " + "(" + std::to_string(Line) + ")";
+  } else {
+    std::string Filename = path::filename(Path);
----------------
ruiu wrote:
> No else after return.
I've removed the else.


================
Comment at: include/lld/Common/ErrorHandler.h:75-76
+}
+inline LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &Msg,
+                                          const Twine &Origin = "") {
+  errorHandler().fatal(Msg, Origin);
----------------
ruiu wrote:
> I don't like a function with a default argument as it is too easy to mess up. People casually add additional parameters to existing functions, and the result would be a function that has tons of parameters. Please avoid doing this if possible.
I left a a default argument so that existing error() and warn() calls needn't be changed. The message output from existing single parameter calls will therefore not be altered by the addition of the VS compatibility mode. I experimented with forcing a message and src parameter but this means every warn() and error() call site must have a source parameter created.


https://reviews.llvm.org/D47540