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

[MSVC] Root is failed with error G694476FC: static_assert failed "Unexpected size" #15321

Closed
1 task done
NEIL-smtg opened this issue Apr 24, 2024 · 14 comments
Closed
1 task done

Comments

@NEIL-smtg
Copy link

NEIL-smtg commented Apr 24, 2024

Check duplicate issues.

  • Checked for duplicates

Description

We've been using MSVC to build ROOT, and we've recently encountered the following error:

error G694476FC: static_assert failed "Unexpected size"

Our developer has suggested the compiler that was used needs to implement the Defect Report https://cplusplus.github.io/CWG/issues/2518.html (which has been implemented by MSVC and Clang).

Reproducer

Repro step:

  1. Open "x64 Native Tools Command Prompt for VS 2022"
  2. git clone https://github.com/root-project/root.git
  3. mkdir root\build_amd64
  4. cd /d root\build_amd64
  5. set VSCMD_SKIP_SENDTELEMETRY=1 & "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\Tools\VsDevCmd.bat" -host_arch=amd64 -arch=amd64
  6. python.exe -m pip install pytest 2>&1
  7. cmake -G "Visual Studio 17 2022" -A x64 -DCMAKE_SYSTEM_VERSION=10.0.22621.0 -Dtesting=ON -Droottest=ON -Droofit=off .. 2>&1
  8. msbuild /m /p:Platform=x64 /p:Configuration=Release ALL_BUILD.vcxproj /t:Rebuild 2>&1

Expected result:

Build succesfully.

ROOT version

We are using the latest commit.

Installation method

git

Operating system

Windows server 2022 data center

Additional context

Detailed log:
Build.log

@bellenot
Copy link
Member

bellenot commented Apr 25, 2024

So I cannot reproduce the error with the recipe you give. But looking at you log file, I see you set more environment variables. That is:

set _CL_= /Bcapture_repro C:\a\_work\_temp\rwc_project_logs\ROOT\preprocessed_repro_build & set _LINK_= /onfailrepro:C:\a\_work\_temp\rwc_project_logs\ROOT\link_repro_build

Without those variables the build works just fine. This is maybe related to #9445
I'm now trying with the same variables to see if it fails the same

@bellenot
Copy link
Member

So I tried with:

cmake -G "Visual Studio 17 2022" -A x64 -Dtesting=ON -Droottest=ON -Droofit=off ..\..\..\git\master
set VSCMD_SKIP_SENDTELEMETRY=1 & "C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\Tools\VsDevCmd.bat" -host_arch=amd64 -arch=amd64 & set _CL_= /Bcapture_repro C:\project_logs\ROOT\preprocessed_repro_build & set _LINK_= /onfailrepro:C:\project_logs\ROOT\link_repro_build
msbuild /m /p:Platform=x64 /p:Configuration=Release ALL_BUILD.vcxproj /t:Rebuild

And it worked...

@StephanTLavavej
Copy link

Neil omitted an important bit of info - this involves a very recent change to MSVC's STL, microsoft/STL#4591 . To reproduce it, you'll either have to build and consume our open-source STL, or wait for VS 2022 17.11 Preview 2 to ship (which will contain this commit). We changed the STL to say static_assert(false) instead of static_assert(_Always_false<SomeType>).

@bellenot
Copy link
Member

@StephanTLavavej OK, thanks for the info. But then what does that mean for ROOT? It will not build anymore with the upcoming version 17.11 of Visual Studio (again!!) ? What would be required to fix the issue on our side?

@StephanTLavavej
Copy link

Correct. To fix this, implement CWG-2518 in Cling (if that's the tool that's emitting this error), allowing static_assert(false) to work in templates. MSVC and Clang have already implemented this Core DR.

@pcanal
Copy link
Member

pcanal commented Apr 26, 2024

Clang have already implemented this Core DR.

Do you happen to know which version this appeared in? (Or better yet which Merge Request added it). Cling uses Clang underneath; currently version 16; Moving the Clang version used is a long process but backporting a narrow Merge Request might be possible.

@StephanTLavavej
Copy link

It shipped in Clang 17, which is the version that microsoft/STL currently requires. https://clang.llvm.org/cxx_dr_status.html documents this.

I believe that llvm/llvm-project@00e2098 implemented it, but there's a slight chance there were followup commits that I'm unaware of.

@bellenot
Copy link
Member

