This is an archive of the discontinued LLVM Phabricator instance.

[LLD] Enable Visual Studio compatible diagnostics.
AbandonedPublic

Authored by ikudrin on Aug 10 2018, 6:42 AM.

Details

Summary

This is an alternative implementation for D47540.

In this patch, I moved the code which produces the location part of a diagnostic message into a distinct place. I hope that this makes the overall code clearer because it simplifies other parts of the codebase.

The patch is primarily intended to demonstrate the approach and collect the feedback, which way we are going to go. The code is far from being perfect and has to be polished if this approach will be chosen.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ikudrin created this revision.Aug 10 2018, 6:42 AM
aganea added a subscriber: aganea.Aug 15 2018, 11:39 AM

@ikudrin

I have experimented with this implementation and I agree that separating the diagnostic location functions into a distinct library is a good structured approach. I think this would be a reasonable and more explicit support for Visual Studio Diagnostics than D47540 which really just shoehorned in support.

arichardson added inline comments.Sep 7 2018, 6:28 AM
ELF/Relocations.cpp
636

This would have been useful for me too when I added new error messages to our fork of LLD. I just ended up copy-pasting the string constants from other files in lld.
I like this since it will ensure consistent warning message formats everywhere.

@ruiu
Hello Rui, do you have any thoughts on this implementation compared to D47540 ?

ruiu added a comment.Sep 19 2018, 2:08 PM

I haven't read Diagnostics.cpp to focus on the ABI itself, but it looks like this approach is better than https://reviews.llvm.org/D47540, though this patch seems a bit too abstracted and too object-oriented to my taste. Can you add comment to the code? Also, please avoid operator overriding and eliminate Diag class (you can do that because all you need is a boolean flag indicating whether VS output is wanted or not.)

@chrisjackson @arichardson @ruiu, thank you for the feedback! I am going to continue working on the patch somewhere in October. I would really appreciate any additional comments meanwhile.

@ruiu, if not overloading operators, what style of combining messages would you prefer?

ruiu added a comment.Sep 20 2018, 9:31 AM

Operator overriding can always be rewritten as function calls, so instead of operator+(X), you can always define something like add(X)?

With overloaded operators we can write something like

error("relocation " + toString(Type) +
        " cannot refer to absolute symbol: " + toString(Sym) +
        definedAt(toObjLoc(Sym.File)) +
        referencedBy(S.getSymLoc(Sym, RelOff)));

How would you prefer it to look like?
I don't insist on the current style, but I believe it is important to be discussed prior to rewriting the code because the decision will influence the design directly.

Ping! Any progress planned on this?

I will provide a patch for this as I'm very keen to add support for Visual Studio diagnostics, but I'm not currently working on it.

ikudrin abandoned this revision.Mar 18 2019, 11:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2019, 11:41 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript