[DO NOT SUBMIT] Add -vs-diagnostics.

[chrisjackson]

Summary

This patch adds -vs-diagnostics flag to lld so that the linker prints
out error messages in the format that is friendly to Microsoft Visual Studio.

Since the existing error message format of lld and MSVC are significantly
different, and in most cases we print out more information than MSVC,
it is not practical nor beneficial to make lld to print out the same error
messages as MSVC does.

It looks like the most annoying issue if you are using lld on MSVC is that
the IDE automatically makes the first word of the first line of a linker
error message a hypertext, expecting that the word is an error location.
We print out "ld.lld" there by default, so when you click that string on
the IDE, it opens the linker binary file -- which is not useful at all.
In this patch, I'm focusing on fixing that issue.

This patch adds a function to find an error location in an error message.
If -vs-diagnostics is given, lld calls that function and prints out its
return value instead of "ld.lld". That operation is purely textual; I
used regexps to recognize some error message patterns to extract an error
location.

An alternative approach would have been abstracting error message
construction so that we can construct both the existing and MSVC-style
error messages.

Doing everything over a text might seems too primitive compared to adding
an abstraction layer, but I think the former is overall a better approach
than the latter. It is much less intrusive and very easy to understand.
And if you think of it, this is similar to what IDEs do to recognize and
highlight compiler and linker error messages. They do recognize some
patterns because linkers print out somewhat structured error messages.
If IDEs can recognize it, so do we. If our error message is machine-
unfriendly, we should change the error message.

This is a counter-proposal to https://reviews.llvm.org/D58300. This is
a proof-of-concept to share the idea. I think I need to add a few more
regexes to make it usable.

[ikudrin]

For me, the code should be, above all, reliable. You can trust it only in that case; otherwise, you never know, when and how it is going to be broken and produce an unexpected result.

I would suggest a simple test for both approaches. What if a developer adds a new diagnostic message?

With D58300 it will probably just work as expected in both modes out-of-the-box. You even don't need to check that it is properly formatted, because most of the formatting is performed in one place, uniformly.

With this patch (D58484) the developer might not be aware of the existence of the --vs-diagnostics switch. Thus, recognizing patterns will not probably be updated and the message generation will be broken for an unpredictable time until someone comes across that new message in Visual Studio.

To sum up, I consider this patch to be very fragile. It would be hard for me to welcome it.

I have also noticed the following differences in the generated messages compared to D58300:

• The line number is not enclosed in parentheses, how it should.
• The patch is unable to generate separate messages for each duplicate symbol, which D58300 does.
• The patch sometimes uses unsuitable strings for the source location, for example, an offset in a section.
• In some cases it cannot recognize the source location to be used in the front of the message.

You may see all these issues with tests from D58300. It looks like trying to fixing them will complicate this patch and make it even more fragile.

I believe that D58300 not only adds VS-compatible diagnostic messages but makes a step towards the better code quality. It separates message formatting in a distinct place, making the other code cleaner. It also adds a possibility to add other formatters in the future, not only for different IDEs but for any other tool.

[ruiu]

In D58484#1407078, @ikudrin wrote:

For me, the code should be, above all, reliable. You can trust it only in that case; otherwise, you never know, when and how it is going to be broken and produce an unexpected result.

I would suggest a simple test for both approaches. What if a developer adds a new diagnostic message?

With D58300 it will probably just work as expected in both modes out-of-the-box. You even don't need to check that it is properly formatted, because most of the formatting is performed in one place, uniformly.

With this patch (D58484) the developer might not be aware of the existence of the --vs-diagnostics switch. Thus, recognizing patterns will not probably be updated and the message generation will be broken for an unpredictable time until someone comes across that new message in Visual Studio.

