Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Clang 12 and resolve forward-compatibility issue with MSVC STL #176

Closed
PathogenDavid opened this issue Mar 26, 2021 · 6 comments
Labels
Area-Translation Issues concerning the translation from libclang into Biohazrd Concept-CppFeatures Issues concerning unsupported C++ features

Comments

@PathogenDavid
Copy link
Member

The MSVC STL is making Clang 11 a requirement, seemingly starting with Visual Studio 16.10 as indicated by this new error:

microsoft/STL@58160d5#diff-bdd45474f9376628362c583aa24d827009c50f6c19e3148b225937224338a444

Looking around in this file, it seems to me that the purpose of this file is to detect what C++ features are supported by the compiler. In the case of this specific error check, it'd indicating the earliest version of Clang which supports the features needed by this version of the STL.

It might also be ideal to look into using a more appropriate version of the STL if possible. Clang's STL locating logic is fairly naïve, it'd be nice if we didn't get broken by the user having a newer version of the STL installed.

I believe the versions of the STL that get installed are tied to the build tools you have selected in the Visual Studio Installer. We can detect the versions of the STL associated with various MSVC tool versions by looking at Path/To/VisualStudio/VC/Auxiliary/Build/Microsoft.VCToolsVersion.vXYZ.default.txt.

When multiple minor versions of the same toolset are installed, this is the newest version. Older versions can be found in a subfolder for the version. For example, with the following MSVC v142 versions installed:

image

You get the following versions of the STL available in Path/To/VisualStudio/VC/Tools/MSVC:

  • 14.20.27508
  • 14.23.28105
  • 14.29.29917

Within Path/To/VisualStudio/VC/Auxiliary/Build/ you'll have the following files pointing to the specific versions:

File Version
14.20/Microsoft.VCToolsVersion.14.20.txt 14.20.27508
14.23/Microsoft.VCToolsVersion.14.23.txt 14.23.28105
Microsoft.VCToolsVersion.v142.default.txt 14.29.29917
Microsoft.VCToolsVersion.default.txt 14.29.29917

We might also be able to restrict things based on the installed components for a given Visual Studio instance.

@PathogenDavid PathogenDavid added Concept-CppFeatures Issues concerning unsupported C++ features Area-Translation Issues concerning the translation from libclang into Biohazrd labels Mar 26, 2021
@PathogenDavid
Copy link
Member Author

PathogenDavid commented Jul 4, 2021

According to GitHub's documentation, only the latest a very recent one specific version of MSVC is installed in addition to whatever the C++ workload adds by default. Currently Microsoft.VisualStudio.Component.VC.14.25.x86.x64

It looks like I've been pinning to 14.28.29910 locally, so we could potentially just use 14.25.

They also install Microsoft.VisualStudio.Component.VC.140, but that's literally the MSVC from Visual Studio 2015.

@PathogenDavid
Copy link
Member Author

I created a test GitHub Action to list all the versions of MSVC which are installed on the runners. PathogenPlayground/GitHubActionsTest@7429e60 and it found the following:

====== Visual Studio 2019 Enterprise found in C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise ======
    MSVC 14.16.27023
    MSVC 14.25.28610
    MSVC 14.29.30037

14.29.30037 is too new and has the issue. I believe 14.28.29910 is the last version released that has Clang 10 compatibility.

14.25.28610 is definitely on the older side, but I think it should probably be new enough.

@PathogenDavid
Copy link
Member Author

As a workaround I'm just hard-coding VCToolsInstallDir: 26f311b

14.25 is good enough for CI purposes (and probably even using purposes.) It was specifically added to the GitHub runners in actions/runner-images#1076 for solve issues similar to ours, so I don't see it going away any time soon. (It's not like this is some latest LTS version or something.)

I think by the time it goes away we'll probably have moved to Clang 11 or to a more formalized system for selecting the MSVC version using the installer API. If it does go away, it sounds like installing it during CI is reasonably fast. Installing an additional version of MSVC takes up about 1 GB, not sure how big the actual download is.

@PathogenDavid
Copy link
Member Author

PathogenDavid commented Aug 29, 2021

Going straight to Clang 12. Bad timing on our part because it looks like Clang 13 is coming out soonish, but oh well. Here's how I got there:

  • Rebase the Pathogen LLVM extensions: git rebase --onto llvmorg-12.0.1 llvmorg-10.0.0 pathogen-main --interactive
    • Edit: You need to rebase because LLVM branches aren't meant to (and don't) merge cleanly.
    • Fixed minor merge conflicts in CMakeLists.txt (Next time I might rewrite the affected changes in a way to avoid the merge conflict entirely.)
      • One of the weirder ones that came up was the commit where we add clangCodeGen. Upstream removed LLVMSupport, it's not totally clear whether we were relying on this so I'll remove it too and see.
    • Had to fix barf around MochiLibraries/llvm-project@4c78573 due to an autocrlf-related error. (I know I should have autocrlf disabled, I've had it on since it was still a default or something and turning it off is a huge pain.)
      • Disable autocrlf
      • Delete everything
      • Hard reset
      • Continue rebase
      • Unfortunately a side-effect of this is a bunch of bogus merge conflicts in README.md for some reason. (I think it accidentally had mixed line endings?)
      • Re-enable autocrlf and put off disabling it for real some other day.
  • Update libClangSharp
  • Update Pathogen Extensions
    • Add explicit cast of StringRef -> std::string (It was made explicit in llvm/llvm-project@777180a to emphasize that it's expensive.)
    • Add support for IndirectAliased argument kind.
  • Update Biohazrd
    • Cursor.CursorParent was replaced with Cursor.SemanticParentCursor/Cursor.LexicalParentCursor
      • I believe LexicalParentCursor is what CursorParent basically was (the implementation is very different), but SemanticParentCursor seems more accurate to the intent in the handful of places it is used so I changed things to that.
    • IntegerLiteral.Value was renamed to IntegerLiteral.ValueString

I ran tests locally on Windows and everything is passing. Currently waiting on ClangSharp.Pathogen CI so I can test Linux CI and with actual generators.

@PathogenDavid PathogenDavid changed the title Upgrade to Clang 11 and resolve forward-compatibility issue with MSVC STL Upgrade to Clang 12 and resolve forward-compatibility issue with MSVC STL Aug 29, 2021
@PathogenDavid
Copy link
Member Author

Something I somehow didn't notice before: You can actually disable the STL's version checks:

https://github.com/microsoft/STL/blob/58160d548f3583b3232129ea38d786ad583ca65c/stl/inc/yvals_core.h#L515

A bit late now since we're moving to Clang 12, but it would've been interesting to try using that. I can't imagine it'd go super well though, my understanding is they bump the required version when they start using new C++ features implemented in that specific version.

@PathogenDavid
Copy link
Member Author

Forgot to tag this issue. Biohazrd uses Clang 12.0.1 as of ad822e7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Translation Issues concerning the translation from libclang into Biohazrd Concept-CppFeatures Issues concerning unsupported C++ features
Projects
None yet
Development

No branches or pull requests

1 participant