This is an archive of the discontinued LLVM Phabricator instance.

Rewrite the VS Integration Scripts
ClosedPublic

Authored by zturner on Jan 31 2018, 1:02 PM.

Details

Summary

Not sure who knows MSBuild, so I'm adding several random people in hopes that someone can review this.

I expect this to need to go through a couple of iterations, but this represents a complete re-write of the Clang Visual Studio Integration. I'll try to summarize the high level differences between how this works now and how it used to work, as well as mention some future work I think we should do:

  1. Previously we would install into %ProgramFiles%\MSBuild. At least with 2017, this no longer appears to be what you do. You now have to install to the VS instance. Rather than try to detect Installed VS Instances from a batch file, for now I just hardcode the path to the default installation path, and in follow-up patches we can port this to using a VSIX installer, which should handle all of this for us.
  1. Previously we had separate .props and .targets files for every version of VS. This turns out to be unnecessary, so all of those have been removed and now there is just 1. The good news is that this means we should continue to work in subsequent versions of VS without any additional maintenance.
  1. Previously would do a configure-style modification of the props and targets file to fill in with compile time values. The only one of these that's tricky is the LLVM Version number, the rest are already available via existing MSBuild substitutions. So all of those are fixed, and to handle the version number the installer is updated to write an additional value to the registry, which the MSBuild files pull. Decoupling these values from the particular build of LLVM opens the door for us to be able to package this in a VSIX and distribute it on the marketplace and update it independently of the actual toolchain.
  1. An xml file is provided to present a custom view of compiler options (e.g. Project > Properties > C/C++). Options we don't support or ignore are grouped together under special tabs in the UI, while the settings we do support are in the original locations. This is done because we additionally will now warn (or error) on certain options which could have been enabled in a project prior to changing the toolset, and we like to be able to point the user exactly to which setting to change to silence the warning / error.
  1. Previously we would copy clang-cl.exe to $(InstallDir)\bin\cl.exe because this is how MSBuild would find the compiler. This is now fixed so that it locates clang-cl.exe directly.
  1. Options that we ignore are now not even passed to clang-cl. This might seem unimportant because if we ignore the option, there's no harm in passing it in. But some of these options trigger additional MSBuild logic that happens before the compiler even has a chance to run. For example, in llvm.org/pr36140, a user noted that using /Zi triggered a full rebuild every time, even if nothing changed. In any case, having smaller command lines makes for easier diagnostics. An additional side benefit of this is that whereas before you would get lots of -Wunused-command-line-argument warnings, now there will be no such warnings.

Future Work (in rough order of priority):

  1. Wrap this all in a VSIX so that it can be properly installed, instead of relying on batch file hackery.
  2. Expose all of clang warnings through a custom UI page.
  3. Expose all of clang optimizations through a custom UI page.
  4. Replace link.exe with lld-link.exe similar to how we currently replace cl.exe with clang-cl.exe
  5. Install a clang platform (in additional to a platform toolset), so that we have Clang (x86) and Clang (x64) platforms for people to enable building side-by-side configurations.
  6. Add a Clang (Cross-Platform) platform that exposes the clang++ driver in all of its glory and allows the user to set the target triple.

Diff Detail

Repository
rC Clang

Event Timeline