I think I disagree. The error messages we are printing out is under our control, so we don't have to worry too much about the hypothetical scenario that "what if someone unaware of this does <something>". We know what we are printing, and we'd also like to make it machine-readable in some degree. The number of error messages added for each release is very small (I'd guess less than ten in average), and that's pretty manageable number. In addition to that, even when something goes wrong and some error message fell through this mechanism, it is not an end-of-the-world kind of thing. It is just mildly annoying if you are using an IDE and there's a link error. But I don't think even that would happen.

To sum up, I consider this patch to be very fragile. It would be hard for me to welcome it.

I have also noticed the following differences in the generated messages compared to D58300:

• The line number is not enclosed in parentheses, how it should.

It shouldn't be too hard to fix it.

• The patch is unable to generate separate messages for each duplicate symbol, which D58300 does.

I'd like to first discuss what would be expected as an error message for a duplicate symbol, as IIUC, MSVC doesn't show two separate error messages for two conflicting locations.

• The patch sometimes uses unsuitable strings for the source location, for example, an offset in a section.
• In some cases it cannot recognize the source location to be used in the front of the message.

It shouldn't be too hard to fix them, too.

You may see all these issues with tests from D58300. It looks like trying to fixing them will complicate this patch and make it even more fragile.

I believe that D58300 not only adds VS-compatible diagnostic messages but makes a step towards the better code quality. It separates message formatting in a distinct place, making the other code cleaner. It also adds a possibility to add other formatters in the future, not only for different IDEs but for any other tool.

I'd like to emphasize the cost of reading, understanding and maintaining code. Imagine that you were a first-time reader of the lld source code and looking for a way to print out an error message. The current way of doing it is very easy. It is also extremely easy to identify which line of code corresponds to a specific error message. That readability comes from the fact that we don't do too much clever things when printing out error messages. I can see that abstracting the error message construction to match exiting IDE may seem like the "right" approach, but we should be careful not to overdesign. This is perhaps a "worse is better" situation. If something very simple-minded works just enough, we should take it instead of a more complex and theoretically comprehensive one as a first approach.

[chrisjackson]

I'm working on extending this to pass the existing tests from D58300. Like @ikudrin , I am not fond of this regex method, but I think we will all be in a better place to make an evaluation once this implementation can pass the tests.

[chrisjackson]

I have expanded Rui's regex implementation to cover the existing tests for Visual Studio diagnostics compatibility patches,

[chrisjackson]

Does anyone have any comments on this expanded alternative implementation of vs-diagnostics?

[ruiu]

Apologies for the belated response. I'm happy with this patch.

Common/ErrorHandler.cpp

98

nit: add {}

ELF/Driver.cpp

761

The members of this class have the same names of the corresponding options, so VsDiagnostics.

ELF/Options.td

363

Please indent just like other definitions.

[MaskRay]

Common/ErrorHandler.cpp

89

How about using llvm::Regex instead of std::regex. I assume that may make the increased code size smaller.

[ruiu]

Common/ErrorHandler.cpp

89

I'm not sure if we still should maintain llvm::Regex as I believe it is written before C++11. Maybe we should migrate to std::regex? I'm also not worried too much about the binary size, as lld already contains entire LLVM and std::regex shouldn't be too large.

[chrisjackson]

Additional regexes to match more messages containing source:line information. Also added a test.

[chrisjackson]

Does anyone have any thoughts on these additions?

[ruiu]

LGTM

Sorry for the belated response. Variable names should be now in camelBack, so please update your code before submitting.

[rnk]

Common/ErrorHandler.cpp

89

FYI, this broke Chromium's packaging of clang: https://crbug.com/929645#c5
I think it's just time for us to upgrade to a newer GCC, so I don't think we should do anything here. But that won't stop me from shaking my fist at folks for creating work for us. =P

Common/ErrorHandler.cpp

 #include "lld/Common/Threads.h"
 
 #include "llvm/ADT/Twine.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <mutex>
+#include <regex>
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
 #endif
 
 using namespace llvm;
 using namespace lld;
 
void lld::diagnosticHandler(const DiagnosticInfo &DI) {
   }
 }
 
 void lld::checkError(Error E) {
   handleAllErrors(std::move(E),
                   [&](ErrorInfoBase &EIB) { error(EIB.message()); });
 }
 
-void ErrorHandler::print(StringRef S, raw_ostream::Colors C) {
+static std::string getLocation(std::string Msg, std::string Default) {
+  static std::vector<std::regex> Regexes{
+      std::regex(
+          R"(^can't create dynamic relocation .* against (?:local |)symbol .*\n>>> defined in .*\n>>> referenced by (\S+):(\d+))"),
+      std::regex(R"(^duplicate symbol:.*\n>>> defined at (\S+):(\d+))"),
+      std::regex(
+          R"(^undefined symbol:.*\n>>> referenced by (\S+):(\d+)\n.*)"),
+      std::regex(R"(^undefined symbol:.*\n>>> referenced by (.*):)")};
+
+  std::smatch Match;
+  for (std::regex &Re : Regexes)
+    if (std::regex_search(Msg, Match, Re)) {
+      return Match.size() > 2 ? Match.str(1) + "(" + Match.str(2) + ")"
+                              : Match.str(1);
+    }
+
+  return Default;
+}
+
+void ErrorHandler::printHeader(StringRef S, raw_ostream::Colors C,
+                               const Twine &Msg) {
+ 
+  if (VsDiag) {
+    // Visual Studio-style error message starts with an error location.
+    // If a location cannot be extracted then we default to LogName
+    *ErrorOS << getLocation(Msg.str(), LogName) << ": ";
+  } else {
     *ErrorOS << LogName << ": ";
+  }
+
   if (ColorDiagnostics) {
     ErrorOS->changeColor(C, true);
     *ErrorOS << S;
     ErrorOS->resetColor();
   } else {
     *ErrorOS << S;
   }
 }

 void ErrorHandler::warn(const Twine &Msg) {
   if (FatalWarnings) {
     error(Msg);
     return;
   }
 
   std::lock_guard<std::mutex> Lock(Mu);
   newline(ErrorOS, Msg);
-  print("warning: ", raw_ostream::MAGENTA);
+  printHeader("warning: ", raw_ostream::MAGENTA, Msg);
   *ErrorOS << Msg << "\n";
 }
 
 void ErrorHandler::error(const Twine &Msg) {
   std::lock_guard<std::mutex> Lock(Mu);
   newline(ErrorOS, Msg);
 
   if (ErrorLimit == 0 || ErrorCount < ErrorLimit) {
-    print("error: ", raw_ostream::RED);
+    printHeader("error: ", raw_ostream::RED, Msg);
     *ErrorOS << Msg << "\n";
   } else if (ErrorCount == ErrorLimit) {
-    print("error: ", raw_ostream::RED);
+    printHeader("error: ", raw_ostream::RED, Msg);
     *ErrorOS << ErrorLimitExceededMsg << "\n";
     if (ExitEarly)
       exitLld(1);
   }
 
   ++ErrorCount;
 }
 
 void ErrorHandler::fatal(const Twine &Msg) {
   error(Msg);
   exitLld(1);
 }

ELF/Driver.cpp

static void parseClangOption(StringRef Opt, const Twine &Msg) {
   error(Msg + ": " + StringRef(Err).trim());
 }
 
 // Initializes Config members by the command line options.
 void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   errorHandler().Verbose = Args.hasArg(OPT_verbose);
   errorHandler().FatalWarnings =
       Args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
+  errorHandler().VsDiag = Args.hasArg(OPT_vs_diagnostics);
   ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_no_threads, true);
 
   Config->AllowMultipleDefinition =
       Args.hasFlag(OPT_allow_multiple_definition,
                    OPT_no_allow_multiple_definition, false) ||
       hasZOption(Args, "muldefs");
   Config->AllowShlibUndefined =
       Args.hasFlag(OPT_allow_shlib_undefined, OPT_no_allow_shlib_undefined,

ELF/Options.td

defm rsp_quoting: Eq<"rsp-quoting", "Quoting style for response files">,
   MetaVarName<"[posix,windows]">;
 
 def v: Flag<["-"], "v">, HelpText<"Display the version number">;
 
 def verbose: F<"verbose">, HelpText<"Verbose mode">;
 
 def version: F<"version">, HelpText<"Display the version number and exit">;
 
+def vs_diagnostics: F<"vs-diagnostics">,
+HelpText<"Changes diagnostic output format to better match Microsoft Visual Studio">;
+
 defm version_script: Eq<"version-script", "Read a version script">;
 
 defm warn_backrefs: B<"warn-backrefs",
     "Warn about backward symbol references to fetch archive members",
     "Do not warn about backward symbol references to fetch archive members (default)">;
 
 defm warn_common: B<"warn-common",
     "Warn about duplicate common symbols",

include/lld/Common/ErrorHandler.h

public:
   uint64_t ErrorLimit = 20;
   StringRef ErrorLimitExceededMsg = "too many errors emitted, stopping now";
   StringRef LogName = "lld";
   llvm::raw_ostream *ErrorOS = &llvm::errs();
   bool ColorDiagnostics = llvm::errs().has_colors();
   bool ExitEarly = true;
   bool FatalWarnings = false;
   bool Verbose = false;
+  bool VsDiag = false;
 
   void error(const Twine &Msg);
   LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &Msg);
   void log(const Twine &Msg);
   void message(const Twine &Msg);
   void warn(const Twine &Msg);
 
   std::unique_ptr<llvm::FileOutputBuffer> OutputBuffer;
 
 private:
