[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

182 ↗

(On Diff #211929)

I'd inline msgStr.

186 ↗

(On Diff #211929)

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

182 ↗

(On Diff #211929)

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

182 ↗

(On Diff #211929)

Ok, it wasn't obvious behavior.

[ikudrin]

Common/ErrorHandler.cpp

186 ↗

(On Diff #211929)

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

186 ↗

(On Diff #211929)

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

186 ↗

(On Diff #211929)

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 ↗

(On Diff #212550)

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 ↗

(On Diff #212550)

Why did you have to change this line?

[ikudrin]

Common/ErrorHandler.cpp

187 ↗

(On Diff #212550)

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 ↗

(On Diff #212550)

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

ELF/Symbols.cpp

int Symbol::compare(const Symbol *other) const {
   return 0;
 }
 
 static void reportDuplicate(Symbol *sym, InputFile *newFile,
                             InputSectionBase *errSec, uint64_t errOffset) {
   if (config->allowMultipleDefinition)
     return;
 
+  auto makeLoc = [&](InputSectionBase *sec, uint64_t offset,
+                     InputFile *file) -> std::string {
+    if (!sec)
+      return "\n>>> defined in " + toString(file);
+    std::string loc = "\n>>> defined at ";
+    std::string src = sec->getSrcMsg(*sym, offset);
+    if (!src.empty())
+      loc += src + "\n>>>            ";
+    loc += sec->getObjMsg(offset);
+    return loc;
+  };
+
   Defined *d = cast<Defined>(sym);
-  if (!d->section || !errSec) {
-    error("duplicate symbol: " + toString(*sym) + "\n>>> defined in " +
-          toString(sym->file) + "\n>>> defined in " + toString(newFile));
-    return;
-  }
+  InputSectionBase *sec = cast_or_null<InputSectionBase>(d->section);
+  std::string msg = "duplicate symbol: " + toString(*sym);
 
+  if (errorHandler().vsDiagnostics) {
+    // Print separate error messages for each source location so that both of
+    // them can be extracted and printed in the corresponding message header.
+    error(msg + makeLoc(sec, d->value, sym->file));
+    error(msg + makeLoc(errSec, errOffset, newFile));
+  } else {
     // Construct and print an error message in the form of:
     //
     //   ld.lld: error: duplicate symbol: foo
     //   >>> defined at bar.c:30
     //   >>>            bar.o (/home/alice/src/bar.o)
     //   >>> defined at baz.c:563
     //   >>>            baz.o in archive libbaz.a
-  auto *sec1 = cast<InputSectionBase>(d->section);
-  std::string src1 = sec1->getSrcMsg(*sym, d->value);
-  std::string obj1 = sec1->getObjMsg(d->value);
-  std::string src2 = errSec->getSrcMsg(*sym, errOffset);
-  std::string obj2 = errSec->getObjMsg(errOffset);
-
-  std::string msg = "duplicate symbol: " + toString(*sym) + "\n>>> defined at ";
-  if (!src1.empty())
-    msg += src1 + "\n>>>            ";
-  msg += obj1 + "\n>>> defined at ";
-  if (!src2.empty())
-    msg += src2 + "\n>>>            ";
-  msg += obj2;
-  error(msg);
+    error(msg + makeLoc(sec, d->value, sym->file) +
+          makeLoc(errSec, errOffset, newFile));
+  }
 }
 
 void Symbol::resolveCommon(const CommonSymbol &other) {
   int cmp = compare(&other);
   if (cmp < 0)
     return;
 
   if (cmp > 0) {

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:      {{.*}}ld.lld{{.*}}: error: duplicate symbol: foo
 // 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:      {{.*}}ld.lld{{.*}}: error: duplicate symbol: baz
 // CHECK-NEXT: >>> defined at duplicate3.s
 // CHECK-NEXT: >>>            {{.*}}3.o:(.text+0x{{.+}})
 
 .global _start, foo, bar, baz
 .text
 _start:
   nop