zturner created this revision.Jan 31 2018, 1:02 PM
zturner edited the summary of this revision. (Show Details)Jan 31 2018, 1:22 PM
smeenai added a subscriber: smeenai.
hans added inline comments.Feb 1 2018, 2:57 AM
llvm/tools/msbuild/Clang.Cpp.Common.props
41 ↗(On Diff #132243)

As I mentioned before, separating the toolset config from the actual toolchain installation makes me a little nervous.

But if we're doing it, the version checks below should probably include the .1 versions too, i.e. at least 5.0.1 and 6.0.1.

49 ↗(On Diff #132243)

It's embarrassing that we didn't figure this out before :-)

With this, we can remove

if (MSVC)
   list(APPEND CLANG_LINKS_TO_CREATE ../msbuild-bin/cl)
 endif()

from Clang's tools/driver/CMakeLists.txt

llvm/tools/msbuild/LLVM.props
8 ↗(On Diff #132243)

Hmm, we previously intentionally called the toolset "LLVM" with the thinking that it would eventually include lld and designated the complete llvm toolchain, not just Clang. And is the "for Windows" part necessary?

llvm/tools/msbuild/Toolset.targets
38 ↗(On Diff #132243)

This seems to duplicate a lot of logic from clang-cl. It's nice to provide a good UI for the user, but maintaining this seems a lot of work. Are you not concerned that this will rot?

46 ↗(On Diff #132243)

But maybe we want clang-cl to map this to -flto one day. Now we need to update two places. And with the toolset/toolchain install split, the two places may be installed separately :-/

73 ↗(On Diff #132243)

Comment copy-paste-o?

83 ↗(On Diff #132243)

Keeping up with all these flags seems like a huge amount of work. Why not just let clang-cl ignore it?

hans added inline comments.Feb 1 2018, 2:57 AM
llvm/tools/msbuild/Toolset.targets
111 ↗(On Diff #132243)

Maybe it will one day. The OpenMP folks claim their runtime works on Windows and we've shipped it for some releases. It seems better to keep this logic tied to the toolchain?

128 ↗(On Diff #132243)

More stuff to keep in sync with MSVC's options and clang-cl :-/

166 ↗(On Diff #132243)

I guess we'll need to update the fork with each VS release (but still stay compatible with old versions?)

llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132243)

Maybe add a short top-level comment about what this file is.

343 ↗(On Diff #132243)

This seems like a huge amount of stuff to keep in sync with MSVC and clang-cl's flags.

llvm/tools/msbuild/install.bat
10 ↗(On Diff #132243)

Don't we want to support at least 2015 too?

20 ↗(On Diff #132243)

This should probably have quotes just to be safe.

zturner added inline comments.Feb 1 2018, 8:46 AM
llvm/tools/msbuild/Clang.Cpp.Common.props
41 ↗(On Diff #132243)

Unless we're going to release the full thing including the compiler, linker, etc through the marketplace I don't see an alternative. In any case, I actually think this it's preferable this way. There's nothing really about the two that benefits from having them coupled together, as far as I can see. I'm willing to be convinced though, if we can figure out how to to do it so that we can still ship it on the marketplace.

llvm/tools/msbuild/LLVM.props
8 ↗(On Diff #132243)

Do you think there's any value in having a toolset that does Clang+Link and a second one that does Clang+LLD? Or do you think we should stick with only a single one? I can change the name to LLVM though.

llvm/tools/msbuild/Toolset.targets
38 ↗(On Diff #132243)

I don't think it will. Maybe I'm being overly optimistic here, but the only case we would ever need to maintain this again is if we started supporting these options. Fiber Safe Optimizations, for example, I'm pretty sure we will never support. If MSVC ever removes the option, for example, we can do nothing and continue to work.

We could also just silently ignore them and just pass the option through to clang-cl, but these are pretty unusual options with pretty specialized use cases, so I feel like if I had gone out of my way to enable such a strange option I would want to know if the compiler was not going to respect it.

46 ↗(On Diff #132243)

That's even better then. All we have to do is change this xml, push a new build to the market place, and the VS UI will update their extension for them.

Note that we could do the mapping at the MSBuild level, in this file down below where we have an ItemGroup. Just add a line that says <AdditionalOptions Condition="%(ClCompile.WholeProgramOptimization)' == 'true'>-flto=thin %(AdditionalOptions)</AdditionalOptions>

and we can do this without touching clang.

83 ↗(On Diff #132243)

See the large comment at the top of the file. For some options, we could probably get by with this. Maybe even this one, I debated on this particular one.

My bar was "If the option fundamentally changes assumptions about the way code could be compiled, we should generate an error. If it changes the behavior of the language in a way we don't support, changes the way we generate code in a meaningful way, or causes specialized output files to be written, warn, and if it's an option we ignore then drop it"

The last category there we could probably just pass through in some cases, but in that comment I also mentioned a case where setting an option that clang-cl ignores impacts MSBuild's ability to figure out dependencies and ends up causing a full rebuild every time even when nothing changed.

We can scour the entire cl build tasks and try to discover if any other ones have unintended consequences, but I think it's easier to just turn them off at the MSBuild level. And as a side benefit, the user gets shorter command lines, which is always nice.

As for maintenance, this all looks like zero-maintenance code to me. Did you have an example in mind of where we'd need to update this? Whether it be a new VS version, or VS dropping support for one of these options or deprecating them, I don't think we'd have to do anything.

111 ↗(On Diff #132243)

I can put some conditions that only cause it to error if clang version is less than or equal to 7, but that seems higher maintenance because we'll probably have many new releases before that ever happens. Even if we do tie it to the toolchain, the user is still going to have an error in their code if they have /openmp on, they'll just have to spend more time figuring out that the underlying cause is we don't currently support this option. I'd rather err on the side of improving the user experience today and worrying about what we might do in the future when it happens.

Even if we do have to push a new version of the extension every so often, that's less trouble for the user than downloading a new installer because they get auto-updated through the VS Extension Manager.

128 ↗(On Diff #132243)

Mentioned before, but I dont' think we have to do anything to keep this in sync with MSVC. All of these lines should be able to remain forever, regardless of what MSVC does.

166 ↗(On Diff #132243)

I don't think it would be necessary to update every release unless they fundamentally change the way the UI looks. The logic didn't change between 2015 and 2017 for example. Unless they redesign the entire Project Settings dialog, I think we should be future proof.

llvm/tools/msbuild/clang-cl.xml
343 ↗(On Diff #132243)

This stuff almost never changes though. Most of these options have been here since at least VS 2010, which is about as far back as I can remember. The only one that wasn't here in 2015 is /diagonstics

llvm/tools/msbuild/install.bat
10 ↗(On Diff #132243)

Mentioned in the other review, but the install.bat file shouldn't really be used anymore except for during development. The VSIX supports both 2015 and 2017 (I tested it in both and confirmed it works)

hans added inline comments.Feb 1 2018, 10:48 AM
llvm/tools/msbuild/Clang.Cpp.Common.props
41 ↗(On Diff #132243)

"There's nothing really about the two that benefits from having them coupled together,"

The toolset needs to know at least where to find the toolchain and how to invoke it. If we decouple them, there needs to be an interface between them that can't change: in this case the LLVM path and version number in the registry.

But at the same time you're baking in all this logic in the toolset about how to invoke the toolchain, what flags are supported, etc. Those things are strongly dependent on the toolchain, which in this de-coupled world seems problematic. It seems like you're actually making the coupling tighter in that way, except you still want to ship the two parts separately.

Are there restrictions in the marketplace about how big a vsix can be? Because if not, I think we could just package up clang+headers+runtime into a vsix and ship the whole thing, and maybe that would be the best thing.

Or we could just stick to the current installer version and make it a little smarter about finding VS2017. Maybe instead of the batch files we write an actual program that finds the installation and copies the files.

llvm/tools/msbuild/LLVM.props
8 ↗(On Diff #132243)

The best would be to only have one, but where the user could select between the two linkers, I think.

llvm/tools/msbuild/Toolset.targets
38 ↗(On Diff #132243)

I feel pretty strongly that we should handle this clang-cl side. If a flag is not supported, either we should ignore it, or if it's something the user would want to know about us not supporting, we should warn. That's what clang-cl tries to do currently, and if there are flags we don't get right, we should fix it.

And we do move flags from the unsupported to supported category now and then, so the "only case we would ever need to maintain this again is if we started supporting these options" scenario is real.

46 ↗(On Diff #132243)

But the toolset is decoupled from the toolchain in your proposal.

Not only would we need to update both clang-cl and this file, but this file would need to handle clang-cl versions both before and after.

83 ↗(On Diff #132243)

The maintenance would come from when clang-cl changes how it handles some option, or when VS adds new options.

llvm/tools/msbuild/install.bat
10 ↗(On Diff #132243)

Hmm, but then we should delete it, or at least take it out of the installer, and we need a replacement. As it is now, if we land this, it breaks the installer for versions before 2017.

zturner updated this revision to Diff 132644.Feb 2 2018, 11:48 AM
  1. Removed the option remapping. I will be adding this back in a follow up patch. The clang-cl specific "limited options view" is still here.
  2. Removed the dependency on writing the version number to the registry. This was originally done so that we could add the clang resource dir to the include path, but clang can always figure out its own resource path anyway, so it turns out this was unnecessary. This has the nice property that it will automatically work with all past, present, and future clang versions without modification.
  3. Changed "clang" to "llvm"
  4. Merged D42769 into this patch.
  5. Remove the batch files (and for that matter all other files as well) from being installed by CMake. The vsix is now the installer, which we can publish on the marketplace.
zturner updated this revision to Diff 132645.Feb 2 2018, 11:51 AM

Don't install the batch files for real. The previous patch didn't copy them to the output directory on install, but it failed to remove the invocation of install.bat and uninstall.bat from the actual installer.

hans added inline comments.Feb 5 2018, 5:19 AM
llvm/CMakeLists.txt
234 ↗(On Diff #132645)

Now that the LLVM toolchain installer no longer includes the VS integration, is there some way we could point the user to the marketplace to install the integration, or could we include the vsix in the installer and launch it for them?

llvm/tools/msbuild/LLVM.Cpp.Common.props
1 ↗(On Diff #132645)

Do we know what other versions this works with besides VS 2017?

47 ↗(On Diff #132645)

Is there a way for the user to point to another LLVM installation? This was one of the reasons for separating the toolset from the toolchain, right?

llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I thought you were going to leave the flag logic out of this patch?

llvm/tools/msbuild/install.bat
1 ↗(On Diff #132645)

I thought you meant to delete this file? I can see that it's still useful for development, but maybe then we should rename it or put a comment on the top or something explaining what it does, since its purpose is now changing from what it was before.

llvm/tools/msbuild/license.txt
1 ↗(On Diff #132645)

Can't we use the old file? Checking in another copy seems unfortunate.

llvm/tools/msbuild/llvm.sln
1 ↗(On Diff #132645)

Should we have a cmake build target to build this vsix, similar to what we do for the clang-format one, which gets built by the "clang_format_vsix" target?

zturner added inline comments.Feb 5 2018, 8:08 AM
llvm/CMakeLists.txt
234 ↗(On Diff #132645)

Do we have a Release Notes file that we install? That could be a good place to include a link to the marketplace?

Although, we'd probably want to wait until it's actually published on the marketplace before we do anything.

llvm/tools/msbuild/LLVM.Cpp.Common.props
47 ↗(On Diff #132645)

At the moment, you can do this by changing the registry key. In subsequent patches I can add a UI setting under C/C++ > General.

llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I thought Nico said to only separate out the option remapping into another patch. I took all of that out.

llvm/tools/msbuild/license.txt
1 ↗(On Diff #132645)

I copied this one from the clang-format vs integration folder. The one at the top level llvm directory has some extra stuff at the bottom which doesn't apply to this, but I don't know if it would still work just as well even with all that extra stuff in it.

llvm/tools/msbuild/llvm.sln
1 ↗(On Diff #132645)

Yea that seems reasonable. I'll look into how that works.

hans added inline comments.Feb 5 2018, 8:22 AM
llvm/tools/msbuild/LLVM.Cpp.Common.props
47 ↗(On Diff #132645)

Is it hard to add, or can you add it to this patch? Changing the registry isn't very good UI and is also global.

llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

This file still attempts to duplicate the logic of what flags clang-cl supports, which are ignored, etc. I'd prefer if we didn't do that.

To repeat my concerns:

  1. This list of flags would need to be maintained, and I expect it will always end up being incomplete or inaccurate in some other way. The only way to keep it accurate would be if it's auto-generated from clang-cl's tablegen file. But that ties it to a specific clang-cl version, which is the next problem
  1. It ties it to a particular clang-cl version, which goes against the point of decoupling the VS integration from the toolchain. As a concrete example, your file is missing /arch:AVX512 and /arch:AVX512F, so those should be added, but only when the user is using a Clang >= r323433. Of course the user could them manually, but why should some flags be listed and some not? And maybe we could handle this with version logic, but now we're getting into *really* hairy maintenance.
llvm/tools/msbuild/license.txt
1 ↗(On Diff #132645)

Right, the clang-format one is my fault :-) It's formated to fit better in the vsix installer window. Could we make the build system copy it instead of checking in another copy? If that's not easy, never mind this is not important obviously.

zturner added inline comments.Feb 5 2018, 8:39 AM
llvm/tools/msbuild/LLVM.Cpp.Common.props
47 ↗(On Diff #132645)

I can add it to this patch, but there's also no way to do this without forking the UI file, which gets to your next comment.

llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I can move this to another patch if you want, but I have to say that presenting a clang-cl tailored UI of the options is something that I feel very strongly about. In fact, the main reason I volunteered to take on this effort is specifically because I wanted to make sure we do this.

We can get into how it would have to be maintained etc, but all of that is irrelevant IMO because I'm approaching this from the perspective of the user. As someone who has used VS pretty heavily for close to 20 years, this is absolutely a large value-add for the user.

To re-iterate though, I don't see where the perceived maintenance comes from. You're already suggesting that I do nothing, which means that the user will already have to manually add options anyway if the version of VS doesn't match up with the version of clang-cl exactly. So what I'm doing here is imperfect (as is the alternative of doing nothing), but is strictly better. I don't see how that could be perceived as anything other than a win.

Version logic isn't even that hairy of maintenance. Every time we ship a new LLVM release (which seems to be about once a year), we copy the file and make the 1 or 2 necessary modifications to the copied file.

hans added inline comments.Feb 5 2018, 9:24 AM
llvm/tools/msbuild/LLVM.Cpp.Common.props
47 ↗(On Diff #132645)

Aww :-(

llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I think a clang-cl tailored UI would be really nice, but I don't think it's sustainable to maintain this file by hand. If we want to expose clang-cl's options this way, I think we need to auto-generate it from the .td file, and that it needs to be tied to the clang-cl version whose options are being exposed.

We ship a major release twice a year, typically two more stable (x.0.1) releases per year, and in addition to that I ship snapshots about once or twice a month. If we're going to present clang-cl's options here, I think the file needs to stay in sync with clang-cl's options.

zturner added inline comments.Feb 5 2018, 10:00 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

Having it be auto-generated from the .td file is a good idea, but it's quite a bit of extra work and it may not be possible to do it in a useful way (for example, MSVC's UI displays the options in categories, which are defined just below this comment in Phab. Having the auto-generated options be correctly categorized seems difficult / impossible).

It's worth trying, and if we can do it then I agree it would be worth doing, but in the meantime I think the approach here is still strictly better than doing nothing, even if this file were never touched again (which is unlikely).

hans added inline comments.Feb 5 2018, 11:17 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

We can add categories to the flags in the .td file.

The "doing nothing" that we currently do is expose Visual Studio's choice of flags and then let clang-cl handle them. I don't think that's terribly bad.

zturner added inline comments.Feb 5 2018, 11:30 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

Sure, it's not terribly bad. But I'd like to do something better than just "not terribly bad". :)

There are other issues with auto-generating the file from the td as well. When an option takes multiple values (such as /arch:), the specific values it takes are not always present in the TD file. You mentioned /arch:AVX512 earlier, but that value is not in the td file anywhere.

That doesn't mean it's impossible, we can probably address all of the various issues to get the td file to a point where this file could be auto-generated. But I don't think that work should hold this up, as I still maintain that the solution here is strictly better from the user's perspective than the old method of doing nothing.

olgaark added inline comments.Feb 5 2018, 12:38 PM
llvm/tools/msbuild/Clang.Cpp.Common.props
49 ↗(On Diff #132243)

Yes, you can redefine CLToolExe/Path if you want to use 'cl' task, but have you looked at using 'ClangCompile' task instead (see Microsoft.Cpp.Clang.props & targets, as well as clang c2 toolset)? Would it work better for you? If not, we'll interested to know why.

llvm/tools/msbuild/LLVM.props
8 ↗(On Diff #132243)

I believe the debugging is different for LLD produced binaries, so if this is the case, I'd recommend creating separate msbuild toolsets for Clang+Link and Clang+LLD.

VS has extensibility to support different debugging and it is expected that when toolset changes the set of available debuggers can change, but not when some arbitrary property is changed.

llvm/tools/msbuild/Toolset.targets
38 ↗(On Diff #132243)

There is a way currently to extend a rule (not override it) where you can add/remove properties, but I am not sure it will make it better from maintenance perspective for clang-cl rule. You might want to use it for ConfigGeneral page though.

https://github.com/Microsoft/VSProjectSystem/commit/3e2e9c7ea2f3155de9932ab53d9ec37b6bdafa17

Also, if you decide to use ClangCompile task, clang.xml it the one which is matching it.

166 ↗(On Diff #132243)

If you don't need to remove rules added by default and want just replace some of them (like cl one with clang-cl) you can allow default rules to be added and then include yours, If several rules in the list have the same name, the last one will be used.

So if you name your rule as "CL" (instead of "clang-cl") and make sure you include it to PropertyPageSchema items after importing MS targets which add 'real' cl rule, your rule will be used instead.

zturner added inline comments.Feb 5 2018, 12:49 PM
llvm/tools/msbuild/Clang.Cpp.Common.props
49 ↗(On Diff #132243)

My understanding of the ClangCompile task is that it invokes clang.exe or clang++.exe and that it passes gcc style command line options (for example, clang.xml looks like it passes all gcc style options such as -fno-exceptions, etc.

The clang cl driver (clang-cl.exe) is designed to be a drop in replacement for cl.exe, and accepts MSVC style command line arguments. It's also the only supported way to produce native Windows exes (that's not to say you can't construct a clang++.exe command line that will do it, just that we only support it via the cl driver).

zturner added inline comments.Feb 5 2018, 12:51 PM
llvm/tools/msbuild/LLVM.props
8 ↗(On Diff #132243)

Recent versions of LLD produce PDBs that are supposed to be completely compatible with link.exe PDBs. So, debugging should not change (we don't support managed debugging and certain other things like that, but on the other hand we don't generate managed code either, so it should be fine).

Question for @olgaark. Is there any way to get a page that looks like PageTemplate="generic" (i.e. with the expanding categories, such as the General page), but as a subpage of a PageTemplate="tool" page? For example, imagine you click Properties > C/C++ > Warnings, and then when you click the warnings page, I'd like that page to look like PageTemplate="generic". Is that possible?

rnk added inline comments.Feb 5 2018, 2:27 PM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I don't think we'd get much value from table generating them. This file contains a lot of information and organization that we'd have to encode in tablegen. Every option would need its property sheet category, name, and display name, none of which we currently encode.

If we did want to automatically fill in undescribed options somehow, I'd suggest storing the property sheet information separately kind of like we currently do for the attribute documentation, and then "joining" the two files in tablegen to make this XML file. I just don't think it's worth it, though.

rnk added inline comments.Feb 5 2018, 3:05 PM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I think part of the reason why I don't like the idea of loading this all into tablegen is that editing tablegen files sucks. We have all these ad-hoc conventions for adding line breaks, but there's no editor support for them or anything. I think if they were YAML files, it would be a lot easier to add new fields to options to help us auto-generate property sheets like this or even full HTML command line reference, something we sorely lack today.

Maybe this is just dreaming, but I expect YAML would also make it easier to automatically parse enum values to options, like /arch:avx512 etc. We could do that today, but it requires hacking on and extending this janky Record abstraction that's not well understood. Or maybe it just requires effort and motivation.

zturner updated this revision to Diff 132907.Feb 5 2018, 4:22 PM

Added a UI setting that allows the user to choose a custom path to clang. It defaults to $(LLVMInstallDir)bin\clang-cl.exe. I added this to the general page of the UI. I then initialize the value of $(CLToolExe) with the value of this setting in the .targets file. And since I'm mucking with that page anyway, removed some settings that don't apply to clang-cl, surrounding managed code and App Store stuff.

Also fixed a bug where DebuggerFlavor wasn't being initialized in the .props file, the effect of which was that you couldn't debug unless you explicitly went to the debugger page and chose Local Windows Debugger.

olgaark added inline comments.Feb 5 2018, 5:16 PM
llvm/tools/msbuild/Clang.Cpp.Common.props
49 ↗(On Diff #132243)

Ok, it makes sense to use 'cl' task then

hans added inline comments.Feb 6 2018, 2:52 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

But this file is effectively duplicating the information about what flags clang-cl supports. Am I the only one that's concerned about how to maintain that, and the two getting out of sync?

Olga mentioned (https://reviews.llvm.org/D42762?id=132243#inline-375127) something about extending rules rather than overwriting them. Would that be an option for us, i.e. just exposing the regular cl options and maybe do a few tweaks where really necessary?

rnk added inline comments.Feb 6 2018, 11:43 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

I am concerned about the duplication, but I think encoding the information in this XML into TableGen is going to be really ugly. Even if we stored this info outside the tablegen, we'd need some way to "join" the property sheet data with the .td data, which effectively duplicates the list of flags, or at least the supported flags with documentation and display names, similar to the way the attribute reference docs work.

Re: Olga's comment, maybe that would work, but I suspect we won't have as much control over the organization.

zturner added inline comments.Feb 6 2018, 11:49 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

extending a rules file only works when you want to add a new category or option, not when you want to remove some options but keep others. So it's not going to save us from the duplication. Personally though I'm not at all concerned about the duplication. If we take the duplicated file as it exists in this CL, and never touch it or maintain it ever again, we would still be providing a better user experience than doing nothing and relying on whatever version happens to ship with Visual Studio.

It's extremely unlikely that we would never touch it again, however. And in the worst case scenario, if we do ultimately find out that it's too much maintenance, we can change 4 or 5 lines of xml to disable our custom rules file, push an update to the marketplace, and people will automatically get updated to the new version.

hans added inline comments.Feb 7 2018, 1:45 AM
llvm/tools/msbuild/clang-cl.xml
1 ↗(On Diff #132645)

"If we take the duplicated file as it exists in this CL, and never touch it or maintain it ever again, we would still be providing a better user experience than doing nothing and relying on whatever version happens to ship with Visual Studio."

I'm not sure I agree, but I think this has been reviewed to exhaustion. Go ahead and submit, please publish under the llvm-vs-code-extensions@google.com account, and I guess we'll see how it works out.

rnk accepted this revision.Feb 9 2018, 9:39 AM

lgtm, thanks for revamping the VS integration!

This revision is now accepted and ready to land.Feb 9 2018, 9:39 AM

FYI, Andrew Pardoe from Microsoft and I were talking and he wanted to express really strong support for getting this in and deployed. He's also offered any help that would be useful to sorting this out so there is a solid installer for VS 2017.

Anything this is waiting on (since it already got an LGTM some time ago)?

FYI, Andrew Pardoe from Microsoft and I were talking and he wanted to express really strong support for getting this in and deployed. He's also offered any help that would be useful to sorting this out so there is a solid installer for VS 2017.

Anything this is waiting on (since it already got an LGTM some time ago)?

I think the biggest reason I've been dragging my feet on this is that there was never really a strong consensus that this was the right direction, and I don't feel great about committing without consensus. I guess I was subconsciously hoping someone else would show up and chime in with their opinion. Part of the problem is that there's not many LLVM developers who are also VS users, and of the ones that actually are, most are VS users of CMake generated projects as opposed to VS users of hand-maintained projects. And that is ultimately who we are targeting with this VS integration, so it would be good to have their feedback.

But since it looks like that probably isn't gonna happen, I guess I can wait until the end of this week and see if the discussion magically picks up again. If not, I suppose I can commit as-is at the end of the week.

Any updates here?

zturner updated this revision to Diff 155494.Jul 13 2018, 2:34 PM
zturner added a reviewer: dtarditi.

I've got a new version of the VS integration here. Below I'll outline the changes from the previous iteration. Most of this is based on feedback from David Tarditi (who I'm now adding as a reviewer) and Hans. But I also added something new that was my own idea.

  1. We no longer present a custom UI of the options. We do not change the layout of any builtin Microsoft UI page in any way.
  1. We now silently discard certain command line options before passing them to clang-cl. We only do this for options that generate -Wunused-command-line warnings and for which we never plan to support the option anyway (for example Fiber Safe Optimizations).
  1. We hard-error on options that fundamentally change the assumptions about the way in which code should be compiled. For example, /clr. These types of options would result in an error anyway because clang wouldn't understand the code, but at least now the user has a more meaningful error message.
  1. We remap /ZI and /Zi to /Z7. See the comment in LLVM.Cpp.Common.Targets for a longer explanation.
  1. Previously we let the user specify the Path to clang-cl.exe in the General page. We did this by forking the general page and adding a field to it. While this is arguably a "cleaner" user experience, it's more maintenance for us (and requires us to change a builtin UI page, which point #1 explains that we are no longer doing), and it may not continue to be a cleaner experience if we ever want to add more than just 1 field here (for example, path to lld-link.exe if/when we update the extension to support LLD). So I made a separate page called LLVM. This page appears whenever the LLVM Platform Toolset is selected. For now it only contains one option (path to clang-cl.exe), but in the future it could contain other options such as "Use clang-cl (True/False)", "Path to clang-cl.exe", "Use LLD", "Path to lld-link.exe", and maybe some other ones we could come up with.

Hopefully this is a nice middle ground and addresses all of the issues we were discussing earlier in the thread.

smeenai added inline comments.Jul 15 2018, 11:05 PM
llvm/tools/msbuild/LLVM.Cpp.Common.props
6 ↗(On Diff #155494)

I think this should say LLVM.Cpp.Common.props instead of Clang.Cpp.Common.props?

42 ↗(On Diff #155494)

Is there a way to add some sort of error here if LLVMInstallDir is still empty? Is it worth it?

72 ↗(On Diff #155494)

Not that it really matters right now, but this wouldn't do the right thing for arm or arm64, right?

llvm/tools/msbuild/LLVM.Cpp.Common.targets
56 ↗(On Diff #155494)

If users want to use LTO with clang-cl + lld-link, are they just expected to set the appropriate compiler and linker options manually?

85 ↗(On Diff #155494)

This comment is swapped...

90 ↗(On Diff #155494)

...with this.

118 ↗(On Diff #155494)

I think it's more accurate to describe this as not supporting C++/CX, since clang does support WinRT using straight up COM.

zturner updated this revision to Diff 155703.Jul 16 2018, 9:21 AM

Fixed issues pointed out by @smeenai

zturner added inline comments.Jul 16 2018, 9:28 AM
llvm/tools/msbuild/LLVM.Cpp.Common.props
42 ↗(On Diff #155494)

I'm not sure it's worth it. We could maybe warn when the project is loaded if the path is invalid, but I don't think we should do anything special for $(LLVMInstallDir) specifically, because ultimately the user can just change this to something else, so having a valid install dir is not even necessary. So for a user that never wants to install LLVM and just set the path to clang, they might get annoyed by constantly seeing a warning which they don't care about. If they do try to use it, they'll get an error pretty quickly along the lines of Unable to find path file \bin\clang-cl.exe so hopefully that's a sufficient clue? I could probably try to catch this in BeforeClCompile and print a better error message if you think the standard error message isn't clear enough.

72 ↗(On Diff #155494)

Right, we'd need something else here for arm.

llvm/tools/msbuild/LLVM.Cpp.Common.targets
56 ↗(On Diff #155494)

For now yes. Even though they are conceptually the same thing, they are incompatible with each other from an implementation standpoint. So, for example, you couldn't use MSVC WPO object files with lld-link, for example, or vice versa. This VS integration right now, for example, doesn't even add support for using lld-link (only clang-cl), so use of this option right now would break the build, as link.exe would be trying to link llvm bitcode files. So I think it makes sense to (eventually) provide a separate UI option for LLVM LTO, perhaps under the new LLVM UI page.

ormris added a subscriber: ormris.Jul 16 2018, 10:00 AM
zturner updated this revision to Diff 156387.Jul 19 2018, 4:46 PM

Deleted clang-cl.xml. We no longer modify the existing settings UI so this file was stale. Also fixed the csproj to properly refer to llvm-general.xml.

rnk accepted this revision.Jul 19 2018, 4:58 PM

I think that addresses @hans's concerns of duplicating all our options, so let's go ahead and land this.

This revision was automatically updated to reflect the committed changes.
hans added a comment.Jul 31 2018, 6:31 AM

Thanks for pushing this through!

Will you upload the vsix to the marketplace? (Use the llvm-vs-code-extensions@google.com account, which despite the name is also used for the clang-format vs plugin etc.)

Then I can update the docs to point at it.