-  void print(StringRef S, raw_ostream::Colors C);
+  void printHeader(StringRef S, raw_ostream::Colors C, const Twine &Msg);
 };
 
 /// Returns the default error handler.
 ErrorHandler &errorHandler();
 
 inline void error(const Twine &Msg) { errorHandler().error(Msg); }
 inline LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &Msg) {
   errorHandler().fatal(Msg);

test/ELF/Inputs/vs-diag-duplicate2.s

+.global foo, bar
+
+.text
+foo:
+  nop
+
+.file 1 "duplicate2.s"
+.loc 1 20
+bar:
+  nop
+
+.section .debug_abbrev,"",@progbits
+  .byte  1                      # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte  0                      # DW_CHILDREN_no
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte  0                      # EOM(1)
+  .byte  0                      # EOM(2)
+  .byte  0                      # EOM(3)
+
+.section .debug_info,"",@progbits
+  .long .Lend0 - .Lbegin0       # Length of Unit
+.Lbegin0:
+  .short 4                      # DWARF version number
+  .long  .debug_abbrev          # Offset Into Abbrev. Section
+  .byte  8                      # Address Size (in bytes)
+  .byte  1                      # Abbrev [1] 0xb:0x1f DW_TAG_compile_unit
+  .long  .debug_line            # DW_AT_stmt_list
+.Lend0:
+  .section .debug_line,"",@progbits

test/ELF/Inputs/vs-diag-duplicate3.s

+.file "duplicate3.s"
+
+.global baz
+.text
+baz:
+  nop

test/ELF/vs-diag-duplicate.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/vs-diag-duplicate2.s -o %t2.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/vs-diag-duplicate3.s -o %t3.o
+// RUN: not ld.lld --vs-diagnostics %t1.o %t2.o %t3.o -o %tout 2>&1 | FileCheck %s
+
+// Case 1. Both symbols have full source location.
+// CHECK:      duplicate.s(15): error: duplicate symbol: bar
+// CHECK-NEXT: >>> defined at duplicate.s:15
+// CHECK-NEXT: >>>{{.*}}1.o:(.text+0x{{.+}})
+// CHECK: >>> defined at duplicate2.s:20
+// CHECK: >>>{{.*}}2.o:(.text+0x{{.+}})
+
+// Case 2. The source locations are unknown for both symbols.
+// CHECK:      {{.*}}ld.lld{{.*}}: error: duplicate symbol: foo
+// CHECK-NEXT: >>> defined at {{.*}}1.o:(.text+0x{{.+}})
+// CHECK-NEXT: >>> defined at {{.*}}2.o:(.text+0x{{.+}})
+
+// Case 3. For the second definition of `baz` we know only the source file found in a STT_FILE symbol.
+// CHECK:      duplicate.s(30): error: duplicate symbol: baz
+// CHECK-NEXT: >>> defined at duplicate.s:30
+// CHECK-NEXT: >>> {{.*}}1.o:(.text+0x{{.+}})
+// CHECK-NEXT: >>> defined at duplicate3.s
+// CHECK-NEXT: >>>            {{.*}}3.o:(.text+0x{{.+}})
+
+.global _start, foo, bar, baz
+.text
+_start:
+  nop
+
+foo:
+  nop
+
+.file 1 "duplicate.s"
+.loc 1 15
+
+bar:
+  nop
+
+.loc 1 30
+baz:
+  nop
+
+.section .debug_abbrev,"",@progbits
+  .byte  1                      # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte  0                      # DW_CHILDREN_no
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte  0                      # EOM(1)
+  .byte  0                      # EOM(2)
+  .byte  0                      # EOM(3)
+
+.section .debug_info,"",@progbits
+  .long .Lend0 - .Lbegin0       # Length of Unit
+.Lbegin0:
+  .short 4                      # DWARF version number
+  .long  .debug_abbrev          # Offset Into Abbrev. Section
+  .byte  8                      # Address Size (in bytes)
+  .byte  1                      # Abbrev [1] 0xb:0x1f DW_TAG_compile_unit
+  .long  .debug_line            # DW_AT_stmt_list
+.Lend0:
+  .section .debug_line,"",@progbits

test/ELF/vs-diag-dyn.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: not ld.lld -shared --vs-diagnostics %t.o -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK: dyn.s(15): error: can't create dynamic relocation R_X86_64_64 against local symbol in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
+// CHECK-NEXT: >>> defined in {{.*}}.o
+// CHECK-NEXT: >>> referenced by dyn.s:15
+// CHECK-NEXT: >>>{{.*}}.o:(.text+0x{{.+}})
+
+.file 1 "dyn.s"
+.loc 1 15
+
+foo:
+.quad foo
+
+.section .debug_abbrev,"",@progbits
+  .byte  1                      # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte  0                      # DW_CHILDREN_no
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte  0                      # EOM(1)
+  .byte  0                      # EOM(2)
+  .byte  0                      # EOM(3)
+
+.section .debug_info,"",@progbits
+  .long .Lend0 - .Lbegin0       # Length of Unit
+.Lbegin0:
+  .short 4                      # DWARF version number
+  .long  .debug_abbrev          # Offset Into Abbrev. Section
+  .byte  8                      # Address Size (in bytes)
+  .byte  1                      # Abbrev [1] 0xb:0x1f DW_TAG_compile_unit
+  .long  .debug_line            # DW_AT_stmt_list
+.Lend0:
+  .section .debug_line,"",@progbits

test/ELF/vs-diag-undef1.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
+// RUN: not ld.lld --vs-diagnostics %t1.o -o %tout 2>&1 \
+// RUN:   | FileCheck -check-prefix=ERR -check-prefix=CHECK %s
+// RUN: ld.lld --vs-diagnostics --warn-unresolved-symbols %t1.o -o %tout 2>&1 \
+// RUN:   | FileCheck -check-prefix=WARN -check-prefix=CHECK %s
+
+// ERR:        {{.*}}undef1.s{{.*}}: error: undefined symbol: foo
+// WARN:       {{.*}}undef1.s{{.*}}: warning: undefined symbol: foo
+// CHECK-NEXT: >>> referenced by {{.*}}1.o:(.text+0x{{.+}})
+
+.global _start, foo
+.text
+_start:
+  jmp foo
 No newline at end of file

test/ELF/vs-diag-undef2.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
+// RUN: not ld.lld --vs-diagnostics %t1.o -o %tout 2>&1 \
+// RUN:   | FileCheck -check-prefix=ERR -check-prefix=CHECK %s
+// RUN: ld.lld --vs-diagnostics --warn-unresolved-symbols %t1.o -o %tout 2>&1 \
+// RUN:   | FileCheck -check-prefix=WARN -check-prefix=CHECK %s
+
+// ERR:        {{.*}}ld.lld{{.*}}: error: undefined symbol: foo
+// WARN:       {{.*}}ld.lld{{.*}}: warning: undefined symbol: foo
+// CHECK-NEXT: >>> referenced by undef2.s
+// CHECK-NEXT: >>>               {{.*}}1.o:(.text+0x{{.+}})
+
+.file "undef2.s"
+
+.global _start, foo
+.text
+_start:
+  jmp foo

test/ELF/vs-diag-undef3.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
+// RUN: not ld.lld --vs-diagnostics %t1.o -o %tout 2>&1 \
+// RUN:   | FileCheck -check-prefix=ERR -check-prefix=CHECK %s
+// RUN: ld.lld --vs-diagnostics --warn-unresolved-symbols %t1.o -o %tout 2>&1 \
+// RUN:   | FileCheck -check-prefix=WARN -check-prefix=CHECK %s
+
+// ERR:        undef3.s(15): error: undefined symbol: foo
+// WARN:       undef3.s(15): warning: undefined symbol: foo
+// CHECK:      >>> referenced by undef3.s:15
+// CHECK-NEXT: >>> {{.*}}1.o:(.text+0x{{.+}})
+
+.file 1 "undef3.s"
+
+.global _start, foo
+.text
+_start:
+.loc 1 15
+  jmp foo
+
+.section .debug_abbrev,"",@progbits
+  .byte  1                      # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte  0                      # DW_CHILDREN_no
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte  0                      # EOM(1)
+  .byte  0                      # EOM(2)
+  .byte  0                      # EOM(3)
+
+.section .debug_info,"",@progbits
+  .long .Lend0 - .Lbegin0       # Length of Unit
+.Lbegin0:
+  .short 4                      # DWARF version number
+  .long  .debug_abbrev          # Offset Into Abbrev. Section
+  .byte  8                      # Address Size (in bytes)
+  .byte  1                      # Abbrev [1] 0xb:0x1f DW_TAG_compile_unit
+  .long  .debug_line            # DW_AT_stmt_list
+.Lend0:
+  .section .debug_line,"",@progbits