Skip to content

[windows] work-around a compiler bug with VS 2022 v17.6.0#13071

Merged
hahnjo merged 1 commit into
root-project:masterfrom
bellenot:workaround-msvc-bug
Jun 24, 2023
Merged

[windows] work-around a compiler bug with VS 2022 v17.6.0#13071
hahnjo merged 1 commit into
root-project:masterfrom
bellenot:workaround-msvc-bug

Conversation

@bellenot
Copy link
Copy Markdown
Member

@bellenot bellenot commented Jun 22, 2023

Fix a crash in root.exe due to a compiler bug with Visual Studio 2022 v17.6.0. The fix works the same way than it is supposed to do, since the (never reached) default: break; statement make the function returns HasREX anyway. To be reviewed and removed as soon as VS works again

@bellenot bellenot requested a review from Axel-Naumann June 22, 2023 09:28
@bellenot bellenot self-assigned this Jun 22, 2023
@bellenot bellenot requested a review from vgvassilev as a code owner June 22, 2023 09:28
@phsft-bot

This comment was marked as outdated.

Sign in to view
@phsft-bot

This comment was marked as outdated.

Sign in to view
@phsft-bot

This comment was marked as outdated.

Sign in to view
@phsft-bot

This comment was marked as outdated.

Sign in to view
@phsft-bot

This comment was marked as outdated.

Sign in to view
@phsft-bot

This comment was marked as outdated.

Sign in to view
@hahnjo

This comment was marked as outdated.

Sign in to view
@phsft-bot

This comment was marked as outdated.

Sign in to view
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jun 22, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/cxx14
How to customize builds

hahnjo
hahnjo requested changes Jun 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be synced into the monorepo. I can take care of that tomorrow, if we also agree on merging soon after (I'd like #13049 to go in before Monday).

@vgvassilev
Copy link
Copy Markdown
Member

vgvassilev commented Jun 22, 2023

Did you have a look if that’s not already fixed in llvm upstream and possibly backport the fix/workaround?

@bellenot
Copy link
Copy Markdown
Member Author

bellenot commented Jun 22, 2023 •
edited
Loading

Did you have a look if that’s not already fixed in llvm upstream and possibly backport the fix/workaround?

Yes, but the whole file is completely different

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jun 22, 2023

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

@hahnjo
Copy link
Copy Markdown
Member

hahnjo commented Jun 22, 2023

Did you have a look if that’s not already fixed in llvm upstream and possibly backport the fix/workaround?

Yes, but the whole file is completely different

The switch statement is still there:
https://github.com/llvm/llvm-project/blob/3ea8f2526541884e03d5bd4f4e46f4eb190990b6/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp#L812-L815
However, there are now function calls before the return, so potentially the compiler bug doesn't trigger...

@bellenot @hahnjo
[windows] work-around a compiler bug with VS 2022 v17.6.0
e88a2f4 
Fix a crash in root.exe due to a compiler bug with Visual Studio 2022
v17.6.0. The fix works the same way than it is supposed to do, since
the (never reached) `default: break;` statement make the function
returns `HasREX` anyway.
To be reviewed and removed as soon as VS works again.
@hahnjo hahnjo force-pushed the workaround-msvc-bug branch from 27fe257 to e88a2f4 Compare June 23, 2023 09:56
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jun 23, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/cxx14
How to customize builds

hahnjo
hahnjo approved these changes Jun 23, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the monorepo and changed interpreter/llvm/llvm-project.tag, so should be good from that perspective.

I discussed with Bertrand, and this work-around is required to get ROOT compiling with the latest compiler. I think we should do this, even temporarily, until the compiler is fixed.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 24, 2023

Test Results

         8 files           8 suites   1d 23h 6m 43s ⏱️
  2 470 tests   2 470 ✔️ 0 💤 0 ❌
18 816 runs  18 816 ✔️ 0 💤 0 ❌

Results for commit e88a2f4.

@hahnjo hahnjo merged commit e70af82 into root-project:master Jun 24, 2023
@bellenot
Copy link
Copy Markdown
Member Author

bellenot commented Jul 12, 2023

FYI, from the release notes of the last VS update (v17.6.5):

Fixed an issue where switches over unsigned 64-bit integers could cause a crash on x86 and ARM targets.

@bellenot bellenot deleted the workaround-msvc-bug branch March 18, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hahnjo hahnjo hahnjo approved these changes

@vgvassilev vgvassilev vgvassilev approved these changes

@Axel-Naumann Axel-Naumann Awaiting requested review from Axel-Naumann

Assignees

@bellenot bellenot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bellenot @phsft-bot @hahnjo @vgvassilev