OK, thanks a lot for the information! And since it's in Clang/Cling, let's add @vgvassilev in the loop...

@vgvassilev
Copy link
Member

We probably should backport this commit. @hahnjo what do you think?

@hahnjo
Copy link
Member

hahnjo commented Apr 29, 2024

Probably, but I'm a bit confused about the error message: Why would Cling output MSVC style error messages?

@StephanTLavavej
Copy link

I don't actually know what the G694476FC error code is (the only codes I recognize are MSVC Cnnnn, IntelliSense Ennnn, and Clang named warnings/errors), but the "Unexpected size" is coming from the STL. We have static_assert(false, "Unexpected size") in a template, and after the DR resolution, we expect that this should be activated only when that if constexpr branch is actually taken.

hahnjo pushed a commit to hahnjo/root that referenced this issue May 7, 2024
This allows `static_assert(false)` to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

* test/SemaTemplate/instantiation-dependence.cpp
* test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to `static_assert`
still allow that sort of tests to be expressed.

Reviewed By: #clang-language-wg, erichkeane, aaron.ballman

Differential Revision: https://reviews.llvm.org/D144285

---

Fixes the build with newer versions of MSVC's STL, reported as
root-project#15321
@hahnjo
Copy link
Member

hahnjo commented May 7, 2024

I believe that llvm/llvm-project@00e2098 implemented it, but there's a slight chance there were followup commits that I'm unaware of.

Seems reasonable and applies cleanly to our LLVM 16 - I opened #15437 with the backport. I will wait for confirmation that this is all we need before syncing to our LLVM monorepo fork and eventually merging.

@bellenot
Copy link
Member

bellenot commented May 7, 2024

@StephanTLavavej Thanks a lot for your help! Unfortunately, I can only try with VS 17.10 preview 6 (the only one available for now), and hence cannot try to reproduce the issue and see if the PR properly fixes it. I will try again as soon as v17.11 preview is available

@hahnjo hahnjo self-assigned this Jun 4, 2024
@hahnjo hahnjo added the in:Cling label Jun 4, 2024
hahnjo pushed a commit to hahnjo/root that referenced this issue Jun 4, 2024
This allows `static_assert(false)` to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

* test/SemaTemplate/instantiation-dependence.cpp
* test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to `static_assert`
still allow that sort of tests to be expressed.

Reviewed By: #clang-language-wg, erichkeane, aaron.ballman

Differential Revision: https://reviews.llvm.org/D144285

---

Fixes the build with newer versions of MSVC's STL, reported as
root-project#15321
hahnjo pushed a commit that referenced this issue Jun 5, 2024
This allows `static_assert(false)` to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

* test/SemaTemplate/instantiation-dependence.cpp
* test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to `static_assert`
still allow that sort of tests to be expressed.

Reviewed By: #clang-language-wg, erichkeane, aaron.ballman

Differential Revision: https://reviews.llvm.org/D144285

---

Fixes the build with newer versions of MSVC's STL, reported as
#15321
hahnjo pushed a commit to hahnjo/root that referenced this issue Jun 5, 2024
This allows `static_assert(false)` to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

* test/SemaTemplate/instantiation-dependence.cpp
* test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to `static_assert`
still allow that sort of tests to be expressed.

Reviewed By: #clang-language-wg, erichkeane, aaron.ballman

Differential Revision: https://reviews.llvm.org/D144285

---

Fixes the build with newer versions of MSVC's STL, reported as
root-project#15321

(cherry picked from commit c767271)
@hahnjo
Copy link
Member

hahnjo commented Jun 5, 2024

Fixed by #15437, backport to v6.32 via #15753

@hahnjo hahnjo closed this as completed Jun 5, 2024
@hahnjo hahnjo added this to Issues in Fixed in 6.34.00 via automation Jun 5, 2024
@hahnjo hahnjo added this to Issues in Fixed in 6.32.02 via automation Jun 5, 2024
hahnjo pushed a commit that referenced this issue Jun 5, 2024
This allows `static_assert(false)` to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

* test/SemaTemplate/instantiation-dependence.cpp
* test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to `static_assert`
still allow that sort of tests to be expressed.

Reviewed By: #clang-language-wg, erichkeane, aaron.ballman

Differential Revision: https://reviews.llvm.org/D144285

---

Fixes the build with newer versions of MSVC's STL, reported as
#15321

(cherry picked from commit c767271)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants