Page MenuHomePhabricator

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

Authored by ikudrin on Jul 24 2019, 7:45 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin created this revision.Jul 24 2019, 7:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
ruiu added a comment.Jul 24 2019, 1:01 PM

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?

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 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 this revision to Diff 211929.Jul 26 2019, 6:29 AM
  • 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 added inline comments.Jul 26 2019, 11:18 PM
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 marked an inline comment as done.Jul 28 2019, 2:09 AM
ikudrin added inline comments.
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 added inline comments.Jul 29 2019, 12:35 AM
Common/ErrorHandler.cpp
182 ↗(On Diff #211929)

Ok, it wasn't obvious behavior.

ikudrin marked an inline comment as done.Jul 30 2019, 4:44 AM
ikudrin added inline comments.
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 added inline comments.Jul 31 2019, 12:46 AM
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 marked an inline comment as done.Jul 31 2019, 2:19 AM
ikudrin added inline comments.
Common/ErrorHandler.cpp
186 ↗(On Diff #211929)

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

ikudrin updated this revision to Diff 212550.Jul 31 2019, 4:23 AM
  • Count incoming error messages, not outgoing.
grimar accepted this revision.Jul 31 2019, 4:37 AM

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).

This revision is now accepted and ready to land.Jul 31 2019, 4:37 AM
MaskRay accepted this revision.Jul 31 2019, 4:57 AM
ruiu added inline comments.Jul 31 2019, 6:20 PM
test/ELF/vs-diagnostics-duplicate.s
12 ↗(On Diff #212550)

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 marked an inline comment as done.Jul 31 2019, 9:16 PM
ikudrin added inline comments.
test/ELF/vs-diagnostics-duplicate.s
12 ↗(On Diff #212550)

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 added inline comments.Jul 31 2019, 9:28 PM
Common/ErrorHandler.cpp
187 ↗(On Diff #212550)

Why did you have to change this line?

ikudrin marked an inline comment as done.Jul 31 2019, 9:38 PM
ikudrin added inline comments.
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 updated this revision to Diff 212732.Jul 31 2019, 11:12 PM
  • Restored curly brackets;
  • Restored the branch which prints errorLimitExceededMsg.
ruiu accepted this revision.Jul 31 2019, 11:18 PM

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.

This revision was automatically updated to reflect the committed changes.