[ELF] With --vs-diagnostics, print a separate message for each location of a duplicate symbol.

[ikudrin]

Summary

We extract and print the source location in the message header so that Visual Studio is able to parse it and jump there. As duplicate symbols defined in several locations, it is more convenient to have separate error messages, which allows a user to easily access all the locations.

[ruiu]

I don't think we should change the original error message, as the original message seems to make more sense than the new one for users. Have you considered changing the regexp to parse this error message?

[ikudrin]

In D65213#1599884, @ruiu wrote:

I don't think we should change the original error message, as the original message seems to make more sense than the new one for users.

The original error message, when --vs-diagnostics is not used, is mostly the same. There is only a small improvement for a case when only one section, either for an old or new definition, is present.

On the other hand, one of the reasons for --vs-diagnostics is to provide a user with means to jump to the mentioned location. That is achieved by placing the location in front of the error message. Thus, as there are usually two source locations, there should be two separate error messages.

Have you considered changing the regexp to parse this error message?

I am afraid I might not fully understand your idea. Are you suggesting to detect this particular error message using regexps, divide it into pieces and then reconstruct two separate messages placing source locations at their beginnings?

[chrisjackson]

In D65213#1600461, @ikudrin wrote:

In D65213#1599884, @ruiu wrote:

I don't think we should change the original error message, as the original message seems to make more sense than the new one for users.

The original error message, when --vs-diagnostics is not used, is mostly the same. There is only a small improvement for a case when only one section, either for an old or new definition, is present.

On the other hand, one of the reasons for --vs-diagnostics is to provide a user with means to jump to the mentioned location. That is achieved by placing the location in front of the error message. Thus, as there are usually two source locations, there should be two separate error messages.

Have you considered changing the regexp to parse this error message?

I am afraid I might not fully understand your idea. Are you suggesting to detect this particular error message using regexps, divide it into pieces and then reconstruct two separate messages placing source locations at their beginnings?

I don't know if what you have suggested is what @ruiu intended, But I think that your interpretation makes a lot of sense. That way most of the vs-diagnostics related implementation can remain in ErrorHandler.

[ikudrin]

• Updated to use a regex to parse the original message and reconstruct two separate messages if both source locations are known.

@ruiu, is that what you meant?

[grimar]

Common/ErrorHandler.cpp

201

I'd inline msgStr.

205

BTW, this might print only the first error if errorLimit is reached.
(I am not sure how much it is critical though).

[ikudrin]

Common/ErrorHandler.cpp

201

I am not sure what you meant, but please note that according to https://en.cppreference.com/w/cpp/regex/match_results,

Because std::match_results holds std::sub_matches, each of which is a pair of iterators into the original character sequence that was matched, it's undefined behavior to examine std::match_results if the original character sequence was destroyed or iterators to it were invalidated for other reasons.

[grimar]

Common/ErrorHandler.cpp

201

Ok, it wasn't obvious behavior.

[ikudrin]

Common/ErrorHandler.cpp

205

That was made in that way deliberately. It would be weird if, for example, having the error limit of 10 we produce 11 messages, no?

[grimar]

Common/ErrorHandler.cpp

205

No more wierd than having a fixed number of errors with !vsDiagnostics when error limit == 10/11 and having a different number of errors when vsDiagnostics is set.

[ikudrin]

Common/ErrorHandler.cpp

205

Well, if you insist, I'll change it.

[ikudrin]

• Count incoming error messages, not outgoing.

[grimar]

This LGTM, thanks. Lets see what Rui think.

Common/ErrorHandler.cpp

186

nit: In LLD if one of the code branches is using {}, then another should also do that:

if (errorLimit == 0 || errorCount < errorLimit) {
  printError(msg);
} else ...

(LLVM does not always use this style, but in LLD we always do in this way I think).

[ruiu]

test/ELF/vs-diagnostics-duplicate.s

12

What about this line? It seems that the second line (e.g. "defined at duplicate2.s:20") is always repeating the same location as the first line (e.g. "duplicate2.s(20: error: ...").

[ikudrin]

test/ELF/vs-diagnostics-duplicate.s

12

It is how your suggestion for --vs-diagnostics works, see D58484. This patch does nothing with that decision. It merely makes it possible to jump to the second location in VS IDE.

[ruiu]

Common/ErrorHandler.cpp

187

Why did you have to change this line?

[ikudrin]

Common/ErrorHandler.cpp

187

I did not have to. It is a possible simplification, as we now have a separate method printErrorMsg.

[ikudrin]

• Restored curly brackets;
• Restored the branch which prints errorLimitExceededMsg.

[ruiu]

LGTM

Common/ErrorHandler.cpp

187

Ah, OK, I got confused a little because previously there was a call of printError(msg) and printErrorMsg(...), so it looked very similar.

Common/ErrorHandler.cpp

void ErrorHandler::warn(const Twine &msg) {
   }
 
   std::lock_guard<std::mutex> lock(mu);
   newline(errorOS, msg);
   printHeader("warning: ", raw_ostream::MAGENTA, msg);
   *errorOS << msg << "\n";
 }
 
-void ErrorHandler::error(const Twine &msg) {
-  std::lock_guard<std::mutex> lock(mu);
-
-  if (errorLimit == 0 || errorCount < errorLimit) {
-    newline(errorOS, msg);
-    printHeader("error: ", raw_ostream::RED, msg);
-    *errorOS << msg << "\n";
-  } else if (errorCount == errorLimit) {
-    newline(errorOS, msg);
-    printHeader("error: ", raw_ostream::RED, msg);
-    *errorOS << errorLimitExceededMsg << "\n";
+void ErrorHandler::printErrorMsg(const Twine &msg) {
+}
+
+void ErrorHandler::printError(const Twine &msg) {
+  if (vsDiagnostics) {
+    static std::regex reDuplicateSymbol(
+        R"(^(duplicate symbol: .*))"
+        R"((\n>>> defined at \S+:\d+\n>>>.*))"
+        R"((\n>>> defined at \S+:\d+\n>>>.*))");
+    std::string msgStr = msg.str();
+    std::smatch match;
+    if (std::regex_match(msgStr, match, reDuplicateSymbol)) {
+      printErrorMsg(match.str(1) + match.str(2));
+      printErrorMsg(match.str(1) + match.str(3));
+      return;
+    }
+  }
+  printErrorMsg(msg);
+}
+
+void ErrorHandler::error(const Twine &msg) {
+  std::lock_guard<std::mutex> lock(mu);
+
+  if (errorLimit == 0 || errorCount < errorLimit)
+    printError(msg);
+  else if (errorCount == errorLimit) {
+    printErrorMsg(errorLimitExceededMsg);
     if (exitEarly)
       exitLld(1);
   }
 
   ++errorCount;
 }
 
 void ErrorHandler::fatal(const Twine &msg) {
   error(msg);
   exitLld(1);
 }

include/lld/Common/ErrorHandler.h

public:
   void log(const Twine &msg);
   void message(const Twine &msg);
   void warn(const Twine &msg);
 
   std::unique_ptr<llvm::FileOutputBuffer> outputBuffer;
 
 private:
   void printHeader(StringRef s, raw_ostream::Colors c, const Twine &msg);
+  void printErrorMsg(const Twine &msg);
+  void printError(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/vs-diagnostics-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-diagnostics-duplicate2.s -o %t2.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/vs-diagnostics-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{{.+}})
+// CHECK:      duplicate2.s(20): error: duplicate symbol: bar
+// CHECK-NEXT: >>> defined at duplicate2.s:20
+// CHECK-NEXT: >>>{{.*}}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